* [PATCH] hwmon: (dell-smm-hwmon) Fix index values
@ 2021-05-13 15:45 W_Armin
2021-05-13 16:48 ` Pali Rohár
2021-05-13 16:54 ` Guenter Roeck
0 siblings, 2 replies; 7+ messages in thread
From: W_Armin @ 2021-05-13 15:45 UTC (permalink / raw)
To: pali; +Cc: linux-hwmon, linux, jdelvare
From: Armin Wolf <W_Armin@gmx.de>
When support for up to 10 temp sensors and for disabling automatic BIOS
fan control was added, noone updated the index values used for
disallowing fan support and fan type calls.
Fix those values.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
Tested on a Dell Latitude C600.
---
drivers/hwmon/dell-smm-hwmon.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 2970892bed82..f2221ca0aa7b 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -838,10 +838,10 @@ static struct attribute *i8k_attrs[] = {
static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
int index)
{
- if (disallow_fan_support && index >= 8)
+ if (disallow_fan_support && index >= 20)
return 0;
if (disallow_fan_type_call &&
- (index == 9 || index == 12 || index == 15))
+ (index == 21 || index == 25 || index == 28))
return 0;
if (index >= 0 && index <= 1 &&
!(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] hwmon: (dell-smm-hwmon) Fix index values
2021-05-13 15:45 [PATCH] hwmon: (dell-smm-hwmon) Fix index values W_Armin
@ 2021-05-13 16:48 ` Pali Rohár
2021-05-13 16:53 ` Guenter Roeck
2021-05-13 16:54 ` Guenter Roeck
1 sibling, 1 reply; 7+ messages in thread
From: Pali Rohár @ 2021-05-13 16:48 UTC (permalink / raw)
To: W_Armin; +Cc: linux-hwmon, linux, jdelvare
On Thursday 13 May 2021 17:45:46 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
>
> When support for up to 10 temp sensors and for disabling automatic BIOS
> fan control was added, noone updated the index values used for
> disallowing fan support and fan type calls.
> Fix those values.
Do you mean this change, right?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bb46a20e73b0bb3364cff3839c9f716ed327770
Yes, it looks like that it should have been part of that change.
Therefore I suggest to add Fixes tag:
Fixes: 1bb46a20e73b ("hwmon: (dell-smm) Support up to 10 temp sensors")
Otherwise looks good!
Reviewed-by: Pali Rohár <pali@kernel.org>
For future development I would suggest to rewrite/drop these magic
numbers as same problem can be easily repeated in future.
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> Tested on a Dell Latitude C600.
> ---
> drivers/hwmon/dell-smm-hwmon.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 2970892bed82..f2221ca0aa7b 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -838,10 +838,10 @@ static struct attribute *i8k_attrs[] = {
> static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
> int index)
> {
> - if (disallow_fan_support && index >= 8)
> + if (disallow_fan_support && index >= 20)
> return 0;
> if (disallow_fan_type_call &&
> - (index == 9 || index == 12 || index == 15))
> + (index == 21 || index == 25 || index == 28))
> return 0;
> if (index >= 0 && index <= 1 &&
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] hwmon: (dell-smm-hwmon) Fix index values
2021-05-13 16:48 ` Pali Rohár
@ 2021-05-13 16:53 ` Guenter Roeck
2021-05-13 18:41 ` Armin Wolf
0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2021-05-13 16:53 UTC (permalink / raw)
To: Pali Rohár, W_Armin; +Cc: linux-hwmon, jdelvare
On 5/13/21 9:48 AM, Pali Rohár wrote:
> On Thursday 13 May 2021 17:45:46 W_Armin@gmx.de wrote:
>> From: Armin Wolf <W_Armin@gmx.de>
>>
>> When support for up to 10 temp sensors and for disabling automatic BIOS
>> fan control was added, noone updated the index values used for
>> disallowing fan support and fan type calls.
>> Fix those values.
>
> Do you mean this change, right?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bb46a20e73b0bb3364cff3839c9f716ed327770
>
> Yes, it looks like that it should have been part of that change.
>
> Therefore I suggest to add Fixes tag:
>
> Fixes: 1bb46a20e73b ("hwmon: (dell-smm) Support up to 10 temp sensors")
>
> Otherwise looks good!
>
> Reviewed-by: Pali Rohár <pali@kernel.org>
>
> For future development I would suggest to rewrite/drop these magic
> numbers as same problem can be easily repeated in future.
>
The best solution would be to rewrite the driver to use
hwmon_device_register_with_info(), but that should be done
by someone with access to the hardware.
Guenter
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> Tested on a Dell Latitude C600.
>> ---
>> drivers/hwmon/dell-smm-hwmon.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 2970892bed82..f2221ca0aa7b 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -838,10 +838,10 @@ static struct attribute *i8k_attrs[] = {
>> static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
>> int index)
>> {
>> - if (disallow_fan_support && index >= 8)
>> + if (disallow_fan_support && index >= 20)
>> return 0;
>> if (disallow_fan_type_call &&
>> - (index == 9 || index == 12 || index == 15))
>> + (index == 21 || index == 25 || index == 28))
>> return 0;
>> if (index >= 0 && index <= 1 &&
>> !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
>> --
>> 2.20.1
>>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] hwmon: (dell-smm-hwmon) Fix index values
2021-05-13 16:53 ` Guenter Roeck
@ 2021-05-13 18:41 ` Armin Wolf
2021-05-13 18:48 ` Guenter Roeck
2021-05-13 18:54 ` Pali Rohár
0 siblings, 2 replies; 7+ messages in thread
From: Armin Wolf @ 2021-05-13 18:41 UTC (permalink / raw)
To: Guenter Roeck, Pali Rohár; +Cc: linux-hwmon, jdelvare
On 13.05.21 18:53 Guenter Roeck wrote:
> On 5/13/21 9:48 AM, Pali Rohár wrote:
>> On Thursday 13 May 2021 17:45:46 W_Armin@gmx.de wrote:
>>> From: Armin Wolf <W_Armin@gmx.de>
>>>
>>> When support for up to 10 temp sensors and for disabling automatic BIOS
>>> fan control was added, noone updated the index values used for
>>> disallowing fan support and fan type calls.
>>> Fix those values.
>>
>> Do you mean this change, right?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bb46a20e73b0bb3364cff3839c9f716ed327770
>>
>>
>> Yes, it looks like that it should have been part of that change.
>>
>> Therefore I suggest to add Fixes tag:
>>
>> Fixes: 1bb46a20e73b ("hwmon: (dell-smm) Support up to 10 temp sensors")
>>
>> Otherwise looks good!
>>
>> Reviewed-by: Pali Rohár <pali@kernel.org>
>>
>> For future development I would suggest to rewrite/drop these magic
>> numbers as same problem can be easily repeated in future.
>>
>
> The best solution would be to rewrite the driver to use
> hwmon_device_register_with_info(), but that should be done
> by someone with access to the hardware.
>
> Guenter
Im currently doing exactly that, since i have an old dell notebook. But
that might take some time.
>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>> Tested on a Dell Latitude C600.
>>> ---
>>> drivers/hwmon/dell-smm-hwmon.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c
>>> b/drivers/hwmon/dell-smm-hwmon.c
>>> index 2970892bed82..f2221ca0aa7b 100644
>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>> @@ -838,10 +838,10 @@ static struct attribute *i8k_attrs[] = {
>>> static umode_t i8k_is_visible(struct kobject *kobj, struct
>>> attribute *attr,
>>> int index)
>>> {
>>> - if (disallow_fan_support && index >= 8)
>>> + if (disallow_fan_support && index >= 20)
>>> return 0;
>>> if (disallow_fan_type_call &&
>>> - (index == 9 || index == 12 || index == 15))
>>> + (index == 21 || index == 25 || index == 28))
>>> return 0;
>>> if (index >= 0 && index <= 1 &&
>>> !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
>>> --
>>> 2.20.1
>>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] hwmon: (dell-smm-hwmon) Fix index values
2021-05-13 18:41 ` Armin Wolf
@ 2021-05-13 18:48 ` Guenter Roeck
2021-05-13 18:54 ` Pali Rohár
1 sibling, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2021-05-13 18:48 UTC (permalink / raw)
To: Armin Wolf, Pali Rohár; +Cc: linux-hwmon, jdelvare
On 5/13/21 11:41 AM, Armin Wolf wrote:
> On 13.05.21 18:53 Guenter Roeck wrote:
>> On 5/13/21 9:48 AM, Pali Rohár wrote:
>>> On Thursday 13 May 2021 17:45:46 W_Armin@gmx.de wrote:
>>>> From: Armin Wolf <W_Armin@gmx.de>
>>>>
>>>> When support for up to 10 temp sensors and for disabling automatic BIOS
>>>> fan control was added, noone updated the index values used for
>>>> disallowing fan support and fan type calls.
>>>> Fix those values.
>>>
>>> Do you mean this change, right?
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bb46a20e73b0bb3364cff3839c9f716ed327770
>>>
>>>
>>> Yes, it looks like that it should have been part of that change.
>>>
>>> Therefore I suggest to add Fixes tag:
>>>
>>> Fixes: 1bb46a20e73b ("hwmon: (dell-smm) Support up to 10 temp sensors")
>>>
>>> Otherwise looks good!
>>>
>>> Reviewed-by: Pali Rohár <pali@kernel.org>
>>>
>>> For future development I would suggest to rewrite/drop these magic
>>> numbers as same problem can be easily repeated in future.
>>>
>>
>> The best solution would be to rewrite the driver to use
>> hwmon_device_register_with_info(), but that should be done
>> by someone with access to the hardware.
>>
>> Guenter
> Im currently doing exactly that, since i have an old dell notebook. But
> that might take some time.
Excellent, thanks!
Guenter
>>
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>> ---
>>>> Tested on a Dell Latitude C600.
>>>> ---
>>>> drivers/hwmon/dell-smm-hwmon.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c
>>>> b/drivers/hwmon/dell-smm-hwmon.c
>>>> index 2970892bed82..f2221ca0aa7b 100644
>>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>>> @@ -838,10 +838,10 @@ static struct attribute *i8k_attrs[] = {
>>>> static umode_t i8k_is_visible(struct kobject *kobj, struct
>>>> attribute *attr,
>>>> int index)
>>>> {
>>>> - if (disallow_fan_support && index >= 8)
>>>> + if (disallow_fan_support && index >= 20)
>>>> return 0;
>>>> if (disallow_fan_type_call &&
>>>> - (index == 9 || index == 12 || index == 15))
>>>> + (index == 21 || index == 25 || index == 28))
>>>> return 0;
>>>> if (index >= 0 && index <= 1 &&
>>>> !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
>>>> --
>>>> 2.20.1
>>>>
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] hwmon: (dell-smm-hwmon) Fix index values
2021-05-13 18:41 ` Armin Wolf
2021-05-13 18:48 ` Guenter Roeck
@ 2021-05-13 18:54 ` Pali Rohár
1 sibling, 0 replies; 7+ messages in thread
From: Pali Rohár @ 2021-05-13 18:54 UTC (permalink / raw)
To: Armin Wolf; +Cc: Guenter Roeck, linux-hwmon, jdelvare
On Thursday 13 May 2021 20:41:06 Armin Wolf wrote:
> On 13.05.21 18:53 Guenter Roeck wrote:
> > On 5/13/21 9:48 AM, Pali Rohár wrote:
> > > On Thursday 13 May 2021 17:45:46 W_Armin@gmx.de wrote:
> > > > From: Armin Wolf <W_Armin@gmx.de>
> > > >
> > > > When support for up to 10 temp sensors and for disabling automatic BIOS
> > > > fan control was added, noone updated the index values used for
> > > > disallowing fan support and fan type calls.
> > > > Fix those values.
> > >
> > > Do you mean this change, right?
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bb46a20e73b0bb3364cff3839c9f716ed327770
> > >
> > >
> > > Yes, it looks like that it should have been part of that change.
> > >
> > > Therefore I suggest to add Fixes tag:
> > >
> > > Fixes: 1bb46a20e73b ("hwmon: (dell-smm) Support up to 10 temp sensors")
> > >
> > > Otherwise looks good!
> > >
> > > Reviewed-by: Pali Rohár <pali@kernel.org>
> > >
> > > For future development I would suggest to rewrite/drop these magic
> > > numbers as same problem can be easily repeated in future.
> > >
> >
> > The best solution would be to rewrite the driver to use
> > hwmon_device_register_with_info(), but that should be done
> > by someone with access to the hardware.
> >
> > Guenter
> Im currently doing exactly that, since i have an old dell notebook. But
> that might take some time.
Perfect! Thank you for looking at this.
I think that this driver converged into state when it is used by more
non-developer users on LTS -stable kernels as developers which use play
with -rc kernels. So issues or breakages are detected sometimes after
longer time.
Also I think that driver is basically feature-complete so I do not have
reason for investing time in its development.
Currently I have only one Dell laptop and I'm not testing on it kernel
patches as it is used for different daily work and also uses 4.19 LTS
distribution kernel.
But when you send a patch which is larger and needs more testing, I have
no problem to try compile it as module for current distribution kernel
and exchange .ko module at runtime. And test if functionality works as
expected.
> >
> > > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > > ---
> > > > Tested on a Dell Latitude C600.
> > > > ---
> > > > drivers/hwmon/dell-smm-hwmon.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/hwmon/dell-smm-hwmon.c
> > > > b/drivers/hwmon/dell-smm-hwmon.c
> > > > index 2970892bed82..f2221ca0aa7b 100644
> > > > --- a/drivers/hwmon/dell-smm-hwmon.c
> > > > +++ b/drivers/hwmon/dell-smm-hwmon.c
> > > > @@ -838,10 +838,10 @@ static struct attribute *i8k_attrs[] = {
> > > > static umode_t i8k_is_visible(struct kobject *kobj, struct
> > > > attribute *attr,
> > > > int index)
> > > > {
> > > > - if (disallow_fan_support && index >= 8)
> > > > + if (disallow_fan_support && index >= 20)
> > > > return 0;
> > > > if (disallow_fan_type_call &&
> > > > - (index == 9 || index == 12 || index == 15))
> > > > + (index == 21 || index == 25 || index == 28))
> > > > return 0;
> > > > if (index >= 0 && index <= 1 &&
> > > > !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
> > > > --
> > > > 2.20.1
> > > >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] hwmon: (dell-smm-hwmon) Fix index values
2021-05-13 15:45 [PATCH] hwmon: (dell-smm-hwmon) Fix index values W_Armin
2021-05-13 16:48 ` Pali Rohár
@ 2021-05-13 16:54 ` Guenter Roeck
1 sibling, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2021-05-13 16:54 UTC (permalink / raw)
To: W_Armin; +Cc: pali, linux-hwmon, jdelvare
On Thu, May 13, 2021 at 05:45:46PM +0200, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
>
> When support for up to 10 temp sensors and for disabling automatic BIOS
> fan control was added, noone updated the index values used for
> disallowing fan support and fan type calls.
> Fix those values.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> Reviewed-by: Pali Rohár <pali@kernel.org>
Applied, with added Fixes: tag.
Guenter
> ---
> Tested on a Dell Latitude C600.
> ---
> drivers/hwmon/dell-smm-hwmon.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> --
> 2.20.1
>
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 2970892bed82..f2221ca0aa7b 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -838,10 +838,10 @@ static struct attribute *i8k_attrs[] = {
> static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
> int index)
> {
> - if (disallow_fan_support && index >= 8)
> + if (disallow_fan_support && index >= 20)
> return 0;
> if (disallow_fan_type_call &&
> - (index == 9 || index == 12 || index == 15))
> + (index == 21 || index == 25 || index == 28))
> return 0;
> if (index >= 0 && index <= 1 &&
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-13 18:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-13 15:45 [PATCH] hwmon: (dell-smm-hwmon) Fix index values W_Armin
2021-05-13 16:48 ` Pali Rohár
2021-05-13 16:53 ` Guenter Roeck
2021-05-13 18:41 ` Armin Wolf
2021-05-13 18:48 ` Guenter Roeck
2021-05-13 18:54 ` Pali Rohár
2021-05-13 16:54 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).