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