All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 1/4] gpio: pca953x: Synchronize interrupt handler properly
@ 2020-06-05 13:40 Andy Shevchenko
  2020-06-05 13:40 ` [PATCH v1 2/4] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2 Andy Shevchenko
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-06-05 13:40 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, linux-gpio
  Cc: Andy Shevchenko, Marek Vasut, Roland Stigge

Since the commit aa58a21ae378 ("gpio: pca953x: disable regmap locking")
the locking of regmap is disabled and that immediately introduces
a synchronization issue. It's easy to see when we try to monitor
more than one interrupt from the same chip.

It seems that the problem exists from the day one and even commit
6e20fb18054c ("drivers/gpio/pca953x.c: add a mutex to fix race condition")
missed this.

Below are the traces and shell reproducers before and after proposed change.
Note duplicates in the IRQ events. /proc/interrupts also shows a deviation,
i.e. sum of children interrupts higher than parent's one.

When locking is disabled for regmap and no protection in IRQ handler
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ...
 gpioset-194          regmap_hw_write_start: i2c-INT3491:02 reg=2 count=1
 irq/31-i2c-INT3-139  regmap_hw_read_start: i2c-INT3491:02 reg=4c count=2
 gpioset-194          regmap_hw_write_done: i2c-INT3491:02 reg=2 count=1
 gpioset-194          regmap_reg_read_cache: i2c-INT3491:02 reg=6 val=f5
 gpioset-194          regmap_reg_write: i2c-INT3491:02 reg=6 val=f5
 gpioset-194          regmap_hw_write_start: i2c-INT3491:02 reg=6 count=1
 irq/31-i2c-INT3-139  regmap_hw_read_done: i2c-INT3491:02 reg=4c count=2
 ...

 % gpiomon gpiochip3 0 &
 % gpioset gpiochip3 1=0
 % gpioset gpiochip3 1=1
 event:  RISING EDGE offset: 0 timestamp: [     302.782583765]
 % gpiomon gpiochip3 2 &
 % gpioset gpiochip3 1=0
 event:  RISING EDGE offset: 2 timestamp: [     312.033148829]
 event: FALLING EDGE offset: 0 timestamp: [     312.022757525]
 % gpioset gpiochip3 1=1
 event:  RISING EDGE offset: 2 timestamp: [     316.201148473]
 event:  RISING EDGE offset: 0 timestamp: [     316.191759599]

When locking is disabled for regmap and protection in IRQ handler
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ...
 gpioset-202          regmap_hw_write_start: i2c-INT3491:02 reg=2 count=1
 gpioset-202          regmap_hw_write_done: i2c-INT3491:02 reg=2 count=1
 gpioset-202          regmap_reg_read_cache: i2c-INT3491:02 reg=6 val=fd
 gpioset-202          regmap_reg_write: i2c-INT3491:02 reg=6 val=fd
 gpioset-202          regmap_hw_write_start: i2c-INT3491:02 reg=6 count=1
 gpioset-202          regmap_hw_write_done: i2c-INT3491:02 reg=6 count=1
 irq/31-i2c-INT3-139  regmap_hw_read_start: i2c-INT3491:02 reg=4c count=2
 irq/31-i2c-INT3-139  regmap_hw_read_done: i2c-INT3491:02 reg=4c count=2
 ...

 % gpiomon gpiochip3 0 &
 % gpioset gpiochip3 1=0
 event: FALLING EDGE offset: 0 timestamp: [     531.330078107]
 % gpioset gpiochip3 1=1
 event:  RISING EDGE offset: 0 timestamp: [     532.912239128]
 % gpiomon gpiochip3 2 &
 % gpioset gpiochip3 1=0
 event: FALLING EDGE offset: 0 timestamp: [     539.633669484]
 % gpioset gpiochip3 1=1
 event:  RISING EDGE offset: 0 timestamp: [     542.256978461]

Fixes: 6e20fb18054c ("drivers/gpio/pca953x.c: add a mutex to fix race condition")
Depends-on: 35d13d94893f ("gpio: pca953x: convert to use bitmap API")
Depends-on: 49427232764d ("gpio: pca953x: Perform basic regmap conversion")
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Roland Stigge <stigge@antcom.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 1fca8dd7824f..afe78639ec58 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -734,14 +734,16 @@ static irqreturn_t pca953x_irq_handler(int irq, void *devid)
 	struct gpio_chip *gc = &chip->gpio_chip;
 	DECLARE_BITMAP(pending, MAX_LINE);
 	int level;
+	bool ret;
 
-	if (!pca953x_irq_pending(chip, pending))
-		return IRQ_NONE;
+	mutex_lock(&chip->i2c_lock);
+	ret = pca953x_irq_pending(chip, pending);
+	mutex_unlock(&chip->i2c_lock);
 
 	for_each_set_bit(level, pending, gc->ngpio)
 		handle_nested_irq(irq_find_mapping(gc->irq.domain, level));
 
-	return IRQ_HANDLED;
+	return IRQ_RETVAL(ret);
 }
 
 static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
-- 
2.27.0.rc2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v1 2/4] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2
  2020-06-05 13:40 [PATCH v1 1/4] gpio: pca953x: Synchronize interrupt handler properly Andy Shevchenko
@ 2020-06-05 13:40 ` Andy Shevchenko
  2020-06-05 13:40 ` [PATCH v1 3/4] gpio: pca953x: Fix direction setting when configure an IRQ Andy Shevchenko
  2020-06-05 13:40 ` [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing Andy Shevchenko
  2 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-06-05 13:40 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, linux-gpio
  Cc: Andy Shevchenko, Mika Westerberg

ACPI table on Intel Galileo Gen 2 has wrong pin number for IRQ resource
of one of the I²C GPIO expanders. Since we know what that number is and
luckily have GPIO bases fixed for SoC's controllers, we may use a simple
DMI quirk to match the platform and retrieve GpioInt() pin on it for
the expander in question.

Mika suggested the way to avoid a quirk in the GPIO ACPI library and
here is the second, almost rewritten version of it.

Fixes: f32517bf1ae0 ("gpio: pca953x: support ACPI devices found on Galileo Gen2")
Depends-on: 25e3ef894eef ("gpio: acpi: Split out acpi_gpio_get_irq_resource() helper")
Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c | 79 +++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index afe78639ec58..4d3157d8b5cd 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -107,6 +107,79 @@ static const struct i2c_device_id pca953x_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, pca953x_id);
 
