Hi Guenter, Jean,
On 09/02/2016 12:12 AM, Guenter Roeck wrote:
> On 08/31/2016 10:17 PM, Joshua Scott wrote:
>> adt7470_remove will wait for the update thread to complete before
>> returning. This has a worst-case time of up to the user-configurable
>> auto_update_interval.
>>
>> Break this delay into smaller slices to allow the thread to exit quickly
>> when adt7470_remove is called.
>>
>> Signed-off-by: Joshua Scott <joshua.sc...@alliedtelesis.co.nz>
>> ---
>> drivers/hwmon/adt7470.c | 15 +++++++++++----
>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
>> index 7d185a9..ba97392 100644
>> --- a/drivers/hwmon/adt7470.c
>> +++ b/drivers/hwmon/adt7470.c
>> @@ -268,14 +268,21 @@ static int adt7470_update_thread(void *p)
>> {
>> struct i2c_client *client = p;
>> struct adt7470_data *data = i2c_get_clientdata(client);
>> + unsigned long next_read = jiffies - 1;
>>
>> while (!kthread_should_stop()) {
>> - mutex_lock(&data->lock);
>> - adt7470_read_temperatures(client, data);
>> - mutex_unlock(&data->lock);
>> +
>> + if (time_after_eq(jiffies, next_read)) {
>> + next_read = jiffies + data->auto_update_interval * HZ /
>> 1000;
>> + mutex_lock(&data->lock);
>> + adt7470_read_temperatures(client, data);
>> + mutex_unlock(&data->lock);
>> + }
>> +
>> + msleep_interruptible(1);
>> +
>
>
> This puts a heavy burden on the system, forcing it to run every ms, just for
> the unlikely case of driver removal. Why is quick removal so important ?
> If it is, we'll have to find a better solution.
>
> Guenter
>
The particular use case we have is a chassis based system with an
adt7470 on a removable fan-tray. The problem is that when the fan tray
is removed it takes auto_update_interval/1000 seconds for the update
thread to stop and the i2c device to be removed. In the intervening time
a new fan-tray could have been installed.
>> if (kthread_should_stop())
>> break;
>> - msleep_interruptible(data->auto_update_interval);
>> }
>>
>> complete_all(&data->auto_update_stop);
>>
On 09/01/2016 08:18 PM, Jean Delvare wrote:
>
> This change looks terribly costly to me. In order to shorten the
> duration of a rare event (you don't "rmmod adt7470" on a regular basis,
> do you?)
It's worse than that. We're not doing rmmod, hardware is physically
being removed.
> you increase the cost of a kernel thread which runs all the
> time. I'm afraid this msleep_interruptible(1) is going to prevent the
> CPU from entering low power states at all, leading to an increased
> system temperature and power consumption. Have you compared the output
> of "powertop" (specifically the "Idle stats" section) before and after
> your patch?
>
> Is there no way to voluntarily interrupt this long msleep_interruptible?
>
If there is I'd like to know. That would be the ideal solution for us.
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
