linux-kernel - Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS screenpad

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: <39570a-d3a-13b9-f2d-e75b3d60c6c4@linux.intel.com>
Date:   Mon, 8 May 2023 11:21:10 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Luke Jones <luke@...nes.dev>
cc:     platform-driver-x86@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        acpi4asus-user@...ts.sourceforge.net, hdegoede@...hat.com,
        corentin.chary@...il.com, markgross@...nel.org, jdelvare@...e.com,
        linux@...ck-us.net
Subject: Re: [PATCH v2 1/1] platform/x86: asus-wmi: add support for ASUS
 screenpad

On Sat, 6 May 2023, Luke Jones wrote:
> On Fri, May 5 2023 at 16:08:16 +0300, Ilpo Järvinen
> <ilpo.jarvinen@...ux.intel.com> wrote:
> > On Fri, 5 May 2023, Luke D. Jones wrote:
> > 
> > >  Add support for the WMI methods used to turn off and adjust the
> > >  brightness of the secondary "screenpad" device found on some high-end
> > >  ASUS laptops like the GX650P series and others.
> > > 
> > >  These methods are utilised in a new backlight device named:
> > >  - asus_screenpad
> > > 
> > >  Signed-off-by: Luke D. Jones <luke@...nes.dev>
> > >  ---

> > >  @@ -3208,6 +3209,129 @@ static int is_display_toggle(int code)
> > >   	return 0;
> > >   }
> > > 
> > >  +/* Screenpad backlight */
> > >  +
> > >  +static int read_screenpad_backlight_power(struct asus_wmi *asus)
> > >  +{
> > >  +	int ret = asus_wmi_get_devstate_simple(asus,
> > > ASUS_WMI_DEVID_SCREENPAD_POWER);
> > 
> > Please move this to own line because now you have the extra newline
> > in between the call and error handling.
> 
> I don't understand what you mean sorry. Remove the new line or:
> int ret;
> ret = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_SCREENPAD_POWER);

Don't do:

int func()
{
	int ret = func();

	if (ret < 0)
		return ret;
	...
}

But do:

int func()
{
	int ret;

	ret = func();
	if (ret < 0)
		return ret;
	...
}

This keeps the error handling next to the actual call following the usual 
error handling pattern (natural logic grouping). The added clarity is well 
worth the one extra line required.

> > >  +static int update_screenpad_bl_status(struct backlight_device *bd)
> > >  +{
> > >  +	struct asus_wmi *asus = bl_get_data(bd);
> > >  +	int power, err = 0;
> > >  +	u32 ctrl_param;
> > >  +
> > >  +	power = read_screenpad_backlight_power(asus);
> > >  +	if (power == -ENODEV)
> > >  +		return err;
> > 
> > Just return 0. Or is there perhaps something wrong/missing here?
> 
> I thought the correct thing was to return any possible error state (here,
> anything less than 0 would be an error, right?)

I think this was covered already but here what I meant, use either:
	return 0;
because err is always 0 at that point, or:
	return power;
Depending on which is correct, I wasn't sure because you had this after 
it:

> > >  +	else if (power < 0)
> > >  +		return power;

...So I thought you might really intentionally wanted to return 0 there.


-- 
 i.