| 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
| ||
|
Message-ID: <CAMRc=Mc_PW32jNO+C5AEQK6ej_CsCSV-HY76aJoQ6bjZ=JPOtg@mail.gmail.com>
Date: Wed, 30 Oct 2024 21:20:45 +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 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.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>
> 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.
> 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.
> drivers/gpio/gpiolib.c | 104 ++++++++++++++++++++++-------------------
> 1 file changed, 56 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5666c462248c..a9a3e032ed5b 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2674,10 +2674,11 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
> ret = gpio_set_config_with_argument_optional(desc,
> PIN_CONFIG_INPUT_DEBOUNCE,
> debounce);
> - 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.
>
> /**
> @@ -2697,16 +2698,17 @@ int gpiod_direction_input(struct gpio_desc *desc)
> VALIDATE_DESC(desc);
>
> 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).
> - return ret;
> + gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> + return 0;
> }
> EXPORT_SYMBOL_GPL(gpiod_direction_input);
>
> int gpiod_direction_input_nonotify(struct gpio_desc *desc)
> {
> - int ret = 0;
> + int ret;
>
> CLASS(gpio_chip_guard, guard)(desc);
> if (!guard.gc)
> @@ -2733,6 +2735,8 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
> if (guard.gc->direction_input) {
> ret = guard.gc->direction_input(guard.gc,
> gpio_chip_hwgpio(desc));
> + if (ret)
> + goto out_trace_direction;
> } else if (guard.gc->get_direction &&
> (guard.gc->get_direction(guard.gc,
> gpio_chip_hwgpio(desc)) != 1)) {
> @@ -2741,11 +2745,11 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc)
> __func__);
> return -EIO;
> }
> - if (ret == 0) {
> - clear_bit(FLAG_IS_OUT, &desc->flags);
> - ret = gpio_set_bias(desc);
> - }
>
> + clear_bit(FLAG_IS_OUT, &desc->flags);
> + ret = gpio_set_bias(desc);
> +
> +out_trace_direction:
This makes it worse! We can't just apply the "return fast" rule
indiscriminately. Sometimes it does make sense to go "if the previous
step worked, do this, otherwise keep going".
Also: adding more labels for no reason?
> trace_gpio_direction(desc_to_gpio(desc), 1, ret);
>
> return ret;
> @@ -2774,6 +2778,8 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
> if (guard.gc->direction_output) {
> ret = guard.gc->direction_output(guard.gc,
> gpio_chip_hwgpio(desc), val);
> + if (ret)
> + goto out_trace_value_and_direction;
> } else {
> /* Check that we are in output mode if we can */
> if (guard.gc->get_direction &&
> @@ -2790,8 +2796,9 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
> guard.gc->set(guard.gc, gpio_chip_hwgpio(desc), val);
> }
>
> - if (!ret)
> - set_bit(FLAG_IS_OUT, &desc->flags);
> + set_bit(FLAG_IS_OUT, &desc->flags);
> +
> +out_trace_value_and_direction:
> trace_gpio_value(desc_to_gpio(desc), 0, val);
> trace_gpio_direction(desc_to_gpio(desc), 0, ret);
> return ret;
> @@ -2816,10 +2823,11 @@ int gpiod_direction_output_raw(struct gpio_desc *desc, int value)
> VALIDATE_DESC(desc);
>
> ret = gpiod_direction_output_raw_commit(desc, value);
> - if (ret == 0)
> - 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;
> }
> EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
>
> @@ -2843,10 +2851,11 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
> VALIDATE_DESC(desc);
>
> ret = gpiod_direction_output_nonotify(desc, value);
> - if (ret == 0)
> - 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;
> }
> EXPORT_SYMBOL_GPL(gpiod_direction_output);
>
> @@ -2877,19 +2886,15 @@ int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
> if (!ret)
> goto set_output_value;
> /* Emulate open drain by not actively driving the line high */
> - if (value) {
> - ret = gpiod_direction_input_nonotify(desc);
> + if (value)
> goto set_output_flag;
> - }
> } else if (test_bit(FLAG_OPEN_SOURCE, &flags)) {
> ret = gpio_set_config(desc, PIN_CONFIG_DRIVE_OPEN_SOURCE);
> if (!ret)
> goto set_output_value;
> /* Emulate open source by not actively driving the line low */
> - if (!value) {
> - ret = gpiod_direction_input_nonotify(desc);
> + if (!value)
> goto set_output_flag;
> - }
> } else {
> gpio_set_config(desc, PIN_CONFIG_DRIVE_PUSH_PULL);
> }
> @@ -2901,15 +2906,17 @@ int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
> return gpiod_direction_output_raw_commit(desc, value);
>
> set_output_flag:
> + ret = gpiod_direction_input_nonotify(desc);
This can't be right. Or am I just not getting it? In any case it's
completely unreadable now. Go set output flag but start by setting
direction to input first?
> + if (ret)
> + return ret;
> /*
> * When emulating open-source or open-drain functionalities by not
> * actively driving the line (setting mode to input) we still need to
> * set the IS_OUT flag or otherwise we won't be able to set the line
> * value anymore.
> */
> - if (ret == 0)
> - set_bit(FLAG_IS_OUT, &desc->flags);
> - return ret;
> + set_bit(FLAG_IS_OUT, &desc->flags);
> + return 0;
> }
>
> /**
> @@ -2994,25 +3001,25 @@ int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
> VALIDATE_DESC(desc);
>
> 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.
> + 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;
> }
> EXPORT_SYMBOL_GPL(gpiod_set_config);
>
> @@ -3730,10 +3737,11 @@ int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
> VALIDATE_DESC(desc);
>
> ret = desc_set_label(desc, name);
> - if (ret == 0)
> - 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;
> }
> EXPORT_SYMBOL_GPL(gpiod_set_consumer_name);
>
> --
> 2.43.0.rc1.1336.g36b5255a03ac
>
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.
Bart
Powered by blists - more mailing lists