[lm-sensors] [PATCH] adt7470: Update sensors periodically via timer

[lm-sensors] [PATCH] adt7470: Update sensors periodically via timer

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 8468 bytes --]

To avoid blocking for 1s in the sysfs read functions, make it so
that adt7470 updates are done periodically with a timer instead
of being called from the read functions directly.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 drivers/hwmon/adt7470.c |   71 +++++++++++++++++++++++++++++++++++------------
 1 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 75aee3b..bfb0072 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -28,6 +28,7 @@
 #include <linux/mutex.h>
 #include <linux/delay.h>
 #include <linux/log2.h>
+#include <linux/timer.h>
 
 /* Addresses to scan */
 static unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END };
@@ -124,6 +125,8 @@ struct adt7470_data {
 	struct mutex		lock;
 	char			valid;
 	unsigned long		last_updated;	/* In jiffies */
+	struct timer_list	timer;
+	struct work_struct	ref_work;
 
 	s8			temp[ADT7470_TEMP_COUNT];
 	s8			temp_min[ADT7470_TEMP_COUNT];
@@ -184,10 +187,9 @@ static void adt7470_init_client(struct i2c_client *client)
 	}
 }
 
-static struct adt7470_data *adt7470_update_device(struct device *dev)
+static void adt7470_update_data(struct adt7470_data *data)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct adt7470_data *data = i2c_get_clientdata(client);
+	struct i2c_client *client = &data->client;
 	u8 cfg;
 	int i;
 
@@ -277,6 +279,29 @@ static struct adt7470_data *adt7470_update_device(struct device *dev)
 
 out:
 	mutex_unlock(&data->lock);
+}
+
+static void adt7470_work_function(struct work_struct *work)
+{
+	struct adt7470_data *data =
+		container_of(work, struct adt7470_data, ref_work);
+
+	adt7470_update_data(data);
+	mod_timer(&data->timer, jiffies + REFRESH_INTERVAL);
+}
+
+static void adt7470_timer_function(unsigned long x)
+{
+	struct adt7470_data *data = (struct adt7470_data *)x;
+
+	schedule_work(&data->ref_work);
+}
+
+static struct adt7470_data *dev_to_adt7470_data(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7470_data *data = i2c_get_clientdata(client);
+
 	return data;
 }
 
@@ -285,7 +310,7 @@ static ssize_t show_temp_min(struct device *dev,
 			     char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 	return sprintf(buf, "%d\n", 1000 * data->temp_min[attr->index]);
 }
 
@@ -313,7 +338,7 @@ static ssize_t show_temp_max(struct device *dev,
 			     char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 	return sprintf(buf, "%d\n", 1000 * data->temp_max[attr->index]);
 }
 
@@ -340,7 +365,7 @@ static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
 			 char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 	return sprintf(buf, "%d\n", 1000 * data->temp[attr->index]);
 }
 
