[flashrom] flashrom -p gfxnvidia detect my atheros ath9k wifi card

[Michael Karcher]

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 at mkarcher.dialup.fu-berlin.de>

[1]: Remember the old saying: "Never check for an error condition you
don't know how to handle"