Date:	Wed, 16 Feb 2011 06:50:49 -0800
From:	Guenter Roeck <guenter.roeck@...csson.com>
To:	Clemens Ladisch <clemens@...isch.de>
CC:	Guenter Roeck <linux@...ck-us.net>,
	Jean Delvare <khali@...ux-fr.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>
Subject: Re: [lm-sensors] [PATCH 3/3] hwmon: (jc42) do not allow writing to
 locked registers

Hi Clemens,

On Wed, Feb 16, 2011 at 08:02:38AM -0500, Clemens Ladisch wrote:
> On systems where the temperature sensor is actually used, the BIOS is
> likely to have locked the alarm registers.  In that case, all writes
> through the corresponding sysfs files would be silently ignored.
> 
> To prevent this, detect the locks and make the affected sysfs files
> read-only.
> 
> Signed-off-by: Clemens Ladisch <clemens@...isch.de>
> ---
>  Documentation/hwmon/jc42 |   12 ++++++++----
>  drivers/hwmon/jc42.c     |   33 +++++++++++++++++++++++++++++----
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> --- a/Documentation/hwmon/jc42
> +++ b/Documentation/hwmon/jc42
> @@ -86,15 +86,19 @@ limits. The chip supports only a single
>  which applies to all limits. This register can be written by writing into
>  temp1_crit_hyst. Other hysteresis attributes are read-only.
>  
> +If the BIOS has configured the sensor for automatic temperature management, it
> +is likely that it has locked the registers, i.e., that the temperature limits
> +cannot be changed.
> +
>  Sysfs entries
>  -------------
>  
>  temp1_input		Temperature (RO)
> -temp1_min		Minimum temperature (RW)
> -temp1_max		Maximum temperature (RW)
> -temp1_crit		Critical high temperature (RW)
> +temp1_min		Minimum temperature (RO or RW)
> +temp1_max		Maximum temperature (RO or RW)
> +temp1_crit		Critical high temperature (RO or RW)
>  
> -temp1_crit_hyst		Critical hysteresis temperature (RW)
> +temp1_crit_hyst		Critical hysteresis temperature (RO or RW)
>  temp1_max_hyst		Maximum hysteresis temperature (RO)
>  
>  temp1_min_alarm		Temperature low alarm
> --- a/drivers/hwmon/jc42.c
> +++ b/drivers/hwmon/jc42.c
> @@ -53,6 +53,8 @@ static const unsigned short normal_i2c[]
>  
>  /* Configuration register defines */
>  #define JC42_CFG_CRIT_ONLY	(1 << 2)
> +#define JC42_CFG_TCRIT_LOCK	(1 << 6)
> +#define JC42_CFG_EVENT_LOCK	(1 << 7)
>  #define JC42_CFG_SHUTDOWN	(1 << 8)
>  #define JC42_CFG_HYST_SHIFT	9
>  #define JC42_CFG_HYST_MASK	0x03
> @@ -380,14 +382,14 @@ static ssize_t show_alarm(struct device
>  
>  static DEVICE_ATTR(temp1_input, S_IRUGO,
>  		   show_temp_input, NULL);
> -static DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO,
> +static DEVICE_ATTR(temp1_crit, S_IRUGO,
>  		   show_temp_crit, set_temp_crit);
> -static DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO,
> +static DEVICE_ATTR(temp1_min, S_IRUGO,
>  		   show_temp_min, set_temp_min);
> -static DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO,
> +static DEVICE_ATTR(temp1_max, S_IRUGO,
>  		   show_temp_max, set_temp_max);
>  
> -static DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO,
> +static DEVICE_ATTR(temp1_crit_hyst, S_IRUGO,
>  		   show_temp_crit_hyst, set_temp_crit_hyst);
>  static DEVICE_ATTR(temp1_max_hyst, S_IRUGO,
>  		   show_temp_max_hyst, NULL);
> @@ -412,8 +414,31 @@ static struct attribute *jc42_attributes
>  	NULL
>  };
>  
> +static mode_t jc42_attribute_mode(struct kobject *kobj,
> +				  struct attribute *attr, int index)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct jc42_data *data = i2c_get_clientdata(client);
> +	unsigned int config = data->config;
> +	bool readonly;
> +
> +	if (attr == &dev_attr_temp1_crit.attr)
> +		readonly = config & JC42_CFG_TCRIT_LOCK;

You are assigning a non-bool to a bool. I can see that recent C compilers
do the right thing, but I am not sure if that is always the case.
So I would prefer
		readonly = !!(config & JC42_CFG_TCRIT_LOCK));

Same for the assignments below. I can make that change if you are ok with it.

> +	else if (attr == &dev_attr_temp1_min.attr ||
> +		 attr == &dev_attr_temp1_max.attr)
> +		readonly = config & JC42_CFG_EVENT_LOCK;
> +	else if (attr == &dev_attr_temp1_crit_hyst.attr)
> +		readonly = config & (JC42_CFG_EVENT_LOCK | JC42_CFG_TCRIT_LOCK);
> +	else
> +		readonly = true;
> +
> +	return S_IRUGO | (readonly ? 0 : S_IWUSR);
> +}
> +
>  static const struct attribute_group jc42_group = {
>  	.attrs = jc42_attributes,
> +	.is_visible = jc42_attribute_mode,
>  };
>  
>  /* Return 0 if detection is successful, -ENODEV otherwise */
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@...sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists