[PATCH 1/2] net: dsa: use DEVICE_ATTR_RW to declare temp1_max

[PATCH 1/2] net: dsa: use DEVICE_ATTR_RW to declare temp1_max

[Vivien Didelot]

Since commit da4759c, sysfs will only use the permissions returned by
is_visible, instead of OR'ing them with the default file mode.

This allows us to declare temp1_max with the DEVICE_ATTR_RW macro and
just return the desired permissions for the hwmon sysfs attributes in
dsa_hwmon_attrs_visible.

Also, allow temp1_max to be write-only if set_temp_limit is provided,
but not get_temp_limit.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/dsa.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 5eaadab..67d2983 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -124,7 +124,7 @@ static ssize_t temp1_max_store(struct device *dev,
 
 	return count;
 }
-static DEVICE_ATTR(temp1_max, S_IRUGO, temp1_max_show, temp1_max_store);
+static DEVICE_ATTR_RW(temp1_max);
 
 static ssize_t temp1_max_alarm_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
@@ -154,16 +154,24 @@ static umode_t dsa_hwmon_attrs_visible(struct kobject *kobj,
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct dsa_switch *ds = dev_get_drvdata(dev);
 	struct dsa_switch_driver *drv = ds->drv;
-	umode_t mode = attr->mode;
+	umode_t mode = 0;
 
-	if (index == 1) {
-		if (!drv->get_temp_limit)
-			mode = 0;
-		else if (drv->set_temp_limit)
+	switch (index) {
+	case 1: /* temp1_max */
+		if (drv->get_temp_limit)
+			mode |= S_IRUGO;
+		if (drv->set_temp_limit)
 			mode |= S_IWUSR;
-	} else if (index == 2 && !drv->get_temp_alarm) {
-		mode = 0;
+		break;
+	case 2: /* temp1_max_alarm */
+		if (drv->get_temp_alarm)
+			mode |= S_IRUGO;
+		break;
+	default:
+		mode = attr->mode;
+		break;
 	}
+
 	return mode;
 }
 
-- 
2.3.5

[PATCH 2/2] net: dsa: register hwmon for any provided function

[Vivien Didelot]

A switch driver may only provide one of the temperature limit accessors,
or the temperature alarm getter. So register the hwmon subsystem if any
of the related functions is provided.

Thus, check get_temp to set the visibility of temp1_input.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/dsa.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 67d2983..6b68994 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -157,6 +157,10 @@ static umode_t dsa_hwmon_attrs_visible(struct kobject *kobj,
 	umode_t mode = 0;
 
 	switch (index) {
+	case 0: /* temp1_input */
+		if (drv->get_temp)
+			mode |= S_IRUGO;
+		break;
 	case 1: /* temp1_max */
 		if (drv->get_temp_limit)
 			mode |= S_IRUGO;
@@ -310,7 +314,8 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 	 * register with hardware monitoring subsystem.
 	 * Treat registration error as non-fatal and ignore it.
 	 */
-	if (drv->get_temp) {
+	if (drv->get_temp || drv->get_temp_limit || drv->set_temp_limit ||
+	    drv->get_temp_alarm) {
 		const char *netname = netdev_name(dst->master_netdev);
 		char hname[IFNAMSIZ + 1];
 		int i, j;
-- 
2.3.5

Re: [PATCH 2/2] net: dsa: register hwmon for any provided function

[Guenter Roeck]

On Thu, Apr 16, 2015 at 02:38:19PM -0400, Vivien Didelot wrote:
> A switch driver may only provide one of the temperature limit accessors,
> or the temperature alarm getter. So register the hwmon subsystem if any
> of the related functions is provided.
> 
> Thus, check get_temp to set the visibility of temp1_input.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  net/dsa/dsa.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 67d2983..6b68994 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -157,6 +157,10 @@ static umode_t dsa_hwmon_attrs_visible(struct kobject *kobj,
>  	umode_t mode = 0;
>  
>  	switch (index) {
> +	case 0: /* temp1_input */
> +		if (drv->get_temp)
> +			mode |= S_IRUGO;

This should be mandatory. Sorry, I don't really understand what you are
trying to accomplish here.

Can you give me a real world example where a chip would support setting
a limit but not reading it ?

Thanks,
Guenter

> +		break;
>  	case 1: /* temp1_max */
>  		if (drv->get_temp_limit)
>  			mode |= S_IRUGO;
> @@ -310,7 +314,8 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
>  	 * register with hardware monitoring subsystem.
>  	 * Treat registration error as non-fatal and ignore it.
>  	 */
> -	if (drv->get_temp) {
> +	if (drv->get_temp || drv->get_temp_limit || drv->set_temp_limit ||
> +	    drv->get_temp_alarm) {
>  		const char *netname = netdev_name(dst->master_netdev);
>  		char hname[IFNAMSIZ + 1];
>  		int i, j;
> -- 
> 2.3.5
> 

Re: [PATCH 2/2] net: dsa: register hwmon for any provided function

[Vivien Didelot]

Hi Guenter,

> >          switch (index) {
> > +        case 0: /* temp1_input */
> > +                if (drv->get_temp)
> > +                        mode |= S_IRUGO;
> 
> This should be mandatory. Sorry, I don't really understand what you are
> trying to accomplish here.
> 
> Can you give me a real world example where a chip would support setting
> a limit but not reading it ?

I have no such example. I just did not see why this couldn't be allowed
(e.g. setting only set_temp_limit and get_temp_alarm looks fine to me).
But if you say that get_temp should be mandatory, I'm OK with that.

The primary goal of this patchset was to use DEVICE_ATTR_RW to declare
temp1_max, instead of reflecting the minimal permissions needed.

Best,
-v

Re: [PATCH 2/2] net: dsa: register hwmon for any provided function

[Guenter Roeck]

hi Vivien,

On 04/16/2015 03:05 PM, Vivien Didelot wrote:
> Hi Guenter,
>
>>>           switch (index) {
>>> +        case 0: /* temp1_input */
>>> +                if (drv->get_temp)
>>> +                        mode |= S_IRUGO;
>>
>> This should be mandatory. Sorry, I don't really understand what you are
>> trying to accomplish here.
>>
>> Can you give me a real world example where a chip would support setting
>> a limit but not reading it ?
>
> I have no such example. I just did not see why this couldn't be allowed
> (e.g. setting only set_temp_limit and get_temp_alarm looks fine to me).
> But if you say that get_temp should be mandatory, I'm OK with that.
>
write-only attributes are not defined in the hwmon ABI. If the 'sensors'
command encounters such an attribute, it will create an error message
each time it executes. That doesn't sound very useful to me.

If a chip - for whatever reason - does not have a limit register
but an alarm register or flag, its temperature limit is usually hard-coded
and can be reported this way (the AMD temperature sensor driver does this,
for example). If there is ever a need to support the alarm-register-only
situation for some odd reason, we can add the code at the time.
For now, it just seems to me that you are adding complexity to solve
some theoretic problem which is very unlikely to occur in the real world.

> The primary goal of this patchset was to use DEVICE_ATTR_RW to declare
> temp1_max, instead of reflecting the minimal permissions needed.
>
Then why don't you just do that and nothing else ? The goal should be
to simplify code, not to make it more complicated. If the result isn't
less code, I don't think it is worth it.

Thanks,
Guenter

Re: [PATCH 2/2] net: dsa: register hwmon for any provided function

[Vivien Didelot]

Hi Guenter,

> >>>           switch (index) {
> >>> +        case 0: /* temp1_input */
> >>> +                if (drv->get_temp)
> >>> +                        mode |= S_IRUGO;
> >>
> >> This should be mandatory. Sorry, I don't really understand what you
> >> are trying to accomplish here.
> >>
> >> Can you give me a real world example where a chip would support
> >> setting a limit but not reading it ?
> >
> > I have no such example. I just did not see why this couldn't be
> > allowed (e.g. setting only set_temp_limit and get_temp_alarm looks
> > fine to me).  But if you say that get_temp should be mandatory, I'm
> > OK with that.
> >
> write-only attributes are not defined in the hwmon ABI. If the
> 'sensors' command encounters such an attribute, it will create an
> error message each time it executes. That doesn't sound very useful to
> me.

Ok, good to know.

> If a chip - for whatever reason - does not have a limit register but
> an alarm register or flag, its temperature limit is usually hard-coded
> and can be reported this way (the AMD temperature sensor driver does
> this, for example). If there is ever a need to support the
> alarm-register-only situation for some odd reason, we can add the code
> at the time.  For now, it just seems to me that you are adding
> complexity to solve some theoretic problem which is very unlikely to
> occur in the real world.

You're right, this change is not necessary.

> > The primary goal of this patchset was to use DEVICE_ATTR_RW to
> > declare temp1_max, instead of reflecting the minimal permissions
> > needed.
> >
> Then why don't you just do that and nothing else ? The goal should be
> to simplify code, not to make it more complicated. If the result isn't
> less code, I don't think it is worth it.

Indeed, I'll rework this patch then.

Thanks for the review,
-v

Re: [PATCH 1/2] net: dsa: use DEVICE_ATTR_RW to declare temp1_max

[Sergei Shtylyov]

Hello.

On 04/16/2015 09:38 PM, Vivien Didelot wrote:

> Since commit da4759c, sysfs will only use the permissions returned by

    Please also specify that commit's summary line in parens.

> is_visible, instead of OR'ing them with the default file mode.

> This allows us to declare temp1_max with the DEVICE_ATTR_RW macro and
> just return the desired permissions for the hwmon sysfs attributes in
> dsa_hwmon_attrs_visible.

> Also, allow temp1_max to be write-only if set_temp_limit is provided,
> but not get_temp_limit.

> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

WBR, Sergei

Re: [PATCH 1/2] net: dsa: use DEVICE_ATTR_RW to declare temp1_max

[Vivien Didelot]

Hello Sergei,

> > Since commit da4759c, sysfs will only use the permissions returned by
> 
>     Please also specify that commit's summary line in parens.

Duly noted.

da4759c is "sysfs: Use only return value from is_visible for the file mode"
(see: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=da4759c)

Best,
-v

Re: [PATCH 1/2] net: dsa: use DEVICE_ATTR_RW to declare temp1_max

[Guenter Roeck]

On Thu, Apr 16, 2015 at 02:38:18PM -0400, Vivien Didelot wrote:
> Since commit da4759c, sysfs will only use the permissions returned by
> is_visible, instead of OR'ing them with the default file mode.
> 
> This allows us to declare temp1_max with the DEVICE_ATTR_RW macro and
> just return the desired permissions for the hwmon sysfs attributes in
> dsa_hwmon_attrs_visible.
> 
> Also, allow temp1_max to be write-only if set_temp_limit is provided,
> but not get_temp_limit.
> 
Hi Vivien,

This would be a first for the entire hwmon subsystem and doesn't really
make sense.

Guenter

> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  net/dsa/dsa.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 5eaadab..67d2983 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -124,7 +124,7 @@ static ssize_t temp1_max_store(struct device *dev,
>  
>  	return count;
>  }
> -static DEVICE_ATTR(temp1_max, S_IRUGO, temp1_max_show, temp1_max_store);
> +static DEVICE_ATTR_RW(temp1_max);
>  
>  static ssize_t temp1_max_alarm_show(struct device *dev,
>  				    struct device_attribute *attr, char *buf)
> @@ -154,16 +154,24 @@ static umode_t dsa_hwmon_attrs_visible(struct kobject *kobj,
>  	struct device *dev = container_of(kobj, struct device, kobj);
>  	struct dsa_switch *ds = dev_get_drvdata(dev);
>  	struct dsa_switch_driver *drv = ds->drv;
> -	umode_t mode = attr->mode;
> +	umode_t mode = 0;
>  
> -	if (index == 1) {
> -		if (!drv->get_temp_limit)
> -			mode = 0;
> -		else if (drv->set_temp_limit)
> +	switch (index) {
> +	case 1: /* temp1_max */
> +		if (drv->get_temp_limit)
> +			mode |= S_IRUGO;
> +		if (drv->set_temp_limit)
>  			mode |= S_IWUSR;
> -	} else if (index == 2 && !drv->get_temp_alarm) {
> -		mode = 0;
> +		break;
> +	case 2: /* temp1_max_alarm */
> +		if (drv->get_temp_alarm)
> +			mode |= S_IRUGO;
> +		break;
> +	default:
> +		mode = attr->mode;
> +		break;
>  	}
> +
>  	return mode;
>  }
>  
> -- 
> 2.3.5
>