linux-kernel - Re: [PATCH v2 2/2] asus-wmi: Add support for ROG X13 tablet mode

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: <CAHp75Vda5KX5pVrNeueQEODoEy405eTb9SYJtts-Lm9jMNocHQ@mail.gmail.com>
Date:   Tue, 9 Aug 2022 09:12:37 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Luke Jones <luke@...nes.dev>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] asus-wmi: Add support for ROG X13 tablet mode

On Tue, Aug 9, 2022 at 5:26 AM Luke Jones <luke@...nes.dev> wrote:

...

> >>  +                       pr_err("This device has lid-flip-rog quirk
> >> but got ENODEV checking it. This is a bug.");
> >
> > dev_err() ?
>
> Okay, changed here and in previous patch to match it.
>
> So that I'm clearer on dev_err(), this doesn't do something like exit
> the module does it? It's just a more detailed error print?

Yes, it's more specific when the user sees it. The pr_err() is global
and anonymous (you can only point to the driver, and not the instance
of the device bound to it), while dev_err() is device specific and the
user will immediately see which device instance is failing. Yet it's
not a problem for this particular driver, because I don't believe one
may have two, but it's a good coding practice in general.

(Note the last sentence: "good coding practice")

...

> >>  +static void lid_flip_rog_tablet_mode_get_state(struct asus_wmi
> >> *asus)
> >>  +{
> >>  +       int result = asus_wmi_get_devstate_simple(asus,
> >> ASUS_WMI_DEVID_LID_FLIP_ROG);
> >>  +
> >>  +       if (result >= 0) {
> >
> > First of all, it's better to decouple assignment and definition, and
> > move assignment closer to its user. This is usual pattern.
>
> I don't fully understand why you would want the separation given how
> short these two blocks are (I'll change in this and previous patch of
> course, I just don't personally understand it).

See above, "good coding practice". Why?

Imagine your code to be in hypothetical v5.10:

  int x = foo(param1, param2, ...);

  if (x)
    return Y;


Now, at v5.12 somebody adds a new feature which touches your code:

  int x = foo(param1, param2, ...);
  struct bar *baz;

  if (we_have_such_feature_disabled)
    return Z;

  if (x)
    return Y;

  baz = ...

And then somebody else in v5.13 does another feature:

  int x = foo(param1, param2, ...);
  struct bar *baz;

  if (we_have_such_feature_disabled)
    return Z;

  /* parameter 1 can be NULL, check it */
  if (!param1)
    return -EINVAL;

  if (x)
    return Y;

  baz = ...

Do you see now an issue? If you emulate this as a sequence of Git
changes the last one is easily missing subtle detail. That's why "good
coding practice".

-- 
With Best Regards,
Andy Shevchenko