@@ -349,7 +374,7 @@ static ssize_t show_alarms(struct device *dev,
 			   char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 
 	if (attr->index)
 		return sprintf(buf, "%x\n", data->alarms);
@@ -362,7 +387,7 @@ static ssize_t show_fan_max(struct device *dev,
 			    char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 
 	if (FAN_DATA_VALID(data->fan_max[attr->index]))
 		return sprintf(buf, "%d\n",
@@ -397,7 +422,7 @@ static ssize_t show_fan_min(struct device *dev,
 			    char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 
 	if (FAN_DATA_VALID(data->fan_min[attr->index]))
 		return sprintf(buf, "%d\n",
@@ -431,7 +456,7 @@ static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
 			char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 
 	if (FAN_DATA_VALID(data->fan[attr->index]))
 		return sprintf(buf, "%d\n",
@@ -444,7 +469,7 @@ static ssize_t show_force_pwm_max(struct device *dev,
 				  struct device_attribute *devattr,
 				  char *buf)
 {
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 	return sprintf(buf, "%d\n", data->force_pwm_max);
 }
 
@@ -475,7 +500,7 @@ static ssize_t show_pwm(struct device *dev, struct device_attribute *devattr,
 			char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 	return sprintf(buf, "%d\n", data->pwm[attr->index]);
 }
 
@@ -500,7 +525,7 @@ static ssize_t show_pwm_max(struct device *dev,
 			    char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 	return sprintf(buf, "%d\n", data->pwm_max[attr->index]);
 }
 
@@ -528,7 +553,7 @@ static ssize_t show_pwm_min(struct device *dev,
 			    char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 	return sprintf(buf, "%d\n", data->pwm_min[attr->index]);
 }
 
@@ -556,7 +581,7 @@ static ssize_t show_pwm_tmax(struct device *dev,
 			     char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 	/* the datasheet says that tmax = tmin + 20C */
 	return sprintf(buf, "%d\n", 1000 * (20 + data->pwm_tmin[attr->index]));
 }
@@ -566,7 +591,7 @@ static ssize_t show_pwm_tmin(struct device *dev,
 			     char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 	return sprintf(buf, "%d\n", 1000 * data->pwm_tmin[attr->index]);
 }
 
@@ -594,7 +619,7 @@ static ssize_t show_pwm_auto(struct device *dev,
 			     char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 	return sprintf(buf, "%d\n", 1 + data->pwm_automatic[attr->index]);
 }
 
@@ -638,7 +663,7 @@ static ssize_t show_pwm_auto_temp(struct device *dev,
 				  char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct adt7470_data *data = adt7470_update_device(dev);
+	struct adt7470_data *data = dev_to_adt7470_data(dev);
 	u8 ctrl = data->pwm_auto_temp[attr->index];
 
 	if (ctrl)
@@ -913,6 +938,14 @@ static int adt7470_detect(struct i2c_adapter *adapter, int address, int kind)
 		goto exit_remove;
 	}
 
+	/* Initialize automatic sensor refresh */
+	INIT_WORK(&data->ref_work, adt7470_work_function);
+	init_timer(&data->timer);
+	data->timer.expires = jiffies + REFRESH_INTERVAL;
+	data->timer.data = (unsigned long)data;
+	data->timer.function = adt7470_timer_function;
+	add_timer(&data->timer);
+
 	return 0;
 
 exit_remove:
@@ -930,6 +963,8 @@ static int adt7470_detach_client(struct i2c_client *client)
 	struct adt7470_data *data = i2c_get_clientdata(client);
 	int i;
 
+	del_timer_sync(&data->timer);
+	cancel_work_sync(&data->ref_work);
 	hwmon_device_unregister(data->class_dev);
 	for (i = 0; i < ARRAY_SIZE(adt7470_attr); i++)
 		device_remove_file(&client->dev, &adt7470_attr[i].dev_attr);

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adt7470: Update sensors periodically via

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 410 bytes --]

On Wed, Jul 25, 2007 at 06:04:21PM -0700, Darrick J. Wong wrote:
> To avoid blocking for 1s in the sysfs read functions, make it so
> that adt7470 updates are done periodically with a timer instead
> of being called from the read functions directly.

Hm... sleeping for long periods of time in the system workqueue causes
the console to become really slow.  Will migrate to a separate workqueue
tomorrow.

--D

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adt7470: Update sensors periodically via

[Juerg Haefliger]

Hi Darrick,

Why using a workqueue at all? Can't you just run a timer and do the
update when it expires and restart the timer?


...juerg


On 7/25/07, Darrick J. Wong <djwong@us.ibm.com> wrote:
> To avoid blocking for 1s in the sysfs read functions, make it so
> that adt7470 updates are done periodically with a timer instead
> of being called from the read functions directly.
>
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
>
>  drivers/hwmon/adt7470.c |   71 +++++++++++++++++++++++++++++++++++------------
>  1 files changed, 53 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 75aee3b..bfb0072 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c
> @@ -28,6 +28,7 @@
>  #include <linux/mutex.h>
>  #include <linux/delay.h>
>  #include <linux/log2.h>
> +#include <linux/timer.h>
>
>  /* Addresses to scan */
>  static unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END };
> @@ -124,6 +125,8 @@ struct adt7470_data {
>         struct mutex            lock;
>         char                    valid;
>         unsigned long           last_updated;   /* In jiffies */
> +       struct timer_list       timer;
> +       struct work_struct      ref_work;
>
>         s8                      temp[ADT7470_TEMP_COUNT];
>         s8                      temp_min[ADT7470_TEMP_COUNT];
> @@ -184,10 +187,9 @@ static void adt7470_init_client(struct i2c_client *client)
>         }
>  }
>
> -static struct adt7470_data *adt7470_update_device(struct device *dev)
> +static void adt7470_update_data(struct adt7470_data *data)
>  {
> -       struct i2c_client *client = to_i2c_client(dev);
> -       struct adt7470_data *data = i2c_get_clientdata(client);
> +       struct i2c_client *client = &data->client;
>         u8 cfg;
>         int i;
>
> @@ -277,6 +279,29 @@ static struct adt7470_data *adt7470_update_device(struct device *dev)
>
>  out:
>         mutex_unlock(&data->lock);
> +}
> +
> +static void adt7470_work_function(struct work_struct *work)
> +{
> +       struct adt7470_data *data > +               container_of(work, struct adt7470_data, ref_work);
> +
> +       adt7470_update_data(data);
> +       mod_timer(&data->timer, jiffies + REFRESH_INTERVAL);
> +}
> +
> +static void adt7470_timer_function(unsigned long x)
> +{
> +       struct adt7470_data *data = (struct adt7470_data *)x;
> +
> +       schedule_work(&data->ref_work);
> +}
> +
> +static struct adt7470_data *dev_to_adt7470_data(struct device *dev)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct adt7470_data *data = i2c_get_clientdata(client);
> +
>         return data;
>  }
>
> @@ -285,7 +310,7 @@ static ssize_t show_temp_min(struct device *dev,
>                              char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>         return sprintf(buf, "%d\n", 1000 * data->temp_min[attr->index]);
>  }
>
> @@ -313,7 +338,7 @@ static ssize_t show_temp_max(struct device *dev,
>                              char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>         return sprintf(buf, "%d\n", 1000 * data->temp_max[attr->index]);
>  }
>
> @@ -340,7 +365,7 @@ static ssize_t show_temp(struct device *dev, struct device_attribute *devattr,
>                          char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>         return sprintf(buf, "%d\n", 1000 * data->temp[attr->index]);
>  }
>
> @@ -349,7 +374,7 @@ static ssize_t show_alarms(struct device *dev,
>                            char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>
>         if (attr->index)
>                 return sprintf(buf, "%x\n", data->alarms);
> @@ -362,7 +387,7 @@ static ssize_t show_fan_max(struct device *dev,
>                             char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>
>         if (FAN_DATA_VALID(data->fan_max[attr->index]))
>                 return sprintf(buf, "%d\n",
> @@ -397,7 +422,7 @@ static ssize_t show_fan_min(struct device *dev,
>                             char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>
>         if (FAN_DATA_VALID(data->fan_min[attr->index]))
>                 return sprintf(buf, "%d\n",
> @@ -431,7 +456,7 @@ static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
>                         char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>
>         if (FAN_DATA_VALID(data->fan[attr->index]))
>                 return sprintf(buf, "%d\n",
> @@ -444,7 +469,7 @@ static ssize_t show_force_pwm_max(struct device *dev,
>                                   struct device_attribute *devattr,
>                                   char *buf)
>  {
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>         return sprintf(buf, "%d\n", data->force_pwm_max);
>  }
>
> @@ -475,7 +500,7 @@ static ssize_t show_pwm(struct device *dev, struct device_attribute *devattr,
>                         char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>         return sprintf(buf, "%d\n", data->pwm[attr->index]);
>  }
>
> @@ -500,7 +525,7 @@ static ssize_t show_pwm_max(struct device *dev,
>                             char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>         return sprintf(buf, "%d\n", data->pwm_max[attr->index]);
>  }
>
> @@ -528,7 +553,7 @@ static ssize_t show_pwm_min(struct device *dev,
>                             char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>         return sprintf(buf, "%d\n", data->pwm_min[attr->index]);
>  }
>
> @@ -556,7 +581,7 @@ static ssize_t show_pwm_tmax(struct device *dev,
>                              char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>         /* the datasheet says that tmax = tmin + 20C */
>         return sprintf(buf, "%d\n", 1000 * (20 + data->pwm_tmin[attr->index]));
>  }
> @@ -566,7 +591,7 @@ static ssize_t show_pwm_tmin(struct device *dev,
>                              char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>         return sprintf(buf, "%d\n", 1000 * data->pwm_tmin[attr->index]);
>  }
>
> @@ -594,7 +619,7 @@ static ssize_t show_pwm_auto(struct device *dev,
>                              char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>         return sprintf(buf, "%d\n", 1 + data->pwm_automatic[attr->index]);
>  }
>
> @@ -638,7 +663,7 @@ static ssize_t show_pwm_auto_temp(struct device *dev,
>                                   char *buf)
>  {
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -       struct adt7470_data *data = adt7470_update_device(dev);
> +       struct adt7470_data *data = dev_to_adt7470_data(dev);
>         u8 ctrl = data->pwm_auto_temp[attr->index];
>
>         if (ctrl)
> @@ -913,6 +938,14 @@ static int adt7470_detect(struct i2c_adapter *adapter, int address, int kind)
>                 goto exit_remove;
>         }
>
> +       /* Initialize automatic sensor refresh */
> +       INIT_WORK(&data->ref_work, adt7470_work_function);
> +       init_timer(&data->timer);
> +       data->timer.expires = jiffies + REFRESH_INTERVAL;
> +       data->timer.data = (unsigned long)data;
> +       data->timer.function = adt7470_timer_function;
> +       add_timer(&data->timer);
> +
>         return 0;
>
>  exit_remove:
> @@ -930,6 +963,8 @@ static int adt7470_detach_client(struct i2c_client *client)
>         struct adt7470_data *data = i2c_get_clientdata(client);
>         int i;
>
> +       del_timer_sync(&data->timer);
> +       cancel_work_sync(&data->ref_work);
>         hwmon_device_unregister(data->class_dev);
>         for (i = 0; i < ARRAY_SIZE(adt7470_attr); i++)
>                 device_remove_file(&client->dev, &adt7470_attr[i].dev_attr);
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
>
> iD8DBQFGp/MVa6vRYYgWQuURApPtAKCc8OrLqZ8C7jdpSWfczMPqLcsuuQCfQkSA
> x10Dhzi/rneNPrH4e2pW9WY> =By5u
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adt7470: Update sensors periodically via

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 661 bytes --]

On Wed, Jul 25, 2007 at 06:19:22PM -0700, Juerg Haefliger wrote:
> Hi Darrick,
> 
> Why using a workqueue at all? Can't you just run a timer and do the
> update when it expires and restart the timer?

Currently, I use mutexes (and msleep) which require the ability to
sleep.  Timers can't sleep because they are implemented as a softirq.
That said, I think the solution is to have two workqueue items--one to
start the read and another to run after the temperature sensors have
been read.  The delays for both can be handled via schedule_delayed_work().
It could be done with two timers and spinlocks, but ... yuck.

New patch forthcoming.

--D

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adt7470: Update sensors periodically via

[Hans de Goede]

Darrick J. Wong wrote:
> On Wed, Jul 25, 2007 at 06:19:22PM -0700, Juerg Haefliger wrote:
>> Hi Darrick,
>>
>> Why using a workqueue at all? Can't you just run a timer and do the
>> update when it expires and restart the timer?
> 
> Currently, I use mutexes (and msleep) which require the ability to
> sleep.  Timers can't sleep because they are implemented as a softirq.
> That said, I think the solution is to have two workqueue items--one to
> start the read and another to run after the temperature sensors have
> been read.  The delays for both can be handled via schedule_delayed_work().
> It could be done with two timers and spinlocks, but ... yuck.
>

Or we could decide that we can live with the current 1 second + blocking of 
userspace apps, that is an option we should seriously consider too if this 
becomes ugly.

Regards,

hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adt7470: Update sensors periodically via

[Juerg Haefliger]

Hi Darrick,

I guess I'm not seeing the big picture :-) The problem you're trying
to solve is that the reading of the registers takes a long time and
needs to be able to sleep?

> > Hi Darrick,
> >
> > Why using a workqueue at all? Can't you just run a timer and do the
> > update when it expires and restart the timer?
>
> Currently, I use mutexes (and msleep) which require the ability to
> sleep.  Timers can't sleep because they are implemented as a softirq.

True.


> That said, I think the solution is to have two workqueue items--one to
> start the read and another to run after the temperature sensors have
> been read.  The delays for both can be handled via schedule_delayed_work().
> It could be done with two timers and spinlocks, but ... yuck.

Hmm... Sounds rather complicated. Is there a problem with blocking
apps for that long?
How about forking off a kernel thread instead of using workqueues?

...juerg


> New patch forthcoming.
>
> --D
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
>
> iD8DBQFGqE2ga6vRYYgWQuURApRaAKCAKsf8b2EQWGDJ3TSFT4dDY5cNsACdFzlI
> yvfRi5A+xgLK8bJ7ea7oVZA> =8vm8
> -----END PGP SIGNATURE-----
>
>

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adt7470: Update sensors periodically via

[Darrick J. Wong]

[-- Attachment #1.1: Type: text/plain, Size: 907 bytes --]

On Thu, Jul 26, 2007 at 08:08:09AM -0700, Juerg Haefliger wrote:
> Hi Darrick,
> 
> I guess I'm not seeing the big picture :-) The problem you're trying
> to solve is that the reading of the registers takes a long time and
> needs to be able to sleep?

Yes, and also that sleeping in the sysfs read function is not desirable.
Personally, I was fine with that, but Mr. de Goede was concerned about
it, so I decided to try to fix it.  I admit, the dual-delayed-workqueue
method is a bit strange, but it solves the reading problem at a slight
cost of waking up the CPU even if nobody's watching the sensors.

> Hmm... Sounds rather complicated. Is there a problem with blocking
> apps for that long?
> How about forking off a kernel thread instead of using workqueues?

I was trying to use as little resources as possible, and a kernel thread
seemed like overkill for a hwmon device.

--D

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adt7470: Update sensors periodically via

[Hans de Goede]

Darrick J. Wong wrote:
> On Thu, Jul 26, 2007 at 08:08:09AM -0700, Juerg Haefliger wrote:
>> Hi Darrick,
>>
>> I guess I'm not seeing the big picture :-) The problem you're trying
>> to solve is that the reading of the registers takes a long time and
>> needs to be able to sleep?
> 
> Yes, and also that sleeping in the sysfs read function is not desirable.
> Personally, I was fine with that, but Mr. de Goede was concerned about
> it, so I decided to try to fix it.  I admit, the dual-delayed-workqueue
> method is a bit strange, but it solves the reading problem at a slight
> cost of waking up the CPU even if nobody's watching the sensors.
> 

I just read in another thread, that other hwmon drivers suffer from similar 
problems, so I guess userspace will just have to be able to deal with this, and 
the best fix is not to fix it. Sorry to waist everybody's time on this.

Regards,

Hans


p.s.

You could still try to shave some time off by not reading all the limits etc 
each update.


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] adt7470: Update sensors periodically via

[Philip Pokorny]

[-- Attachment #1: Type: text/plain, Size: 215 bytes --]

--===============8985833807290501701==
Content-class: urn:content-classes:message
Content-Type: multipart/alternative;
	boundary="----_=_NextPart_001_01C7CFC0.167E2253"

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 1905 bytes --]

The trick of not reading the limits was something I did in the LM85 driver...

The read function has two time stamps.  One to trigger reading of the dynamic values if they haven't been read recently (1-2 second) and another that triggers reading the limits but no more often than 10 to 60 seconds.

:v)

-- 
Philip Pokorny, RHCE
Director of Field Engineering 
Penguin Computing  http://www.penguincomputing.com

 -----Original Message-----
From: 	Hans de Goede [mailto:j.w.r.degoede@hhs.nl]
Sent:	Thursday, July 26, 2007 10:01 AM Pacific Standard Time
To:	Darrick J. Wong
Cc:	Vadim Zeitlin; LM Sensors
Subject:	Re: [lm-sensors] [PATCH] adt7470: Update sensors periodically via timer to avoid blocking

Darrick J. Wong wrote:
> On Thu, Jul 26, 2007 at 08:08:09AM -0700, Juerg Haefliger wrote:
>> Hi Darrick,
>>
>> I guess I'm not seeing the big picture :-) The problem you're trying
>> to solve is that the reading of the registers takes a long time and
>> needs to be able to sleep?
> 
> Yes, and also that sleeping in the sysfs read function is not desirable.
> Personally, I was fine with that, but Mr. de Goede was concerned about
> it, so I decided to try to fix it.  I admit, the dual-delayed-workqueue
> method is a bit strange, but it solves the reading problem at a slight
> cost of waking up the CPU even if nobody's watching the sensors.
> 

I just read in another thread, that other hwmon drivers suffer from similar 
problems, so I guess userspace will just have to be able to deal with this, and 
the best fix is not to fix it. Sorry to waist everybody's time on this.

Regards,

Hans


p.s.

You could still try to shave some time off by not reading all the limits etc 
each update.


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[-- Attachment #3: Type: text/html, Size: 2730 bytes --]

Re: [lm-sensors] [PATCH] adt7470: Update sensors periodically via

[Mark M. Hoffman]

Hi Hans, Darrick:

* Hans de Goede <j.w.r.degoede@hhs.nl> [2007-07-26 18:58:18 +0200]:
> Darrick J. Wong wrote:
> > On Thu, Jul 26, 2007 at 08:08:09AM -0700, Juerg Haefliger wrote:
> >> Hi Darrick,
> >>
> >> I guess I'm not seeing the big picture :-) The problem you're trying to
> >> solve is that the reading of the registers takes a long time and needs to
> >> be able to sleep?
> > 
> > Yes, and also that sleeping in the sysfs read function is not desirable.
> > Personally, I was fine with that, but Mr. de Goede was concerned about it,
> > so I decided to try to fix it.  I admit, the dual-delayed-workqueue method
> > is a bit strange, but it solves the reading problem at a slight cost of
> > waking up the CPU even if nobody's watching the sensors.
> > 
> 
> I just read in another thread, that other hwmon drivers suffer from similar
> problems, so I guess userspace will just have to be able to deal with this,
> and the best fix is not to fix it. Sorry to waist everybody's time on this.

Yes, and in this case the cure is worse than the disease.  As I mentioned in
another thread recently, most I2C bus drivers are very slow and polled, using
%100 cpu while doing transfers.

To put that kind of load on a system periodically just to keep the app from
blocking would be IMHO a terrible tradeoff.

The root cause of the speed problem is in the I2C bus drivers, not the hwmon
drivers.

> You could still try to shave some time off by not reading all the limits etc 
> each update.

Notwithstanding what I wrote above, this is still a good idea.  But don't go
too far with it... we do still want to read/update the limits occasionally.
Sometimes that's the only way a user can find out that the BIOS or ACPI or
whatever is modifying chip values behind our backs.

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors