[coreboot] superiotool sometimes skips further SuperIO chips in the system - suggested remedy included

[Frantisek Rysanek]

Dear gentlemen,

thanks for the polite responses from Mr. Hendricks and Herr Reinauer.
I have some further notes... As top-posting is a big no-no at least 
in the LKML, see my further comments way below :-)


--------------------------------------------------
On 28 Dec 2010 at 23:51, David Hendricks wrote:

Heh, I hacked that "if (chip_found) return;" stuff into SuperI/O tool 
a few months ago (http://www.coreboot.org/pipermail/coreboot/2010-
August/059607.html). 
The reason was to avoid duplicate listings. In my specific case, I 
was working with an ITE chip that does not need an entry/exit 
sequence. So when probe_regs_ite() was called, it would find my chip 
5 times and dump its registers 5 times. Sorry if it's causing you 
headaches now :-/ 
I think long-term we should make superiotool more intelligent with 
regard to duplicate entries. For example, if a chip has been found at 
0x2e, then superiotool should stop probing devices on port 0x2e and 
continue probing devices on ports 0x4e, 0x3f0, etc (as listed in 
superioto_ports_table[]). I think we could accomplish this easily: 
- Add a return code to each chip's probe_idregs_* function. 
- Update the loop in main() which iterates thru 
superio_ports_table[]. If a probe_idregs_* function returns a 
specific code to indicate a chip has been found, then record the 
corresponding superio port entry and avoid further calls to 
superio_ports_table[i].probe_idregs() on that port. 
 

--------------------------------------------------
On 30 Dec 2010 at 12:41, Stefan Reinauer wrote:
    
    Therefore, after the first chip detected (of any kind),
    only the first init sequence is ever tried, in any subsequent calls
    to the aforementioned probe_* functions. 
    
    That function is called with a port address. There can only be one 
    chip at one port address. So leaving the function after a chip has 
    been found at that address seems like the right thing to do. The bug 
    is that chip_found is a global variable and not a local variable.
    
    Attached patch should fix the issue. 
And an equivalent change is needed for winbond.c I guess.



=======   Okay here are my comments: =======

I have to say that I agree with Mr. Hendricks.
Once you find a chip at a particular port, it should not make sense 
to probe *at that particular port* again. So you need some global way 
of tagging I/O ports already occupied.

I do not think that making the "chip_found" local to the particular 
vendor-specific probe function would solve the problem. Yes it does 
make it scan all the different init sequences across all the ports,
but other vendor-specific modules will re-scan the ports anyway (as 
the info about ports already occupied doesn't exist in the global 
scope).

Now... the way the topmost superio_ports_table[vendor]->ports[] is 
organized, indeed the same port will be scanned by multiple probes. 
The I/O port is a "leaf attribute" of a vendor in the data model, and 
is not indexed upon in any way.
I believe this calls for a slight "reinforcement" of the data model.
Specifically: It seems to me that superiotool.c would have to keep an 
additional index (even just a list with some manipulator functions) 
of ports already known to be occupied.

We'll need to retain the mapping of vendor->ports. 
It doesn't seem very fruitful to re-factor the table as port->vendors 
:-) Or does it?
Side note: at a fist glance, it seemd to me that for some advanced 
functionality (writing to ports, separation of "probe" from "dump" 
etc) it might be interesting to create a global list of "devices" 
detected. This would have the neat side effect of having a list of 
the IO ports already probed... but at a closer look, it seems that 
the vendor-specific modules are much too "polymorphic", the "device 
private data" would have to be polymorphic (vendor-specific), and all 
the vendor-specific code would have to be somewhat heavily refactored 
in quite an individual fashion, to meet the new and more refined 
"abstraction model"... looks like too much work and very little 
immediate use.
I have to say that I've dipped my toes in some intermediate-level 
C++, but I can also sculpt encapsulation and polymorphism in bare 
C... yet in this case I don't think there's any call for either of 
that.

Another minor point: it seems interesting to me that the 
superio_ports_table is declared+defined "static const" 
_in_the_header_file_. Thus, each module that includes superiotool.h 
instantiates that table, even though only superiotool.c ever makes 
any use of it. The obvious solution would be to move the 
superio_ports_table to superiotool.c. Or maybe break it up and make 
the tables of ports "private" to the individual vendor modules, 
managed/walked by the per-module probe functions. Yet for the moment 
I admit that there is indeed certain beauty (elegance?) in having all 
that data in a single table in superiotool.c, in a common 
structure... so I'd probably just keep that arrangement :-)

I am a newcomer :-) Sorry for talking so much.
At the same time, I don't like adding further layers of cruft on top 
of each other - especially if this has been incited by my own 
"feedback".
The Superiotool looks like a neatly compact piece of source code - 
small enough for me to understand / encompass :-)
I'd like to offer a bit of my own time to add such a global index of 
"ports occupied" (likely not "devices detected" - see above why).
I'd also have to touch all the vendor-specific modules, but only very 
slightly (to make the probe functions return a result code). I'd have 
to undo the interim hacks (sorry) added by Mr. Hendricks and Mr. 
Reinauer.
I'd probably start from the version that I've checked out right now 
from SVN. I don't know how to return to an earlier version, and 
besides there has been a further detection update to the ITE module, 
on top of Mr. Reinauer's recent change...

Would you be interested?
It might take me days of real time - my job and family allow 
relatively little time for ad hoc meddling... :-)

Frank Rysanek