linux-kernel - Re: [PATCH v1 1/1] gpio: Use traditional pattern when checking error codes

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] [day] [month] [year] [list]
Message-ID: <CAMRc=MfJqqBvSj_RmRMfMRwkfz80iF1fA2Rvu+vBm4wSineufQ@mail.gmail.com>
Date: Thu, 31 Oct 2024 19:23:23 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, linux-gpio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH v1 1/1] gpio: Use traditional pattern when checking error codes

On Thu, Oct 31, 2024 at 10:15 AM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Wed, Oct 30, 2024 at 09:20:45PM +0100, Bartosz Golaszewski wrote:
> > On Mon, Oct 28, 2024 at 2:44 PM Andy Shevchenko
> > <andriy.shevchenko@...ux.intel.com> wrote:
> > >
> > > Instead of 'if (ret == 0)' switch to "check for the error first" rule.
> >
> > Well there's much more to this patch than that and I have some issues with it.
> >
> > > While it gives a "+" (plus) statistics it makes the code easier to read
> >
> > Not only does it increase the footprint but it also adds completely
> > unnecessary goto labels.
>
> These pieces can be dropped.
>
> ...
>
> > > and maintain (when, e.g., want to add somethning in between touched lines).
> >
> > The single line calls to the notifier chain are unlikely to be
> > extended anytime soon but even then I think we should cross that
> > bridge when we get there.
>
> Okay.
>
> ...
>
> > > -       if (!ret)
> > > -               gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > +       if (ret)
> > > +               return ret;
> > >
> > > -       return ret;
> > > +       gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > +       return 0;
> > >  }
> >
> > I really don't see how this makes it better. The logic here is: if the
> > underlying set config worked fine - emit the event. Otherwise continue
> > with the function (even if there's nothing there now). If anything
> > you're making it more difficult to modify later because logically the
> > notification is just an optional step on the way to returning from the
> > function.
>
> Optional steps are covered by flags, and not by checking the previous call for
> failure. So, I barely see the "optionality" of the notifications in these calls.
>
> ...
>
> > >         ret = gpiod_direction_input_nonotify(desc);
> > > -       if (ret == 0)
> > > -               gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > +       if (ret)
> > > +               return ret;
> >
> > Ok, for consistency I could take it but please put this into a
> > separate commit doing just that (here and elsewhere).
>
> Based on the other comments from you in this email I'm not sure I understood
> this correctly. Do you want to reject the complete patch, or do you agree on
> some pieces out of it.
>

I want to reject most of the patch in this form. s/(ret == 0)/(!ret)/
is fine as a standalone change.

> > > -       return ret;
> > > +       gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > +       return 0;
>
> ...
>
> > >         ret = gpio_do_set_config(desc, config);
> > > -       if (!ret) {
> > > -               /* These are the only options we notify the userspace about. */
> > > -               switch (pinconf_to_config_param(config)) {
> > > -               case PIN_CONFIG_BIAS_DISABLE:
> > > -               case PIN_CONFIG_BIAS_PULL_DOWN:
> > > -               case PIN_CONFIG_BIAS_PULL_UP:
> > > -               case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > -               case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> > > -               case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > -               case PIN_CONFIG_INPUT_DEBOUNCE:
> > > -                       gpiod_line_state_notify(desc,
> > > -                                               GPIO_V2_LINE_CHANGED_CONFIG);
> > > -                       break;
> > > -               default:
> > > -                       break;
> > > -               }
> >
> > If you really want to get rid of one level of indentation here,
> > I suggest moving it into a separate function.
>
> Perhaps you suggested a separate change for that?
>

This doesn't really bother me but if it triggers you then feel free to
change it like:

if (!ret)
    gpio_set_config_line_state_notify(desc);

Bart

> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       /* These are the only options we notify the userspace about */
> > > +       switch (pinconf_to_config_param(config)) {
> > > +       case PIN_CONFIG_BIAS_DISABLE:
> > > +       case PIN_CONFIG_BIAS_PULL_DOWN:
> > > +       case PIN_CONFIG_BIAS_PULL_UP:
> > > +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > +       case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> > > +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > +       case PIN_CONFIG_INPUT_DEBOUNCE:
> > > +               gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> > > +               break;
> > > +       default:
> > > +               break;
> > >         }
> > >
> > > -       return ret;
> > > +       return 0;
>
> ...
>
> > Most of this is IMO pointless churn. You typically do a lot of great
> > cleanups but this just doesn't make sense. Sorry but NAK.
>
> OK, I do one change out of that with deduplication of the direction input call,
> the rest is up to you, let's it be less readable.
>