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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55306D40.1060604@roeck-us.net>
Date:	Thu, 16 Apr 2015 19:17:36 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>
CC:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Florian Fainelli <f.fainelli@...il.com>,
	kernel@...oirfairelinux.com
Subject: Re: [PATCH 2/2] net: dsa: register hwmon for any provided function

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html