On 5/28/20 3:02 AM, Jean Delvare wrote: > On Wed, 27 May 2020 16:33:34 -0700, Darrick J. Wong wrote: >> On Wed, May 27, 2020 at 03:42:52PM -0700, Guenter Roeck wrote: >>> On Tue, May 26, 2020 at 11:22:59AM +0200, Jean Delvare wrote: >>>> Hi all, >>>> >>>> In the context of bug #207771, I got to look into the adt7470 driver. >>>> I'm slowing understanding the logic of the background temperature >>>> registers update thread, still there are 2 things I do not understand: >>>> >>>> 1* Function adt7470_read_temperatures() sets data->num_temp_sensors, >>>> however this value seems to be only used to limit the wait time of >>>> future calls to the same function. In the general update function we >>>> still read ALL temperature sensors regardless of its value: >>>> >>>> for (i = 0; i < ADT7470_TEMP_COUNT; i++) >>>> data->temp[i] = i2c_smbus_read_byte_data(client, >>>> ADT7470_TEMP_REG(i)); >>>> >>>> Shouldn't this loop be bounded with data->num_temp_sensors instead of >>>> ADT7470_TEMP_COUNT? >>>> >>> Guess so. >>> >>>> 2* Function adt7470_read_temperatures() also sets >>>> data->temperatures_probed to 1, and this boolean is then used to skip >>>> further calls to that function. But do we really need a separate >>>> variable for this, given that num_temp_sensors >= 0 matches the same >>>> condition as far as I can see? >>> >>> Agreed. On the other side, those are optimizations. I am not really sure >>> if that driver is worth spending time on, given the age of the chip. > > 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. >> I /think/ the answer to both questions is yes, but I don't have the >> hardware anymore so I have no way to QA that... :/ > > Thanks for both of you for your answers. > > Darrick, I have 3 more questions for you if you remember... > > 3* I understand that the temperatures need to be read periodically in > order for automatic fan speed control to work. What I do not understand > is why you bother switching PWM outputs to manual mode each time? What > bad would happen if we did not do that? I see nothing in the datasheet > justifying it. > > 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. 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. > 5* Is there any reason why the update thread is being started > unconditionally? As I understand it, it is only needed if at least one > PWM output is configured in automatic mode, which (I think) is not the > default. It is odd that the bug reporter hits a problem with the > polling thread when they are not using automatic fan speed control in > the first place so they do not need the polling thread to run. I was > wondering if it would be possible to start and stop the polling thread > depending on whether automatic PWM is enabled or not. > The datasheet says nothing about the need to run such a thread for automatic mode either. 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. Guenter
