On Fri, 2024-04-12 at 05:48 -0700, Guenter Roeck wrote: > On Fri, Apr 12, 2024 at 08:11:11PM +1000, Frank Crawford wrote: > > > > > > Not really. There is also the watchdog code which will happily > > > disable > > > SIO access after it is done, causing subsequent accesses by the > > > hwmon > > > driver to fail. The code also assumes that SIO access was not > > > erroneously > > > left enabled by some other code which we don't have any control > > > over. > > > > And unfortunately if I can't do anything about it, I can only > > ignore > > it. If something does come up we can see what can work out at the > > time. > > > > > > You assume that the hwmon driver is the only driver accessing the > > > chip. > > > That is a wrong assumption. I understand that the underlying > > > problem > > > is really that there is no SIO access infrastructure in the > > > kernel. > > > In the absence of such an infrastructure we can not make any > > > assumptions > > > about SIO access control implemented by other drivers in the > > > kernel, > > > and specifically can not assume that SIO access won't be disabled > > > by > > > other drivers just because it was enabled when the hwmon driver > > > probe > > > function was running. > > > > In this case the fact that it is the second chip may mean it will > > not > > come up. While I am told that the chip is fully functional with > > non- > > hwmon functions, but currently it does look like most of those > > aren't > > used. While this won't necessarily stay this way in the future, we > > currently cannot do anything about it. > > This patch affects all chips, not just the second one. If any chip is > in configuration mode when instantiating this driver, configuration > mode > won't be enabled anymore, no matter what other drivers may or may not > do. > That includes situations where other drivers (or the BIOS) > erroneously > do not disable configuration mode. > > I understand your reasoning about not enabling configuration mode for > certain chips, but that does not explain why it would be necessary > to do this for all chips all the time. > > Sure, there is something we can do: Unless there is a known problem > that affects _all_ chips, drop this patch. Ahh, if that is the impression you have from my description, then I will need to improve it, as that is not the case. The actual update does the following: 1) Lock the memory, but does not perform a SIO entry (previously it would have performed an SIO entry). 2) Attempt to read the chipID. This should be safe no matter which chip we have. 3) If step (2) fails, then perform SIO entry and retry chipID read. If it fails, act similarly to prior to this patch. 4) Set the sio_data->type, similar to previously. 5) If we have not performed an SIO entry, and this is not a chip type with the NOCONF feature, then it will perform an SIO entry at this point. 6) Proceed setup as prior to this patch. 7) Any following access to the SIO registers will invoke the SIO entry and SIO exit steps unless it is a chip with the NOCONF feature set. This was set up in the previous patches in this patchset. 8) There is also some minor update to the failure exit based on if it had performed a SIO entry or not, in addition to the previous tests. > > Thanks, > Guenter Regards Frank
