linux-kernel - Re: [PATCH] platform/x86: thinkpad_acpi: Add Thinkpad Edge E531 fan support

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: <456fea75-b6d2-4150-86db-ce385e2860b1@linux.intel.com>
Date: Mon, 12 Aug 2024 10:17:48 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Matthias Fetzer <kontakt@...thias-fetzer.de>
cc: hmh@....eng.br, Hans de Goede <hdegoede@...hat.com>, 
    ibm-acpi-devel@...ts.sourceforge.net, platform-driver-x86@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: Add Thinkpad Edge E531 fan
 support

On Fri, 9 Aug 2024, Matthias Fetzer wrote:
>
> Thanks for the review!
> 
> Am 08.08.24 um 15:14 schrieb Ilpo Järvinen:
> > On Sun, 14 Jul 2024, Matthias Fetzer wrote:
> > 
> > > Fan control on the E531 is done using the ACPI methods FANG and
> > > FANW. The correct parameters and register values were found by
> > > analyzing EC firmware as well as DSDT. This has been tested on
> > > my Thinkpad Edge E531 (6885CTO, BIOS HEET52WW 1.33).
> > > 
> > > Signed-off-by: Matthias Fetzer <kontakt@...thias-fetzer.de>

> > > @@ -8157,6 +8213,7 @@ static int fan2_get_speed(unsigned int *speed)
> > >     static int fan_set_level(int level)
> > >   {
> > > +	int rc;
> > >   	if (!fan_control_allowed)
> > >   		return -EPERM;
> > >   @@ -8206,6 +8263,36 @@ static int fan_set_level(int level)
> > >   			tp_features.fan_ctrl_status_undef = 0;
> > >   		break;
> > >   +	case TPACPI_FAN_WR_ACPI_FANW:
> > > +		if ((!(level & TP_EC_FAN_AUTO) &&
> > > +		    ((level < 0) || (level > 7))) ||
> > > +		    (level & TP_EC_FAN_FULLSPEED))
> > > +			return -EINVAL;
> > 
> > I'd split this into two to make it more readable:
> > 
> > 		if (!(level & TP_EC_FAN_AUTO) && (level < 0 || level > 7))
> > 			return -EINVAL;
> > 		if (level & TP_EC_FAN_FULLSPEED)
> > 			return -EINVAL;
> 
> This is much better, thanks.
> 
> > 
> > I'm not sure if -EINVAL is really the right code to return though in these
> > cases.
> > 
> 
> I thought that since those are invalid values/parameters the return code of
> -EINVAL
> would be a good choice. What do you suggest to use instead?

Actually, now that I look into it more carefully, forget what I said.
I think -EINVAL is correct to return in these cases because the input 
value is invalid (I previously assumed something else based on the 
define names).

-- 
 i.