| 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
| ||
|
Message-ID:
<CAGwozwEGLB+duOD=GyAZ25fFm0RmoEdawAxvLJ+_nJBTNmcEqg@mail.gmail.com>
Date: Sat, 12 Apr 2025 09:59:28 +0200
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: platform-driver-x86@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-doc@...r.kernel.org, linux-pm@...r.kernel.org,
Guenter Roeck <linux@...ck-us.net>, Jean Delvare <jdelvare@...e.com>,
Jonathan Corbet <corbet@....net>,
Joaquin Ignacio Aramendia <samsagax@...il.com>,
Derek J Clark <derekjohn.clark@...il.com>,
Kevin Greenberg <kdgreenberg234@...tonmail.com>,
Joshua Tam <csinaction@...me>,
Parth Menon <parthasarathymenon@...il.com>, Eileen <eileen@...-netbook.com>,
LKML <linux-kernel@...r.kernel.org>, sre@...nel.org, linux@...ssschuh.net,
Hans de Goede <hdegoede@...hat.com>, mario.limonciello@....com
Subject: Re: [PATCH v8 11/14] platform/x86: oxpec: Adhere to sysfs-class-hwmon
and enable pwm on 2
On Fri, 11 Apr 2025 at 17:13, Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> On Sat, 22 Mar 2025, Antheas Kapenekakis wrote:
>
> > Currently, the driver does not adhere to the sysfs-class-hwmon
> > specification: 0 is used for auto fan control and 1 is used for manual
> > control. However, it is expected that 0 sets the fan to full speed,
> > 1 sets the fan to manual, and then 2 is used for automatic control.
> >
> > Therefore, change the sysfs API to reflect this and enable pwm on 2.
> >
> > As we are breaking the ABI for this driver, rename oxpec to oxp_ec,
> > reflecting the naming convention used by other drivers, to allow for
> > a smooth migration in current userspace programs.
>
> So there's no gracious deprecation of the previous interface? It doesn't
> sound "smooth" by any means from userspace perspective.
The previous interface was not compliant with the hwmon ABI, so any
software that used it without being cognisant of that would misbehave.
This driver got really fleshed out with 6.12, before that there was
one userspace software that relied on this. We made sure to update all
software that binds to oxpec specifically so it is not a problem now.
By adding a dash at the name at the same time as the change is done it
is possible to handle both the previous interface and this one at the
same time.
It is not ideal by any means, but if we don't change it now we will
not be able to again.
> > Closes: https://lore.kernel.org/linux-hwmon/20241027174836.8588-1-derekjohn.clark@gmail.com/
> > Reviewed-by: Derek J. Clark <derekjohn.clark@...il.com>
> > Reviewed-by: Thomas Weißschuh <linux@...ssschuh.net>
> > Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> > ---
> > drivers/platform/x86/oxpec.c | 37 ++++++++++++++++++++++++++++++++----
> > 1 file changed, 33 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/platform/x86/oxpec.c b/drivers/platform/x86/oxpec.c
> > index 5b84647569931..3bf2c597e9b00 100644
> > --- a/drivers/platform/x86/oxpec.c
> > +++ b/drivers/platform/x86/oxpec.c
> > @@ -731,7 +731,27 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
> > case hwmon_pwm_input:
> > return oxp_pwm_input_read(val);
> > case hwmon_pwm_enable:
> > - return oxp_pwm_read(val);
> > + ret = oxp_pwm_read(val);
> > + if (ret)
> > + return ret;
> > +
> > + /* Check for auto and return 2 */
> > + if (!*val) {
> > + *val = 2;
> > + return 0;
> > + }
> > +
> > + /* Return 0 if at full fan speed, 1 otherwise */
> > + ret = oxp_pwm_fan_speed(val);
> > + if (ret)
> > + return ret;
> > +
> > + if (*val == 255)
> > + *val = 0;
> > + else
> > + *val = 1;
>
> Why all these literals? These should be named properly with defines are
> there some defines/enums for some of them already if this hwmon ABI?
Specifically for pwm_enable I don't think so actually. With a quick
grep all drivers I checked use literals.
> > +
> > + return 0;
> > default:
> > break;
> > }
> > @@ -745,15 +765,24 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
> > static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
> > u32 attr, int channel, long val)
> > {
> > + int ret;
> > +
> > switch (type) {
> > case hwmon_pwm:
> > switch (attr) {
> > case hwmon_pwm_enable:
> > if (val == 1)
> > return oxp_pwm_enable();
> > - else if (val == 0)
> > + else if (val == 2)
> > return oxp_pwm_disable();
> > - return -EINVAL;
> > + else if (val != 0)
>
> Literals here too should be changed.
>
> > + return -EINVAL;
> > +
> > + /* Enable PWM and set to max speed */
> > + ret = oxp_pwm_enable();
> > + if (ret)
> > + return ret;
> > + return oxp_pwm_input_write(255);
> > case hwmon_pwm_input:
> > return oxp_pwm_input_write(val);
> > default:
> > @@ -818,7 +847,7 @@ static int oxp_platform_probe(struct platform_device *pdev)
> > struct device *dev = &pdev->dev;
> > struct device *hwdev;
> >
> > - hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", NULL,
> > + hwdev = devm_hwmon_device_register_with_info(dev, "oxp_ec", NULL,
> > &oxp_ec_chip_info, NULL);
> >
> > return PTR_ERR_OR_ZERO(hwdev);
> >
>
> --
> i.
Powered by blists - more mailing lists