linux-kernel - Re: [PATCH v3 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum

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]

Date:   Tue, 9 Aug 2022 10:37:05 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     "Luke D. Jones" <luke@...nes.dev>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum

On Tue, Aug 9, 2022 at 5:30 AM Luke D. Jones <luke@...nes.dev> wrote:
>
> Due to multiple types of tablet/lidflip, the existing code for
> handling these events is refactored to use an enum for each type.

...

>  static int asus_wmi_input_init(struct asus_wmi *asus)
>  {
> +       struct device *dev;
>         int err, result;
>
> +       dev = &asus->platform_device->dev;
> +

While the discussed pattern of splitting assignments is a good
practice, for some cases we don't do it when we rely on the guarantee
by the callers that some of the stuff won't be problematic. Here the
device is part of the platform device and can't be NULL, there is no
point to split definition and assignment (and you may find plenty
examples in the kernel), so

  struct device *dev = &asus->platform_device->dev;

is better to have as it's guaranteed not to be NULL and since that we
don't check it in the code anyway.

...

>                         input_report_switch(asus->inputdev, SW_TABLET_MODE,
> -                                           !result);
> +                                               !result);

Irrelevant change.

...

It also seems you switched to dev_err() here for the pr_err() that
aren't related to the change, either split that to a separate patch,
or don't do it right now. I.o.w. use dev_err() only in the lines your
change touches, otherwise it's irrelevant.

-- 
With Best Regards,
Andy Shevchenko