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.scott@xxxxxxxxxxxxxxxxxxx> >> --- >> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html