On Thu, 28 May 2020 06:52:37 -0700, Guenter Roeck wrote: > On 5/28/20 3:02 AM, Jean Delvare wrote: > > Well it's still in use and apparently at least one user cares enough to > > report a bug and propose a candidate fix. > > ... but at the same time that user doesn't have any temperature sensors > installed and won't be able to test any changes. They would be able to test the rest of the driver still. Plus I have a register dump which I am feeding i2c-stub with, and that lets me test the driver to some extent. God bless Mark M. Hoffman! What I can't test are timing issues, and hardware misbehavior as described by Darrick. > > (...) > > 4* Why are you calling msleep_interruptible() in > > adt7470_read_temperatures() to wait for the temperature conversions? We > > return -EAGAIN if that happens, but then ignore that error code, and we > > log a cryptic error message. Do I understand correctly that the only > > case where this should happen is when the user unloads the kernel > > driver, in which case we do not care about having been interrupted? I > > can't actually get the error message to be logged when rmmod'ing the > > module so I don't know what it would take to trigger it. > > Not sure if rmmod generates a signal. I tested and it doesn't seem so. I expected rmmod to call adt7470_remove(), in turn calling kthread_stop() and I thought this would interrupt the thread. But the interrupt message never gets logged. "modprobe adt7470 && rmmod adt7470" takes 2 seconds, so I assume that rmmod gives the thread all the time it wants to exit on its own (through kthread_should_stop()). > In theory you could possibly > keep writing -1 into the num_temp_sensors attribute, execute the > sensors command (or just read a temperature), and interrupt the > sequence. Interrupt how? I tried Ctrl+C while "sensors" is waiting for the driver to update the values, but it waits for completion before actually exiting. So far I couldn't find any way to get this msleep_interruptible() to actually get interrupted. > (...) > The datasheet says nothing about the need to run such a thread for > automatic mode either. But that at least makes some sense due to the external nature of the thermal sensors. The data needs to be fetched from the slaves somehow before you can read it from the ADT7470's registers. On the other hand, having to change PWM configuration registers for temperature readings to be correct (/if/ that's what is happening here... not sure) is highly unexpected. > As I understand it, the chip is supposed to > repeat temperature measurements automatically once configuration > register 1 bit 7 is set. Of course that is difficult if not > impossible to confirm without access to the chip. Well there's clearly a huge design mistake for that chip, sharing a pin between FULL_SPEED and TMP_START makes it pretty much impossible for automatic temperature monitoring to be implemented without a software loop. And a hardware monitor device that can't run on its own is just asking for trouble. Oh well. -- Jean Delvare SUSE L3 Support
