[2/3] gpio: mockup: add locking

Message ID 20181108165255.9940-3-brgl@bgdev.pl
State New
Headers show
Series
  • gpio: mockup: misc updates
Related show

Commit Message

Bartosz Golaszewski Nov. 8, 2018, 4:52 p.m.
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(-)

Comments

Uwe Kleine-König Nov. 8, 2018, 9:13 p.m. | #1
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
Linus Walleij Nov. 16, 2018, 9:43 p.m. | #2
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
Bartosz Golaszewski Nov. 19, 2018, 9:09 a.m. | #3
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
Linus Walleij Nov. 20, 2018, 8:46 a.m. | #4
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

Patch

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;