| Message ID | 20240130124828.14678-11-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: rework locking and object life-time control | expand |
On Tue, Jan 30, 2024 at 1:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > We now removed the gpio_lock spinlock and modified the places > previously protected by it to handle desc->flags access in a consistent > way. Let's improve other places that were previously unprotected by > reading the flags field of gpio_desc once and using the stored value for > logic consistency. If we need to modify the field, let's also write it > back once with a consistent value resulting from the function's logic. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> (...) I have a trouble with this one: gpiochip_find_base_unlocked() > + unsigned long flags; (...) > + flags = READ_ONCE(desc->flags); (...) > + if (test_bit(FLAG_OPEN_DRAIN, &flags) && > + test_bit(FLAG_IS_OUT, &flags)) > return 0; (...) > + assign_bit(FLAG_IS_OUT, &flags, !ret); > + WRITE_ONCE(desc->flags, flags); I unerstand the atomicity of each operation here, but ... if what you want to protect is modifications from other CPUs, how do we know that another CPU isn't coming in and reading and modifying and assigning another flag inbetween these operations while the value is only stored in the CPU-local flags variable? Same with gpiod_direction_output(). To me it seems like maybe you need to actually protect the desc->flags with the SRCU struct in these cases? (and not only use it for the label protection then). An alternative is maybe to rewrite the code with test_and_set(). But as you say it is currently unprotected, I just wonder if this really adds any protection. Yours, Linus Walleij
On Wed, Jan 31, 2024 at 9:01 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Jan 30, 2024 at 1:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > We now removed the gpio_lock spinlock and modified the places > > previously protected by it to handle desc->flags access in a consistent > > way. Let's improve other places that were previously unprotected by > > reading the flags field of gpio_desc once and using the stored value for > > logic consistency. If we need to modify the field, let's also write it > > back once with a consistent value resulting from the function's logic. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > (...) > > I have a trouble with this one: > > gpiochip_find_base_unlocked() > > + unsigned long flags; > (...) > > + flags = READ_ONCE(desc->flags); > (...) > > + if (test_bit(FLAG_OPEN_DRAIN, &flags) && > > + test_bit(FLAG_IS_OUT, &flags)) > > return 0; > (...) > > + assign_bit(FLAG_IS_OUT, &flags, !ret); > > + WRITE_ONCE(desc->flags, flags); > > I unerstand the atomicity of each operation here, but ... if what you want > to protect is modifications from other CPUs, how do we know that another > CPU isn't coming in and reading and modifying and assigning > another flag inbetween these operations while the value is only > stored in the CPU-local flags variable? > > Same with gpiod_direction_output(). > > To me it seems like maybe you need to actually protect the desc->flags > with the SRCU struct in these cases? (and not only use it for the > label protection then). > > An alternative is maybe to rewrite the code with test_and_set(). > > But as you say it is currently unprotected, I just wonder if this really > adds any protection. After re-reading the cover letter I'm fine with this, but I still wonder if it buys us anything. Maybe some words looped back from the commit message that we are not really protecting the callbacks because access is [predominantly] exclusive? Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, Jan 31, 2024 at 9:35 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, Jan 31, 2024 at 9:01 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Jan 30, 2024 at 1:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > We now removed the gpio_lock spinlock and modified the places > > > previously protected by it to handle desc->flags access in a consistent > > > way. Let's improve other places that were previously unprotected by > > > reading the flags field of gpio_desc once and using the stored value for > > > logic consistency. If we need to modify the field, let's also write it > > > back once with a consistent value resulting from the function's logic. > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > (...) > > > > I have a trouble with this one: > > > > gpiochip_find_base_unlocked() > > > + unsigned long flags; > > (...) > > > + flags = READ_ONCE(desc->flags); > > (...) > > > + if (test_bit(FLAG_OPEN_DRAIN, &flags) && > > > + test_bit(FLAG_IS_OUT, &flags)) > > > return 0; > > (...) > > > + assign_bit(FLAG_IS_OUT, &flags, !ret); > > > + WRITE_ONCE(desc->flags, flags); > > > > I unerstand the atomicity of each operation here, but ... if what you want > > to protect is modifications from other CPUs, how do we know that another > > CPU isn't coming in and reading and modifying and assigning > > another flag inbetween these operations while the value is only > > stored in the CPU-local flags variable? > > > > Same with gpiod_direction_output(). > > > > To me it seems like maybe you need to actually protect the desc->flags > > with the SRCU struct in these cases? (and not only use it for the > > label protection then). > > > > An alternative is maybe to rewrite the code with test_and_set(). > > > > But as you say it is currently unprotected, I just wonder if this really > > adds any protection. > > After re-reading the cover letter I'm fine with this, but I still wonder > if it buys us anything. > This was a tough one... I don't really see any way around it. SRCU is for pointers but even then - we wouldn't get with SRCU anything more than what we're getting with atomic reads and writes. As neither sleeping nor atomic locks will work in the case of the GPIO subsystem, I figured that we should strive for the maximum of coherence we can achieve - and for that I figured that we should read the flags once, do our thing and then write back a consistent result. If someone else comes around at the same time and writes something else - well, he better be an *exclusive* user of that GPIO and know what they're doing. :) Anyway, I think this series is already a big step forward and should at least protect us from crashing. We can continue the work on achieving full state consistency later. Bart > Maybe some words looped back from the > commit message that we are not really protecting the callbacks > because access is [predominantly] exclusive? > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Yours, > Linus Walleij
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 2a7439db7392..f15b854bbcb2 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -336,18 +336,20 @@ static int gpiochip_find_base_unlocked(int ngpio) int gpiod_get_direction(struct gpio_desc *desc) { struct gpio_chip *gc; + unsigned long flags; unsigned int offset; int ret; gc = gpiod_to_chip(desc); offset = gpio_chip_hwgpio(desc); + flags = READ_ONCE(desc->flags); /* * Open drain emulation using input mode may incorrectly report * input here, fix that up. */ - if (test_bit(FLAG_OPEN_DRAIN, &desc->flags) && - test_bit(FLAG_IS_OUT, &desc->flags)) + if (test_bit(FLAG_OPEN_DRAIN, &flags) && + test_bit(FLAG_IS_OUT, &flags)) return 0; if (!gc->get_direction) @@ -361,7 +363,8 @@ int gpiod_get_direction(struct gpio_desc *desc) if (ret > 0) ret = 1; - assign_bit(FLAG_IS_OUT, &desc->flags, !ret); + assign_bit(FLAG_IS_OUT, &flags, !ret); + WRITE_ONCE(desc->flags, flags); return ret; } @@ -747,9 +750,6 @@ static void gpiochip_machine_hog(struct gpio_chip *gc, struct gpiod_hog *hog) return; } - if (test_bit(FLAG_IS_HOGGED, &desc->flags)) - return; - rv = gpiod_hog(desc, hog->line_name, hog->lflags, hog->dflags); if (rv) gpiod_err(desc, "%s: unable to hog GPIO line (%s:%u): %d\n", @@ -2519,13 +2519,16 @@ static int gpio_set_config(struct gpio_desc *desc, enum pin_config_param mode) static int gpio_set_bias(struct gpio_desc *desc) { enum pin_config_param bias; + unsigned long flags; unsigned int arg; - if (test_bit(FLAG_BIAS_DISABLE, &desc->flags)) + flags = READ_ONCE(desc->flags); + + if (test_bit(FLAG_BIAS_DISABLE, &flags)) bias = PIN_CONFIG_BIAS_DISABLE; - else if (test_bit(FLAG_PULL_UP, &desc->flags)) + else if (test_bit(FLAG_PULL_UP, &flags)) bias = PIN_CONFIG_BIAS_PULL_UP; - else if (test_bit(FLAG_PULL_DOWN, &desc->flags)) + else if (test_bit(FLAG_PULL_DOWN, &flags)) bias = PIN_CONFIG_BIAS_PULL_DOWN; else return 0; @@ -2691,24 +2694,28 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output_raw); */ int gpiod_direction_output(struct gpio_desc *desc, int value) { + unsigned long flags; int ret; VALIDATE_DESC(desc); - if (test_bit(FLAG_ACTIVE_LOW, &desc->flags)) + + flags = READ_ONCE(desc->flags); + + if (test_bit(FLAG_ACTIVE_LOW, &flags)) value = !value; else value = !!value; /* GPIOs used for enabled IRQs shall not be set as output */ - if (test_bit(FLAG_USED_AS_IRQ, &desc->flags) && - test_bit(FLAG_IRQ_IS_ENABLED, &desc->flags)) { + if (test_bit(FLAG_USED_AS_IRQ, &flags) && + test_bit(FLAG_IRQ_IS_ENABLED, &flags)) { gpiod_err(desc, "%s: tried to set a GPIO tied to an IRQ as output\n", __func__); return -EIO; } - if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) { + if (test_bit(FLAG_OPEN_DRAIN, &flags)) { /* First see if we can enable open drain in hardware */ ret = gpio_set_config(desc, PIN_CONFIG_DRIVE_OPEN_DRAIN); if (!ret) @@ -2718,7 +2725,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value) ret = gpiod_direction_input(desc); goto set_output_flag; } - } else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) { + } else if (test_bit(FLAG_OPEN_SOURCE, &flags)) { ret = gpio_set_config(desc, PIN_CONFIG_DRIVE_OPEN_SOURCE); if (!ret) goto set_output_value; @@ -4411,21 +4418,22 @@ int gpiod_hog(struct gpio_desc *desc, const char *name, int hwnum; int ret; + if (test_and_set_bit(FLAG_IS_HOGGED, &desc->flags)) + return 0; + gc = gpiod_to_chip(desc); hwnum = gpio_chip_hwgpio(desc); local_desc = gpiochip_request_own_desc(gc, hwnum, name, lflags, dflags); if (IS_ERR(local_desc)) { + clear_bit(FLAG_IS_HOGGED, &desc->flags); ret = PTR_ERR(local_desc); pr_err("requesting hog GPIO %s (chip %s, offset %d) failed, %d\n", name, gc->label, hwnum, ret); return ret; } - /* Mark GPIO as hogged so it can be identified and removed later */ - set_bit(FLAG_IS_HOGGED, &desc->flags); - gpiod_dbg(desc, "hogged as %s%s\n", (dflags & GPIOD_FLAGS_BIT_DIR_OUT) ? "output" : "input", (dflags & GPIOD_FLAGS_BIT_DIR_OUT) ?