| Message ID | 20181108165255.9940-3-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: mockup: misc updates | expand |
On Thu, Nov 08, 2018 at 05:52:54PM +0100, Bartosz Golaszewski wrote: > While no user reported any race condition problems with gpio-mockup, > let's be on the safe side and use a mutex when performing any changes > on the dummy chip structures. > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > --- > drivers/gpio/gpio-mockup.c | 50 ++++++++++++++++++++++++++++++++------ > 1 file changed, 43 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index 6a50f9f59c90..3cd92912c414 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -54,6 +54,7 @@ struct gpio_mockup_chip { > struct gpio_mockup_line_status *lines; > struct irq_sim irqsim; > struct dentry *dbg_dir; > + struct mutex lock; > }; > > struct gpio_mockup_dbgfs_private { > @@ -82,29 +83,53 @@ static int gpio_mockup_range_ngpio(unsigned int index) > return gpio_mockup_ranges[index * 2 + 1]; > } > > -static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset) > +static int __gpio_mockup_get(struct gpio_chip *gc, unsigned int offset) > { > struct gpio_mockup_chip *chip = gpiochip_get_data(gc); > > return chip->lines[offset].value; > } > > -static void gpio_mockup_set(struct gpio_chip *gc, > - unsigned int offset, int value) > +static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset) > +{ > + struct gpio_mockup_chip *chip = gpiochip_get_data(gc); > + int val; > + > + mutex_lock(&chip->lock); > + val = __gpio_mockup_get(gc, offset); > + mutex_unlock(&chip->lock); > + > + return val; > +} I think this function doesn't need locking. I returns a single value and if there is a race and some other process currently changes the value it matters little if the return value is zero or not. But even with this kept unchanged the patch looks good. Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks Uwe
On Thu, Nov 8, 2018 at 5:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > While no user reported any race condition problems with gpio-mockup, > let's be on the safe side and use a mutex when performing any changes > on the dummy chip structures. > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> I tried to apply this but it failed, does it require patch 1? I can pull in the next -rc after I merged the fix in that case and we can apply on top. __gpio_* I tend to dislike __underscore_notation because I feel it is semantically ambguous. I prefer a proper name, even to the point that I prefer inner_function_foo over __foo, but it's your driver and I might be a bit grumpy. :) Yours, Linus Walleij
pt., 16 lis 2018 o 22:43 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > On Thu, Nov 8, 2018 at 5:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > While no user reported any race condition problems with gpio-mockup, > > let's be on the safe side and use a mutex when performing any changes > > on the dummy chip structures. > > > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > > I tried to apply this but it failed, does it require patch 1? > Yes, because of the change in get_direction(). > I can pull in the next -rc after I merged the fix in that case > and we can apply on top. > This is fine, it's aimed for 4.21 anyway. > __gpio_* > I tend to dislike __underscore_notation because I feel it > is semantically ambguous. I prefer a proper name, even > to the point that I prefer inner_function_foo over __foo, > but it's your driver and I might be a bit grumpy. :) > I think this is a common and intuitive pattern in the kernel codebase. Many subsystems and drivers use '__' to mark functions that execute internal logic and expect certain locks to be held etc. If you don't mind, I'd like to leave it like this. Bart
On Mon, Nov 19, 2018 at 10:09 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > pt., 16 lis 2018 o 22:43 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > __gpio_* > > I tend to dislike __underscore_notation because I feel it > > is semantically ambguous. I prefer a proper name, even > > to the point that I prefer inner_function_foo over __foo, > > but it's your driver and I might be a bit grumpy. :) > > I think this is a common and intuitive pattern in the kernel codebase. > Many subsystems and drivers use '__' to mark functions that execute > internal logic and expect certain locks to be held etc. You say it yourself: interpretation depends on context. I might be especially stupid for being unable to discern meaning from context in these cases and so what is intuitive for some is just not intuitive for me. Example: set_bit() vs __set_bit() Apparently some kernel developers think it is completely obvious that the latter is the unlocked non-atomic version of set_bit(). However I was confused for years with no idea as to what the difference was. Had it simply been named set_bit_nonatomic(), at the cost of a few characters, confusion on my part would be avoided and at least to me the world would be a better place. > If you don't mind, I'd like to leave it like this. No big deal, keep it as is :) Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c index 6a50f9f59c90..3cd92912c414 100644 --- a/drivers/gpio/gpio-mockup.c +++ b/drivers/gpio/gpio-mockup.c @@ -54,6 +54,7 @@ struct gpio_mockup_chip { struct gpio_mockup_line_status *lines; struct irq_sim irqsim; struct dentry *dbg_dir; + struct mutex lock; }; struct gpio_mockup_dbgfs_private { @@ -82,29 +83,53 @@ static int gpio_mockup_range_ngpio(unsigned int index) return gpio_mockup_ranges[index * 2 + 1]; } -static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset) +static int __gpio_mockup_get(struct gpio_chip *gc, unsigned int offset) { struct gpio_mockup_chip *chip = gpiochip_get_data(gc); return chip->lines[offset].value; } -static void gpio_mockup_set(struct gpio_chip *gc, - unsigned int offset, int value) +static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset) +{ + struct gpio_mockup_chip *chip = gpiochip_get_data(gc); + int val; + + mutex_lock(&chip->lock); + val = __gpio_mockup_get(gc, offset); + mutex_unlock(&chip->lock); + + return val; +} + +static void __gpio_mockup_set(struct gpio_chip *gc, + unsigned int offset, int value) { struct gpio_mockup_chip *chip = gpiochip_get_data(gc); chip->lines[offset].value = !!value; } +static void gpio_mockup_set(struct gpio_chip *gc, + unsigned int offset, int value) +{ + struct gpio_mockup_chip *chip = gpiochip_get_data(gc); + + mutex_lock(&chip->lock); + __gpio_mockup_set(gc, offset, value); + mutex_unlock(&chip->lock); +} + static void gpio_mockup_set_multiple(struct gpio_chip *gc, unsigned long *mask, unsigned long *bits) { + struct gpio_mockup_chip *chip = gpiochip_get_data(gc); unsigned int bit; + mutex_lock(&chip->lock); for_each_set_bit(bit, mask, gc->ngpio) - gpio_mockup_set(gc, bit, test_bit(bit, bits)); - + __gpio_mockup_set(gc, bit, test_bit(bit, bits)); + mutex_unlock(&chip->lock); } static int gpio_mockup_dirout(struct gpio_chip *gc, @@ -112,8 +137,10 @@ static int gpio_mockup_dirout(struct gpio_chip *gc, { struct gpio_mockup_chip *chip = gpiochip_get_data(gc); - gpio_mockup_set(gc, offset, value); + mutex_lock(&chip->lock); + __gpio_mockup_set(gc, offset, value); chip->lines[offset].dir = GPIO_MOCKUP_DIR_OUT; + mutex_unlock(&chip->lock); return 0; } @@ -122,7 +149,9 @@ static int gpio_mockup_dirin(struct gpio_chip *gc, unsigned int offset) { struct gpio_mockup_chip *chip = gpiochip_get_data(gc); + mutex_lock(&chip->lock); chip->lines[offset].dir = GPIO_MOCKUP_DIR_IN; + mutex_unlock(&chip->lock); return 0; } @@ -130,8 +159,13 @@ static int gpio_mockup_dirin(struct gpio_chip *gc, unsigned int offset) static int gpio_mockup_get_direction(struct gpio_chip *gc, unsigned int offset) { struct gpio_mockup_chip *chip = gpiochip_get_data(gc); + int direction; - return !chip->lines[offset].dir; + mutex_lock(&chip->lock); + direction = !chip->lines[offset].dir; + mutex_unlock(&chip->lock); + + return direction; } static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset) @@ -283,6 +317,8 @@ static int gpio_mockup_probe(struct platform_device *pdev) return -ENOMEM; } + mutex_init(&chip->lock); + gc = &chip->gc; gc->base = base; gc->ngpio = ngpio;
While no user reported any race condition problems with gpio-mockup, let's be on the safe side and use a mutex when performing any changes on the dummy chip structures. Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> --- drivers/gpio/gpio-mockup.c | 50 ++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 7 deletions(-)