+#ifdef CONFIG_GPIO_PCA953X_IRQ
+
+#include <linux/dmi.h>
+#include <linux/gpio.h>
+#include <linux/list.h>
+
+static const struct dmi_system_id pca953x_dmi_acpi_irq_info[] = {
+	{
+		/*
+		 * On Intel Galileo Gen 2 board the IRQ pin of one of
+		 * the I²C GPIO expanders, which has GpioInt() resource,
+		 * is provided as an absolute number instead of being
+		 * relative. Since first controller (gpio-sch.c) and
+		 * second (gpio-dwapb.c) are at the fixed bases, we may
+		 * safely refer to the number in the global space to get
+		 * an IRQ out of it.
+		 */
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"),
+		},
+	},
+	{}
+};
+
+#ifdef CONFIG_ACPI
+static int pca953x_acpi_get_pin(struct acpi_resource *ares, void *data)
+{
+	struct acpi_resource_gpio *agpio;
+	int *pin = data;
+
+	if (acpi_gpio_get_irq_resource(ares, &agpio))
+		*pin = agpio->pin_table[0];
+	return 1;
+}
+
+static int pca953x_acpi_find_pin(struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	int pin = -ENOENT, ret;
+	LIST_HEAD(r);
+
+	ret = acpi_dev_get_resources(adev, &r, pca953x_acpi_get_pin, &pin);
+	acpi_dev_free_resource_list(&r);
+	if (ret < 0)
+		return ret;
+
+	return pin;
+}
+#else
+static inline int pca953x_acpi_find_pin(struct device *dev) { return -ENXIO; }
+#endif
+
+static int pca953x_acpi_get_irq(struct device *dev)
+{
+	int pin, ret;
+
+	pin = pca953x_acpi_find_pin(dev);
+	if (pin < 0)
+		return pin;
+
+	dev_info(dev, "Applying ACPI interrupt quirk (GPIO %d)\n", pin);
+
+	if (!gpio_is_valid(pin))
+		return -EINVAL;
+
+	ret = gpio_request(pin, "pca953x interrupt");
+	if (ret)
+		return ret;
+
+	return gpio_to_irq(pin);
+}
+#endif
+
 static const struct acpi_device_id pca953x_acpi_ids[] = {
 	{ "INT3491", 16 | PCA953X_TYPE | PCA_LATCH_INT, },
 	{ }
@@ -754,6 +827,12 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
 	DECLARE_BITMAP(irq_stat, MAX_LINE);
 	int ret;
 
+	if (dmi_first_match(pca953x_dmi_acpi_irq_info)) {
+		ret = pca953x_acpi_get_irq(&client->dev);
+		if (ret > 0)
+			client->irq = ret;
+	}
+
 	if (!client->irq)
 		return 0;
 
-- 
2.27.0.rc2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v1 3/4] gpio: pca953x: Fix direction setting when configure an IRQ
  2020-06-05 13:40 [PATCH v1 1/4] gpio: pca953x: Synchronize interrupt handler properly Andy Shevchenko
  2020-06-05 13:40 ` [PATCH v1 2/4] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2 Andy Shevchenko
@ 2020-06-05 13:40 ` Andy Shevchenko
  2020-06-05 13:40 ` [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing Andy Shevchenko
  2 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-06-05 13:40 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, linux-gpio
  Cc: Andy Shevchenko, Marek Vasut

The commit 0f25fda840a9 ("gpio: pca953x: Zap ad-hoc reg_direction cache")
seems inadvertently made a typo in pca953x_irq_bus_sync_unlock().

When the direction bit is 1 it means input, and the piece of code in question
was looking for output ones that should be turned to inputs.

Fix direction setting when configure an IRQ by injecting a bitmap complement
operation.

Fixes: 0f25fda840a9 ("gpio: pca953x: Zap ad-hoc reg_direction cache")
Depends-on: 35d13d94893f ("gpio: pca953x: convert to use bitmap API")
Cc: Marek Vasut <marek.vasut@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 4d3157d8b5cd..97c9ac31ecb5 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -696,8 +696,6 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
 	DECLARE_BITMAP(reg_direction, MAX_LINE);
 	int level;
 
-	pca953x_read_regs(chip, chip->regs->direction, reg_direction);
-
 	if (chip->driver_data & PCA_PCAL) {
 		/* Enable latch on interrupt-enabled inputs */
 		pca953x_write_regs(chip, PCAL953X_IN_LATCH, chip->irq_mask);
@@ -708,7 +706,11 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
 		pca953x_write_regs(chip, PCAL953X_INT_MASK, irq_mask);
 	}
 
+	/* Switch direction to input if needed */
+	pca953x_read_regs(chip, chip->regs->direction, reg_direction);
+
 	bitmap_or(irq_mask, chip->irq_trig_fall, chip->irq_trig_raise, gc->ngpio);
+	bitmap_complement(reg_direction, reg_direction, gc->ngpio);
 	bitmap_and(irq_mask, irq_mask, reg_direction, gc->ngpio);
 
 	/* Look for any newly setup interrupt */
-- 
2.27.0.rc2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing
  2020-06-05 13:40 [PATCH v1 1/4] gpio: pca953x: Synchronize interrupt handler properly Andy Shevchenko
  2020-06-05 13:40 ` [PATCH v1 2/4] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2 Andy Shevchenko
  2020-06-05 13:40 ` [PATCH v1 3/4] gpio: pca953x: Fix direction setting when configure an IRQ Andy Shevchenko
@ 2020-06-05 13:40 ` Andy Shevchenko
  2020-06-15 12:20   ` Uwe Kleine-König
  2 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-06-05 13:40 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, linux-gpio
  Cc: Andy Shevchenko, Uwe Kleine-König

It's a repetition of the commit aa58a21ae378
  ("gpio: pca953x: disable regmap locking")
which states the following:

  This driver uses its own locking but regmap silently uses
  a mutex for all operations too. Add the option to disable
  locking to the regmap config struct.

Fixes: bcf41dc480b1 ("gpio: pca953x: fix handling of automatic address incrementing")
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-pca953x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 97c9ac31ecb5..6f409ee0b033 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -395,6 +395,7 @@ static const struct regmap_config pca953x_ai_i2c_regmap = {
 	.writeable_reg = pca953x_writeable_register,
 	.volatile_reg = pca953x_volatile_register,
 
+	.disable_locking = true,
 	.cache_type = REGCACHE_RBTREE,
 	.max_register = 0x7f,
 };
-- 
2.27.0.rc2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing
  2020-06-05 13:40 ` [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing Andy Shevchenko
@ 2020-06-15 12:20   ` Uwe Kleine-König
  2020-06-15 12:53     ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Uwe Kleine-König @ 2020-06-15 12:20 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]

On Fri, Jun 05, 2020 at 04:40:36PM +0300, Andy Shevchenko wrote:
> It's a repetition of the commit aa58a21ae378
>   ("gpio: pca953x: disable regmap locking")
> which states the following:
> 
>   This driver uses its own locking but regmap silently uses
>   a mutex for all operations too. Add the option to disable
>   locking to the regmap config struct.
> 
> Fixes: bcf41dc480b1 ("gpio: pca953x: fix handling of automatic address incrementing")
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Ah, good catch. I assume that I didn't have aa58a21ae378 in my tree when
I created the patch that then became bcf41dc480b1.

Looks right

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing
  2020-06-15 12:20   ` Uwe Kleine-König
@ 2020-06-15 12:53     ` Andy Shevchenko
  2020-06-15 13:20       ` Uwe Kleine-König
  2020-06-16  9:24       ` Bartosz Golaszewski
  0 siblings, 2 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-06-15 12:53 UTC (permalink / raw)
  To: Uwe Kleine-König; +Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio

On Mon, Jun 15, 2020 at 02:20:44PM +0200, Uwe Kleine-König wrote:
> On Fri, Jun 05, 2020 at 04:40:36PM +0300, Andy Shevchenko wrote:
> > It's a repetition of the commit aa58a21ae378
> >   ("gpio: pca953x: disable regmap locking")
> > which states the following:
> > 
> >   This driver uses its own locking but regmap silently uses
> >   a mutex for all operations too. Add the option to disable
> >   locking to the regmap config struct.
> > 
> > Fixes: bcf41dc480b1 ("gpio: pca953x: fix handling of automatic address incrementing")
> > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Ah, good catch. I assume that I didn't have aa58a21ae378 in my tree when
> I created the patch that then became bcf41dc480b1.
> 
> Looks right
> 
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks!

Linus, Bart, just to clarify that this is material for one of the next v5.8-rcX
(this cycle!).

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing
  2020-06-15 12:53     ` Andy Shevchenko
@ 2020-06-15 13:20       ` Uwe Kleine-König
  2020-06-15 13:38         ` Andy Shevchenko
  2020-06-16  9:24       ` Bartosz Golaszewski
  1 sibling, 1 reply; 12+ messages in thread
From: Uwe Kleine-König @ 2020-06-15 13:20 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Geert Uytterhoeven

[-- Attachment #1: Type: text/plain, Size: 1733 bytes --]

Hello,

[adding Geert to Cc as he was involved with
aa58a21ae37894d456a2f91a37e9fd71ad4aa27e]

On Mon, Jun 15, 2020 at 03:53:49PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 15, 2020 at 02:20:44PM +0200, Uwe Kleine-König wrote:
> > On Fri, Jun 05, 2020 at 04:40:36PM +0300, Andy Shevchenko wrote:
> > > It's a repetition of the commit aa58a21ae378
> > >   ("gpio: pca953x: disable regmap locking")
> > > which states the following:
> > > 
> > >   This driver uses its own locking but regmap silently uses
> > >   a mutex for all operations too. Add the option to disable
> > >   locking to the regmap config struct.
> > > 
> > > Fixes: bcf41dc480b1 ("gpio: pca953x: fix handling of automatic address incrementing")
> > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > Ah, good catch. I assume that I didn't have aa58a21ae378 in my tree when
> > I created the patch that then became bcf41dc480b1.
> > 
> > Looks right
> > 
> > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Thanks!
> 
> Linus, Bart, just to clarify that this is material for one of the next v5.8-rcX
> (this cycle!).

I didn't test but I wonder if this patch is really urgent. Just from
looking I'd say two locks are not nice but also don't hurt much. If it
is more urgent the commit log should maybe mention how the driver is
broken without this change? (Also applies to
aa58a21ae37894d456a2f91a37e9fd71ad4aa27e of course (but too late).)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing
  2020-06-15 13:20       ` Uwe Kleine-König
@ 2020-06-15 13:38         ` Andy Shevchenko
  2020-06-15 14:18           ` Uwe Kleine-König
  0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-06-15 13:38 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Andy Shevchenko, Linus Walleij, Bartosz Golaszewski,
	open list:GPIO SUBSYSTEM, Geert Uytterhoeven

On Mon, Jun 15, 2020 at 4:23 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> [adding Geert to Cc as he was involved with
> aa58a21ae37894d456a2f91a37e9fd71ad4aa27e]
>
> On Mon, Jun 15, 2020 at 03:53:49PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 15, 2020 at 02:20:44PM +0200, Uwe Kleine-König wrote:
> > > On Fri, Jun 05, 2020 at 04:40:36PM +0300, Andy Shevchenko wrote:
> > > > It's a repetition of the commit aa58a21ae378
> > > >   ("gpio: pca953x: disable regmap locking")
> > > > which states the following:
> > > >
> > > >   This driver uses its own locking but regmap silently uses
> > > >   a mutex for all operations too. Add the option to disable
> > > >   locking to the regmap config struct.
> > > >
> > > > Fixes: bcf41dc480b1 ("gpio: pca953x: fix handling of automatic address incrementing")
> > > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > Ah, good catch. I assume that I didn't have aa58a21ae378 in my tree when
> > > I created the patch that then became bcf41dc480b1.
> > >
> > > Looks right
> > >
> > > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >
> > Thanks!
> >
> > Linus, Bart, just to clarify that this is material for one of the next v5.8-rcX
> > (this cycle!).
>
> I didn't test but I wonder if this patch is really urgent.

I'm talking about this entire fix-series.

> Just from
> looking I'd say two locks are not nice but also don't hurt much. If it
> is more urgent the commit log should maybe mention how the driver is
> broken without this change? (Also applies to
> aa58a21ae37894d456a2f91a37e9fd71ad4aa27e of course (but too late).)

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing
  2020-06-15 13:38         ` Andy Shevchenko
@ 2020-06-15 14:18           ` Uwe Kleine-König
  2020-06-15 15:12             ` Andy Shevchenko
  0 siblings, 1 reply; 12+ messages in thread
From: Uwe Kleine-König @ 2020-06-15 14:18 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Andy Shevchenko, Linus Walleij, Bartosz Golaszewski,
	open list:GPIO SUBSYSTEM, Geert Uytterhoeven

[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]

On Mon, Jun 15, 2020 at 04:38:23PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 15, 2020 at 4:23 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > [adding Geert to Cc as he was involved with
> > aa58a21ae37894d456a2f91a37e9fd71ad4aa27e]
> >
> > On Mon, Jun 15, 2020 at 03:53:49PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 15, 2020 at 02:20:44PM +0200, Uwe Kleine-König wrote:
> > > > On Fri, Jun 05, 2020 at 04:40:36PM +0300, Andy Shevchenko wrote:
> > > > > It's a repetition of the commit aa58a21ae378
> > > > >   ("gpio: pca953x: disable regmap locking")
> > > > > which states the following:
> > > > >
> > > > >   This driver uses its own locking but regmap silently uses
> > > > >   a mutex for all operations too. Add the option to disable
> > > > >   locking to the regmap config struct.
> > > > >
> > > > > Fixes: bcf41dc480b1 ("gpio: pca953x: fix handling of automatic address incrementing")
> > > > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > >
> > > > Ah, good catch. I assume that I didn't have aa58a21ae378 in my tree when
> > > > I created the patch that then became bcf41dc480b1.
> > > >
> > > > Looks right
> > > >
> > > > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > >
> > > Thanks!
> > >
> > > Linus, Bart, just to clarify that this is material for one of the next v5.8-rcX
> > > (this cycle!).
> >
> > I didn't test but I wonder if this patch is really urgent.
> 
> I'm talking about this entire fix-series.

Ah, I didn't notice this is a series as I only got patch 4.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing
  2020-06-15 14:18           ` Uwe Kleine-König
@ 2020-06-15 15:12             ` Andy Shevchenko
  0 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-06-15 15:12 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	Geert Uytterhoeven

On Mon, Jun 15, 2020 at 04:18:04PM +0200, Uwe Kleine-König wrote:
> On Mon, Jun 15, 2020 at 04:38:23PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 15, 2020 at 4:23 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Mon, Jun 15, 2020 at 03:53:49PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Jun 15, 2020 at 02:20:44PM +0200, Uwe Kleine-König wrote:
> > > > > On Fri, Jun 05, 2020 at 04:40:36PM +0300, Andy Shevchenko wrote:
> > > > > > It's a repetition of the commit aa58a21ae378
> > > > > >   ("gpio: pca953x: disable regmap locking")
> > > > > > which states the following:
> > > > > >
> > > > > >   This driver uses its own locking but regmap silently uses
> > > > > >   a mutex for all operations too. Add the option to disable
> > > > > >   locking to the regmap config struct.
> > > > > >
> > > > > > Fixes: bcf41dc480b1 ("gpio: pca953x: fix handling of automatic address incrementing")
> > > > > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > >
> > > > > Ah, good catch. I assume that I didn't have aa58a21ae378 in my tree when
> > > > > I created the patch that then became bcf41dc480b1.
> > > > >
> > > > > Looks right
> > > > >
> > > > > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > >
> > > > Thanks!
> > > >
> > > > Linus, Bart, just to clarify that this is material for one of the next v5.8-rcX
> > > > (this cycle!).
> > >
> > > I didn't test but I wonder if this patch is really urgent.
> > 
> > I'm talking about this entire fix-series.
> 
> Ah, I didn't notice this is a series as I only got patch 4.

In case you are curious / want to try:
https://lore.kernel.org/linux-gpio/20200605134036.9013-1-andriy.shevchenko@linux.intel.com/T/#m5e01e6462f2f72fbc4bdd3f71c330b7a2f75d5ba

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing
  2020-06-15 12:53     ` Andy Shevchenko
  2020-06-15 13:20       ` Uwe Kleine-König
@ 2020-06-16  9:24       ` Bartosz Golaszewski
  2020-06-16 12:53         ` Andy Shevchenko
  1 sibling, 1 reply; 12+ messages in thread
From: Bartosz Golaszewski @ 2020-06-16  9:24 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Uwe Kleine-König, Linus Walleij, linux-gpio

pon., 15 cze 2020 o 14:53 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Mon, Jun 15, 2020 at 02:20:44PM +0200, Uwe Kleine-König wrote:
> > On Fri, Jun 05, 2020 at 04:40:36PM +0300, Andy Shevchenko wrote:
> > > It's a repetition of the commit aa58a21ae378
> > >   ("gpio: pca953x: disable regmap locking")
> > > which states the following:
> > >
> > >   This driver uses its own locking but regmap silently uses
> > >   a mutex for all operations too. Add the option to disable
> > >   locking to the regmap config struct.
> > >
> > > Fixes: bcf41dc480b1 ("gpio: pca953x: fix handling of automatic address incrementing")
> > > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Ah, good catch. I assume that I didn't have aa58a21ae378 in my tree when
> > I created the patch that then became bcf41dc480b1.
> >
> > Looks right
> >
> > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> Thanks!
>
> Linus, Bart, just to clarify that this is material for one of the next v5.8-rcX
> (this cycle!).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

I applied the whole series for fixes. Thanks!

Bartosz

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing
  2020-06-16  9:24       ` Bartosz Golaszewski
@ 2020-06-16 12:53         ` Andy Shevchenko
  0 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-06-16 12:53 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Andy Shevchenko, Uwe Kleine-König, Linus Walleij, linux-gpio

On Tue, Jun 16, 2020 at 12:25 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> pon., 15 cze 2020 o 14:53 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> > On Mon, Jun 15, 2020 at 02:20:44PM +0200, Uwe Kleine-König wrote:
> > > On Fri, Jun 05, 2020 at 04:40:36PM +0300, Andy Shevchenko wrote:

...

> > > Looks right
> > >
> > > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >
> > Thanks!
> >
> > Linus, Bart, just to clarify that this is material for one of the next v5.8-rcX
> > (this cycle!).

> I applied the whole series for fixes. Thanks!

Thank you!

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-06-16 12:53 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-05 13:40 [PATCH v1 1/4] gpio: pca953x: Synchronize interrupt handler properly Andy Shevchenko
2020-06-05 13:40 ` [PATCH v1 2/4] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2 Andy Shevchenko
2020-06-05 13:40 ` [PATCH v1 3/4] gpio: pca953x: Fix direction setting when configure an IRQ Andy Shevchenko
2020-06-05 13:40 ` [PATCH v1 4/4] gpio: pca953x: disable regmap locking for automatic address incrementing Andy Shevchenko
2020-06-15 12:20   ` Uwe Kleine-König
2020-06-15 12:53     ` Andy Shevchenko
2020-06-15 13:20       ` Uwe Kleine-König
2020-06-15 13:38         ` Andy Shevchenko
2020-06-15 14:18           ` Uwe Kleine-König
2020-06-15 15:12             ` Andy Shevchenko
2020-06-16  9:24       ` Bartosz Golaszewski
2020-06-16 12:53         ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.