Am Samstag, den 30.06.2012, 01:40 +0200 schrieb Carl-Daniel Hailfinger:
This patch is essentially a small code move, better device counting and lots of indentation changes (look at diff -uw to see real code changes).
So basically you are moving the device test level check from pcidev_validate to pcidev_init. The result is that pcidev_validate does no longer in fact validate whether a candidate device is the correct device, as the name implies. A name like "pcidev_readbar" would make more sense, IMHO.
Now, check where pcidev_validate is called from. It is called from pcidev_init to do its original job, namely validating a device, and it is called from nicintel code to get a second BAR, because the first BAR returned by pcidev_init is not really enough. That invocation of pcidev_validate already carries a nice comment reading "/* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */". This indicates that something is wrong with this code. And in my oppinion, indeed, it is. The problem is that the current code contains an abstraction violation. Neither pcidev_validate (in the pre-patch form) is really meant to be called from higher level code, but it is meant to be called by pcidev_init only (doing it's job, validating a PCI device, poorly), nor is using a global variable pcidev_dev a good idea for communication between pcidev.c and the clients of that code.
Furthermore, pcidev_init (conveniently, I admit) does two things at once: It finds a PCI device, and it reads one BAR. What about having pcidev_init return the struct pci_dev* instead, so it is up to the client driver to handle that the value instead of putting it in a global variable? Of course, this implies all clients have to use something like pcidev_readbar (the post-patch pcidev_validate) function, even if just a single address is needed. Considering our current codebase, which uses pci_read_long to get base addresses in satamv and satasii, I think giving pcidev_readbar some extra exposure would even be a good educational thing.
And as I go along ranting about the current state of that code, I think it is nice that pcidev_validate does so many sanity checks, but there is one thing I do *not* like about it, you could call it a "missing vital sanity check". pcidev_validate checks whether the given BAR is I/O mapped or memory mapped, and handles each of those types correctly. Great! Finally, it returns the obtained base address, which might either be in I/O or memory space to the caller, *not* indicating whether it just returned an I/O or a memory address. This is quite "convenient", as the callers (should) know what type they expect, and they are not prepared to handle mismatches[1], but it would be even more convenient if this quite important verification of the address space would be performed. So pcidev_readbar (the new pcidev_validate) needs an extra parameter, which should indicate I/O or memory space (or, alternatively, provide pcidev_readbar_io and pcidev_readbar_mem, as there are some strict coding styles claiming the flag parameters are evil, because if there are two different ways a function can be executed (selected by that flag parameter), it really means there are two functions which should have their own name) and have it fail if the BAR type does not match the callers expectation.
Considering all the stuff I was writing, I think your current patch might be the first of a series leading towards a better pcidev interface, if you apply the name fix for pcidev_validate.
I guess the second step would be removing the global pcidev_dev variable, which implies all callers of pcidev_init need to be adjusted. Maybe for modules that really only need one BAR, we can still provide the old behaviour as "pcidev_init_getbar" or "pcidev_init_simple".
In a third step, add I/O vs. memory validation to pcidev_readbar (ex pcidev_validate), and remove all pci_read_long(...) &~7; invocations to read BARs.
So the final vote is:
If you rename pcidev_validate to pcidev_readbar, this is Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
[1]: Remember the old saying: "Never check for an error condition you don't know how to handle"