On Sat, 2024-04-27 at 10:00 -0700, Guenter Roeck wrote: > On 4/27/24 04:53, Frank Crawford wrote: > > On Sat, 2024-04-27 at 04:11 -0700, Guenter Roeck wrote: > > > On 4/27/24 01:33, Frank Crawford wrote: > > ... > > > > @@ -3144,7 +3186,7 @@ static int __init it87_find(int sioaddr, > > > > unsigned short *address, > > > > } > > > > > > > > exit: > > > > - superio_exit(sioaddr, config ? has_noconf(config) : false); > > > > + superio_exit(sioaddr, opened && config && > > > > has_noconf(config)); > > > > > > If 'opened' is false, this could be an affected chip. Are you > > > sure > > > that it makes sense to pass 'false' as parameter here in that > > > case ? > > > Doesn't that mean that the chip might be one of the affected > > > chips, > > > but the superio exit sequence would be executed anyway ? > > > Am I missing something ? > > > > Ohh, you may be right, I think I have got myself confused here with > > opened and how it is used in superio_exit. > > > > It took me a while to understand as well. The double negation of the > 'noentry' parameter makes it difficult to understand. > > > I think it should be !opened, but I will just check I still haven't > > messed it up. > > > > Maybe it should be something like > !config && !opened || /* no or unknown chip, not enabled */ > config && has_noconf(config) /* chip known to be affected */ > > which would translate "don't disable configuration mode for affected > chips > and for unknown chips if configuration mode was not enabled". In fact, I think in this case that entire expression can be simplified to just "!opened", as we only want to perform the exit code when the entry code was given, and it is only set to true in those cases. The only points where there is some "concern" is where it is an unknown chipID, in which case we should not send the exit unless we previously needed it to get a chipID (so probably a valid chip we don't know about), or in the case of an error, which almost certainly means that sending the code will have no effect, and is actually the same as previously anyway. > > Btw, I think "enabled" might be a better variable name. Yes, it probably is. I'll rename the variable. I'll send a new version of this patch with those updates. > > Thanks, > Guenter Regards Frank
