On Thu, 23 Mar 2023, Mark Cave-Ayland wrote:
On 18/03/2023 21:37, BALATON Zoltan wrote:
This is untested and probably should be marked RFC as it's mainly to show what I was suggesting as an alternative fix for the decode-unit problem discovered by Mark. This could be taken further to adopt the decode-unit and encode-unit words from openfirmware to reduce clutter in pci.c even more which I may attempt if this direction is accepted. My config words patch will also need to be rebased but let this part settle fitst and I can submit the rest as a follow up or a v2 for this series later.
Regards,
BALATON Zoltan (3): pci: Use forth to add trivial open/close methods pci: Add is-pci-bus word to install some methods from forth pci: Fix calling decode-unit without active-package
drivers/pci.c | 78 +++++--------------------------------------------- drivers/pci.fs | 10 +++++++ 2 files changed, 17 insertions(+), 71 deletions(-)
Thanks for posting an RFC for discussion. There are two separate meanings to the word "simplify" in the context of this patch: the first is being concise, in which Forth can often provide a very compact solution as demonstrated in patch 3. The second is being understandable to developers which improves maintainability in the long run.
What is it that's not understandable in these methods? I can understand it but cannot understand yours. Since forth is a fundamental part of Open Firmware and we already have a lot of it in OpenBIOS people who maintain it are expected to be at least somewhat familiar with forth so it should not be an issue to use it. I think this series simplifies this in both sense of the word and avoids introducing additional complexity unlike your approach.
I think it's easy to argue that patch 3 meets the first criteria, but as mentioned before I'm not particularly keen on this approach because it starts splitting the core PCI methods out of pci.c and into pci.fs whilst mixing languages and runtime/compile time semantics, and also as per my previous comment you'll end up embedding strings of Forth which is really something to be avoided.
We already have a lot of embedded forth strings all over the place, also in pci.c. Look at ebus_config_cb() for example. So why is the one line added by this series worse than those already there? Splitting methods between forth and C may also not be a problem. My original proposal was to move all pci bus methods to forth and I'm willing to do that if this was accepted but this series only demostrated the idea because I don't want to spend time on patches that will be discarded. The decode/encode unit words can be taken from openfirmware and I had a patch to implement map-in in forth which is currently broken and needs to be fixed anyway. The rest are just trivial call parents so at the end we could get rid of most of these. Leaving some in C when it's easier to do that should also not be a problem. We have two languages in OpenBIOS and we can use both and we already do that everywhere. You also mix forth and C otherwise you would not need to patch the bridge between them.
I can see how the first couple of patches can appear opaque, but then it's easy to see that they work in the same way as the existing BIND_NODE_METHODS macro. And from a developer perspective this is something that "just works" in that you use the new macro, and you end up with the phandle as a C function parameter.
I'm still not convinced that is needed. The node methods already take a parameter. What is that? Can you get the needed info from that parameter? Or can you do the same that openfirmware does but from C? Passing a phandle to a method seems like a hack to me.
If there were already a significant amount of Forth code in the PCI bindings then a Forth solution such as yours would be a better approach, however for now my preference would be to use the approach from my original series since the glue code provides a much more intuitive solution given that the PCI bindings are written in C.
I disagree but since nobody else here seems to have an opinion or care about OpenBIOS in the end you do what you want. Forth and C are mixed in OpenBIOS also in pci.c already so using that to make pci.c less cluttered and easier to read by adding some very simple forth that's also easily understandable would make this easier to maintain but if this did not convince you I can't do much else.
Regards, BALATON Zoltan