| Message ID | 20210728205142.8959-1-W_Armin@gmx.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | hwmon: (dell-smm-hwmon) Convert to new hwmon registration api | expand |
On 7/28/21 1:51 PM, W_Armin@gmx.de wrote: > From: Armin Wolf <W_Armin@gmx.de> > > This patch series is converting the dell-smm-hwmon driver > to the new hwmon registration API. In order to do so, > it introduces a platform device in the first patch, and > applies some optimisations in the next three patches. > The switch to the new hwmon registration API is done in > the next patch. The last patch is fixing a small bug. > > The caching of the fan/temp values was modified to better fit > the new hwmon API. > > The patches work fine for my Dell Latitude C600, but i whould > appreciate someone testing the code on another model too. > > Changes in v6: > - Make pwm1_enable permissions write-only Sorry, guess I am missing something. Why ? Guenter > - Do not test fan speed in dell_smm_is_visible() > > Changes in v5: > - Fix checkpatch warning after patch 5/6 > - Hide fanX_label if fan type calls are disallowed > > Changes in v4: > - Make fan detection behave like before patch 5/6 > - Update coverletter title > > Changes in v3: > - Update description of patch 1/6 and remove empty change > - Let pwm1_enable remain write-only > - Include a small bugfix > > Changes in v2: > - Fix coverletter title > - Update docs regarding pwm1_enable > > Armin Wolf (6): > hwmon: (dell-smm-hwmon) Use platform device > hwmon: (dell-smm-hwmon) Mark functions as __init > hwmon: (dell-smm-hwmon) Use devm_add_action_or_reset() > hwmon: (dell-smm-hwmon) Move variables into a driver private data > structure > hwmon: (dell-smm-hwmon) Convert to > devm_hwmon_device_register_with_info() > hwmon: (dell-smm-hwmon) Fix fan mutliplier detection for 3rd fan > > drivers/hwmon/dell-smm-hwmon.c | 847 ++++++++++++++++----------------- > 1 file changed, 419 insertions(+), 428 deletions(-) > > -- > 2.20.1 >
On 28.07.21 23:07 Guenter Roeck wrote: > On 7/28/21 1:51 PM, W_Armin@gmx.de wrote: >> From: Armin Wolf <W_Armin@gmx.de> >> >> This patch series is converting the dell-smm-hwmon driver >> to the new hwmon registration API. In order to do so, >> it introduces a platform device in the first patch, and >> applies some optimisations in the next three patches. >> The switch to the new hwmon registration API is done in >> the next patch. The last patch is fixing a small bug. >> >> The caching of the fan/temp values was modified to better fit >> the new hwmon API. >> >> The patches work fine for my Dell Latitude C600, but i whould >> appreciate someone testing the code on another model too. >> >> Changes in v6: >> - Make pwm1_enable permissions write-only > > Sorry, guess I am missing something. Why ? > > Guenter > pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file permissions where 0200. In the v5 patch series however, the file permission where not 0200, so i changed that. Armin >> - Do not test fan speed in dell_smm_is_visible() >> >> Changes in v5: >> - Fix checkpatch warning after patch 5/6 >> - Hide fanX_label if fan type calls are disallowed >> >> Changes in v4: >> - Make fan detection behave like before patch 5/6 >> - Update coverletter title >> >> Changes in v3: >> - Update description of patch 1/6 and remove empty change >> - Let pwm1_enable remain write-only >> - Include a small bugfix >> >> Changes in v2: >> - Fix coverletter title >> - Update docs regarding pwm1_enable >> >> Armin Wolf (6): >> hwmon: (dell-smm-hwmon) Use platform device >> hwmon: (dell-smm-hwmon) Mark functions as __init >> hwmon: (dell-smm-hwmon) Use devm_add_action_or_reset() >> hwmon: (dell-smm-hwmon) Move variables into a driver private data >> structure >> hwmon: (dell-smm-hwmon) Convert to >> devm_hwmon_device_register_with_info() >> hwmon: (dell-smm-hwmon) Fix fan mutliplier detection for 3rd fan >> >> drivers/hwmon/dell-smm-hwmon.c | 847 ++++++++++++++++----------------- >> 1 file changed, 419 insertions(+), 428 deletions(-) >> >> -- >> 2.20.1 >> >
On 7/28/21 2:19 PM, Armin Wolf wrote: > On 28.07.21 23:07 Guenter Roeck wrote: >> On 7/28/21 1:51 PM, W_Armin@gmx.de wrote: >>> From: Armin Wolf <W_Armin@gmx.de> >>> >>> This patch series is converting the dell-smm-hwmon driver >>> to the new hwmon registration API. In order to do so, >>> it introduces a platform device in the first patch, and >>> applies some optimisations in the next three patches. >>> The switch to the new hwmon registration API is done in >>> the next patch. The last patch is fixing a small bug. >>> >>> The caching of the fan/temp values was modified to better fit >>> the new hwmon API. >>> >>> The patches work fine for my Dell Latitude C600, but i whould >>> appreciate someone testing the code on another model too. >>> >>> Changes in v6: >>> - Make pwm1_enable permissions write-only >> >> Sorry, guess I am missing something. Why ? >> >> Guenter >> > pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file > permissions where 0200. > In the v5 patch series however, the file permission where not 0200, so i > changed that. > Is there a _reason_ for declaring this attribute write only, other than "it used to be that way" ? Guenter
Am 28.07.21 um 23:28 schrieb Guenter Roeck: > On 7/28/21 2:19 PM, Armin Wolf wrote: >> On 28.07.21 23:07 Guenter Roeck wrote: >>> On 7/28/21 1:51 PM, W_Armin@gmx.de wrote: >>>> From: Armin Wolf <W_Armin@gmx.de> >>>> >>>> This patch series is converting the dell-smm-hwmon driver >>>> to the new hwmon registration API. In order to do so, >>>> it introduces a platform device in the first patch, and >>>> applies some optimisations in the next three patches. >>>> The switch to the new hwmon registration API is done in >>>> the next patch. The last patch is fixing a small bug. >>>> >>>> The caching of the fan/temp values was modified to better fit >>>> the new hwmon API. >>>> >>>> The patches work fine for my Dell Latitude C600, but i whould >>>> appreciate someone testing the code on another model too. >>>> >>>> Changes in v6: >>>> - Make pwm1_enable permissions write-only >>> >>> Sorry, guess I am missing something. Why ? >>> >>> Guenter >>> >> pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file >> permissions where 0200. >> In the v5 patch series however, the file permission where not 0200, so i >> changed that. >> > > Is there a _reason_ for declaring this attribute write only, other than > "it used to be that way" ? > > Guenter I dont think so, dell_smm_read will just return -EOPNOTSUPP if someone tries to read pwm1_enable. Armin
On 7/28/21 2:34 PM, Armin Wolf wrote: > Am 28.07.21 um 23:28 schrieb Guenter Roeck: >> On 7/28/21 2:19 PM, Armin Wolf wrote: >>> On 28.07.21 23:07 Guenter Roeck wrote: >>>> On 7/28/21 1:51 PM, W_Armin@gmx.de wrote: >>>>> From: Armin Wolf <W_Armin@gmx.de> >>>>> >>>>> This patch series is converting the dell-smm-hwmon driver >>>>> to the new hwmon registration API. In order to do so, >>>>> it introduces a platform device in the first patch, and >>>>> applies some optimisations in the next three patches. >>>>> The switch to the new hwmon registration API is done in >>>>> the next patch. The last patch is fixing a small bug. >>>>> >>>>> The caching of the fan/temp values was modified to better fit >>>>> the new hwmon API. >>>>> >>>>> The patches work fine for my Dell Latitude C600, but i whould >>>>> appreciate someone testing the code on another model too. >>>>> >>>>> Changes in v6: >>>>> - Make pwm1_enable permissions write-only >>>> >>>> Sorry, guess I am missing something. Why ? >>>> >>>> Guenter >>>> >>> pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file >>> permissions where 0200. >>> In the v5 patch series however, the file permission where not 0200, so i >>> changed that. >>> >> >> Is there a _reason_ for declaring this attribute write only, other than >> "it used to be that way" ? >> >> Guenter > > I dont think so, dell_smm_read will just return -EOPNOTSUPP if someone tries to read pwm1_enable. > > Armin > That is not what I meant. Is there a reason to not report the current status of pwm1_enable to the user ? In other words, does the firmware not report its current status ? Guenter
Am 28.07.21 um 23:37 schrieb Guenter Roeck: > On 7/28/21 2:34 PM, Armin Wolf wrote: >> Am 28.07.21 um 23:28 schrieb Guenter Roeck: >>> On 7/28/21 2:19 PM, Armin Wolf wrote: >>>> On 28.07.21 23:07 Guenter Roeck wrote: >>>>> On 7/28/21 1:51 PM, W_Armin@gmx.de wrote: >>>>>> From: Armin Wolf <W_Armin@gmx.de> >>>>>> >>>>>> This patch series is converting the dell-smm-hwmon driver >>>>>> to the new hwmon registration API. In order to do so, >>>>>> it introduces a platform device in the first patch, and >>>>>> applies some optimisations in the next three patches. >>>>>> The switch to the new hwmon registration API is done in >>>>>> the next patch. The last patch is fixing a small bug. >>>>>> >>>>>> The caching of the fan/temp values was modified to better fit >>>>>> the new hwmon API. >>>>>> >>>>>> The patches work fine for my Dell Latitude C600, but i whould >>>>>> appreciate someone testing the code on another model too. >>>>>> >>>>>> Changes in v6: >>>>>> - Make pwm1_enable permissions write-only >>>>> >>>>> Sorry, guess I am missing something. Why ? >>>>> >>>>> Guenter >>>>> >>>> pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file >>>> permissions where 0200. >>>> In the v5 patch series however, the file permission where not 0200, >>>> so i >>>> changed that. >>>> >>> >>> Is there a _reason_ for declaring this attribute write only, other than >>> "it used to be that way" ? >>> >>> Guenter >> >> I dont think so, dell_smm_read will just return -EOPNOTSUPP if >> someone tries to read pwm1_enable. >> >> Armin >> > That is not what I meant. Is there a reason to not report > the current status of pwm1_enable to the user ? In other > words, does the firmware not report its current status ? > > Guenter Pali said the driver cannot query the state of pwm1_enable from the BIOS, and with userspace tools being able to change the state of BIOS fan control behind our back, we cannot simply return the last set value.
On Wednesday 28 July 2021 23:40:21 Armin Wolf wrote: > Am 28.07.21 um 23:37 schrieb Guenter Roeck: > > On 7/28/21 2:34 PM, Armin Wolf wrote: > > > Am 28.07.21 um 23:28 schrieb Guenter Roeck: > > > > On 7/28/21 2:19 PM, Armin Wolf wrote: > > > > > On 28.07.21 23:07 Guenter Roeck wrote: > > > > > > On 7/28/21 1:51 PM, W_Armin@gmx.de wrote: > > > > > > > From: Armin Wolf <W_Armin@gmx.de> > > > > > > > > > > > > > > This patch series is converting the dell-smm-hwmon driver > > > > > > > to the new hwmon registration API. In order to do so, > > > > > > > it introduces a platform device in the first patch, and > > > > > > > applies some optimisations in the next three patches. > > > > > > > The switch to the new hwmon registration API is done in > > > > > > > the next patch. The last patch is fixing a small bug. > > > > > > > > > > > > > > The caching of the fan/temp values was modified to better fit > > > > > > > the new hwmon API. > > > > > > > > > > > > > > The patches work fine for my Dell Latitude C600, but i whould > > > > > > > appreciate someone testing the code on another model too. > > > > > > > > > > > > > > Changes in v6: > > > > > > > - Make pwm1_enable permissions write-only > > > > > > > > > > > > Sorry, guess I am missing something. Why ? > > > > > > > > > > > > Guenter > > > > > > > > > > > pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file > > > > > permissions where 0200. > > > > > In the v5 patch series however, the file permission where not 0200, > > > > > so i > > > > > changed that. > > > > > > > > > > > > > Is there a _reason_ for declaring this attribute write only, other than > > > > "it used to be that way" ? > > > > > > > > Guenter > > > > > > I dont think so, dell_smm_read will just return -EOPNOTSUPP if > > > someone tries to read pwm1_enable. > > > > > > Armin > > > > > That is not what I meant. Is there a reason to not report > > the current status of pwm1_enable to the user ? In other > > words, does the firmware not report its current status ? > > > > Guenter > > Pali said the driver cannot query the state of pwm1_enable from the BIOS, and with userspace tools being able to change > the state of BIOS fan control behind our back, we cannot simply return the last set value. We have already discussed this problem years ago, see: https://lore.kernel.org/linux-hwmon/20160522152823.GA18331@roeck-us.net/ And also again this year: https://lore.kernel.org/linux-hwmon/20210528211037.2tnblovgb3rkcwnq@pali/ Basically there is no known firmware command to retrieve current status yet. And both userspace and SMM/firmware itself can change state. So kernel has no way how to retrieve current status and caching last value cannot be used (due to userspace and firmware can change it). Whole SMM API is undocumented and seems that Dell does not want to provide any documentation for it. I know that it is pity that we have no read functionality, but we have to deal with it... Maybe it would make sense to add comment into driver code, why attribute is write-only...
On 7/28/21 2:54 PM, Pali Rohár wrote: > On Wednesday 28 July 2021 23:40:21 Armin Wolf wrote: >> Am 28.07.21 um 23:37 schrieb Guenter Roeck: >>> On 7/28/21 2:34 PM, Armin Wolf wrote: >>>> Am 28.07.21 um 23:28 schrieb Guenter Roeck: >>>>> On 7/28/21 2:19 PM, Armin Wolf wrote: >>>>>> On 28.07.21 23:07 Guenter Roeck wrote: >>>>>>> On 7/28/21 1:51 PM, W_Armin@gmx.de wrote: >>>>>>>> From: Armin Wolf <W_Armin@gmx.de> >>>>>>>> >>>>>>>> This patch series is converting the dell-smm-hwmon driver >>>>>>>> to the new hwmon registration API. In order to do so, >>>>>>>> it introduces a platform device in the first patch, and >>>>>>>> applies some optimisations in the next three patches. >>>>>>>> The switch to the new hwmon registration API is done in >>>>>>>> the next patch. The last patch is fixing a small bug. >>>>>>>> >>>>>>>> The caching of the fan/temp values was modified to better fit >>>>>>>> the new hwmon API. >>>>>>>> >>>>>>>> The patches work fine for my Dell Latitude C600, but i whould >>>>>>>> appreciate someone testing the code on another model too. >>>>>>>> >>>>>>>> Changes in v6: >>>>>>>> - Make pwm1_enable permissions write-only >>>>>>> >>>>>>> Sorry, guess I am missing something. Why ? >>>>>>> >>>>>>> Guenter >>>>>>> >>>>>> pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file >>>>>> permissions where 0200. >>>>>> In the v5 patch series however, the file permission where not 0200, >>>>>> so i >>>>>> changed that. >>>>>> >>>>> >>>>> Is there a _reason_ for declaring this attribute write only, other than >>>>> "it used to be that way" ? >>>>> >>>>> Guenter >>>> >>>> I dont think so, dell_smm_read will just return -EOPNOTSUPP if >>>> someone tries to read pwm1_enable. >>>> >>>> Armin >>>> >>> That is not what I meant. Is there a reason to not report >>> the current status of pwm1_enable to the user ? In other >>> words, does the firmware not report its current status ? >>> >>> Guenter >> >> Pali said the driver cannot query the state of pwm1_enable from the BIOS, and with userspace tools being able to change >> the state of BIOS fan control behind our back, we cannot simply return the last set value. > > We have already discussed this problem years ago, see: > https://lore.kernel.org/linux-hwmon/20160522152823.GA18331@roeck-us.net/ > > And also again this year: > https://lore.kernel.org/linux-hwmon/20210528211037.2tnblovgb3rkcwnq@pali/ > > Basically there is no known firmware command to retrieve current status > yet. And both userspace and SMM/firmware itself can change state. So > kernel has no way how to retrieve current status and caching last value > cannot be used (due to userspace and firmware can change it). > > Whole SMM API is undocumented and seems that Dell does not want to > provide any documentation for it. > > I know that it is pity that we have no read functionality, but we have > to deal with it... > > Maybe it would make sense to add comment into driver code, why attribute > is write-only... > Yes, please. Thanks, Guenter
From: Armin Wolf <W_Armin@gmx.de> This patch series is converting the dell-smm-hwmon driver to the new hwmon registration API. In order to do so, it introduces a platform device in the first patch, and applies some optimisations in the next three patches. The switch to the new hwmon registration API is done in the next patch. The last patch is fixing a small bug. The caching of the fan/temp values was modified to better fit the new hwmon API. The patches work fine for my Dell Latitude C600, but i whould appreciate someone testing the code on another model too. Changes in v6: - Make pwm1_enable permissions write-only - Do not test fan speed in dell_smm_is_visible() Changes in v5: - Fix checkpatch warning after patch 5/6 - Hide fanX_label if fan type calls are disallowed Changes in v4: - Make fan detection behave like before patch 5/6 - Update coverletter title Changes in v3: - Update description of patch 1/6 and remove empty change - Let pwm1_enable remain write-only - Include a small bugfix Changes in v2: - Fix coverletter title - Update docs regarding pwm1_enable Armin Wolf (6): hwmon: (dell-smm-hwmon) Use platform device hwmon: (dell-smm-hwmon) Mark functions as __init hwmon: (dell-smm-hwmon) Use devm_add_action_or_reset() hwmon: (dell-smm-hwmon) Move variables into a driver private data structure hwmon: (dell-smm-hwmon) Convert to devm_hwmon_device_register_with_info() hwmon: (dell-smm-hwmon) Fix fan mutliplier detection for 3rd fan drivers/hwmon/dell-smm-hwmon.c | 847 ++++++++++++++++----------------- 1 file changed, 419 insertions(+), 428 deletions(-) -- 2.20.1