gpio-f7188x: Fix concurrent GPIO accesses (and minor improvements)

gpio-f7188x: Fix concurrent GPIO accesses (and minor improvements)

[Vincent Pelletier]

Hello,

Making intensive use of the GPIOs on this SuperIO chip, I triggered a race
condition causing apparently the wrong GPIO pins to be modified (and likely
read) randomly.
For some reason (unclear to me, I'm not used at all to kernel API),
request_muxed_region/release_region as it is used in this driver is not
sufficient to serialize accesses.
Also, reading other drivers for the same chip, I see a different pattern:
rather than accessing GPIO registers through the common IO region, this
region is only used for device discovery. From there, per-function base
address is retrieved, and that IO region gets requested once.

So I fixed the concurrent access by:
- using the same access pattern as in other drivers
- adding mutexes around IO region accesses

Once this was done and I got a bit more comfortable with this driver, chip and
gpiolib, I continued and fixed 3 minor issues/lacks.

Regards,
--
Vincent Pelletier

[1/4] gpio: gpio-f7188x: Use mutex for access serialisation.

[Vincent Pelletier]

For some (unclear to me) reason, superio_enter/superio_exit, although
calling request_muxed_region/release_region (respectively), are not
sufficient on an multi-processor system to guarantee serialised access,
leading to the following errors:
[ 1210.586990] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1211.227414] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1211.867890] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1212.508299] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1213.148734] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1213.789172] Trying to free nonexistent resource <000000000000002e-000000000000002f>
[ 1214.429607] Trying to free nonexistent resource <000000000000002e-000000000000002f>

Disabling all but one core make the error disappear, and re-enabling it
make it reappear.

Loaded modules for this SuperIO chip on this system are:
  8250_fintek (not actively used)
  fintek_cir (not actively used)
  f71882fg (polled every 2 seconds from userland)
  gpio_f7188x

Loaded modules using GPIO functionality:
  gpio_keys_polled (20ms polling period)
  leds_gpio (usb-host and 500ms timer triggers)

This change replaces frequent superio_enter/superio_exit and accesses via the
common GPIO IO region (0x2e..0x2f or 0x4e..0x4f) with mutex locking and
accesses via GPIO-dedicated IO region (pre-configured on chip).

Code inspired from hwmon/f71882fg .

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
 drivers/gpio/gpio-f7188x.c | 126 ++++++++++++++++++++++++++++-----------------
 1 file changed, 80 insertions(+), 46 deletions(-)

diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
index dbda843..2915f8d 100644
--- a/drivers/gpio/gpio-f7188x.c
+++ b/drivers/gpio/gpio-f7188x.c
@@ -16,6 +16,8 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
+#include <linux/mutex.h>
+#include <linux/acpi.h>
 
 #define DRVNAME "gpio-f7188x"
 
@@ -26,11 +28,17 @@
 #define SIO_DEVID		0x20	/* Device ID (2 bytes) */
 #define SIO_DEVREV		0x22	/* Device revision */
 #define SIO_MANID		0x23	/* Fintek ID (2 bytes) */
+#define SIO_ENABLE		0x30	/* Logical device enable */
+#define SIO_ADDR		0x60	/* Logical device address (2 bytes) */
 
 #define SIO_LD_GPIO		0x06	/* GPIO logical device */
 #define SIO_UNLOCK_KEY		0x87	/* Key to enable Super-I/O */
 #define SIO_LOCK_KEY		0xAA	/* Key to disable Super-I/O */
 
+#define REGION_LENGTH		8
+#define ADDR_REG_OFFSET		5
+#define DATA_REG_OFFSET		6
+
 #define SIO_FINTEK_ID		0x1934	/* Manufacturer ID */
 #define SIO_F71869_ID		0x0814	/* F71869 chipset ID */
 #define SIO_F71869A_ID		0x1007	/* F71869A chipset ID */
@@ -47,8 +55,9 @@ static const char * const f7188x_names[] = {
 };
 
 struct f7188x_sio {
-	int addr;
+	u16 addr;
 	enum chips type;
+	struct mutex lock;
 };
 
 struct f7188x_gpio_bank {
@@ -118,6 +127,22 @@ static inline void superio_exit(int base)
 	release_region(base, 2);
 }
 
+static u8 f7188x_read8(struct f7188x_sio *data, u8 reg)
+{
+	u8 val;
+
+	outb(reg, data->addr + ADDR_REG_OFFSET);
+	val = inb(data->addr + DATA_REG_OFFSET);
+
+	return val;
+}
+
+static void f7188x_write8(struct f7188x_sio *data, u8 reg, u8 val)
+{
+	outb(reg, data->addr + ADDR_REG_OFFSET);
+	outb(val, data->addr + DATA_REG_OFFSET);
+}
+
 /*
  * GPIO chip.
  */
@@ -192,47 +217,35 @@ static struct f7188x_gpio_bank f71889_gpio_bank[] = {
 
 static int f7188x_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
 {
-	int err;
 	struct f7188x_gpio_bank *bank =
 		container_of(chip, struct f7188x_gpio_bank, chip);
 	struct f7188x_sio *sio = bank->data->sio;
 	u8 dir;
 
-	err = superio_enter(sio->addr);
-	if (err)
-		return err;
-	superio_select(sio->addr, SIO_LD_GPIO);
-
-	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
+	mutex_lock(&sio->lock);
+	dir = f7188x_read8(sio, gpio_dir(bank->regbase));
 	dir &= ~(1 << offset);
-	superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
-
-	superio_exit(sio->addr);
+	f7188x_write8(sio, gpio_dir(bank->regbase), dir);
+	mutex_unlock(&sio->lock);
 
 	return 0;
 }
 
 static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
-	int err;
 	struct f7188x_gpio_bank *bank =
 		container_of(chip, struct f7188x_gpio_bank, chip);
 	struct f7188x_sio *sio = bank->data->sio;
 	u8 dir, data;
 
-	err = superio_enter(sio->addr);
-	if (err)
-		return err;
-	superio_select(sio->addr, SIO_LD_GPIO);
-
-	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
+	mutex_lock(&sio->lock);
+	dir = f7188x_read8(sio, gpio_dir(bank->regbase));
 	dir = !!(dir & (1 << offset));
 	if (dir)
-		data = superio_inb(sio->addr, gpio_data_out(bank->regbase));
+		data = f7188x_read8(sio, gpio_data_out(bank->regbase));
 	else
-		data = superio_inb(sio->addr, gpio_data_in(bank->regbase));
-
-	superio_exit(sio->addr);
+		data = f7188x_read8(sio, gpio_data_in(bank->regbase));
+	mutex_unlock(&sio->lock);
 
 	return !!(data & 1 << offset);
 }
@@ -240,54 +253,42 @@ static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset)
 static int f7188x_gpio_direction_out(struct gpio_chip *chip,
 				     unsigned offset, int value)
 {
-	int err;
 	struct f7188x_gpio_bank *bank =
 		container_of(chip, struct f7188x_gpio_bank, chip);
 	struct f7188x_sio *sio = bank->data->sio;
 	u8 dir, data_out;
 
-	err = superio_enter(sio->addr);
-	if (err)
-		return err;
-	superio_select(sio->addr, SIO_LD_GPIO);
-
-	data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase));
+	mutex_lock(&sio->lock);
+	data_out = f7188x_read8(sio, gpio_data_out(bank->regbase));
 	if (value)
 		data_out |= (1 << offset);
 	else
 		data_out &= ~(1 << offset);
-	superio_outb(sio->addr, gpio_data_out(bank->regbase), data_out);
+	f7188x_write8(sio, gpio_data_out(bank->regbase), data_out);
 
-	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
+	dir = f7188x_read8(sio, gpio_dir(bank->regbase));
 	dir |= (1 << offset);
-	superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
-
-	superio_exit(sio->addr);
+	f7188x_write8(sio, gpio_dir(bank->regbase), dir);
+	mutex_unlock(&sio->lock);
 
 	return 0;
 }
 
 static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 {
-	int err;
 	struct f7188x_gpio_bank *bank =
 		container_of(chip, struct f7188x_gpio_bank, chip);
 	struct f7188x_sio *sio = bank->data->sio;
 	u8 data_out;
 
-	err = superio_enter(sio->addr);
-	if (err)
-		return;
-	superio_select(sio->addr, SIO_LD_GPIO);
-
-	data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase));
+	mutex_lock(&sio->lock);
+	data_out = f7188x_read8(sio, gpio_data_out(bank->regbase));
 	if (value)
 		data_out |= (1 << offset);
 	else
 		data_out &= ~(1 << offset);
-	superio_outb(sio->addr, gpio_data_out(bank->regbase), data_out);
-
-	superio_exit(sio->addr);
+	f7188x_write8(sio, gpio_data_out(bank->regbase), data_out);
+	mutex_unlock(&sio->lock);
 }
 
 /*
@@ -326,6 +327,7 @@ static int f7188x_gpio_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 	data->sio = sio;
+	mutex_init(&sio->lock);
 
 	platform_set_drvdata(pdev, data);
 
@@ -372,6 +374,7 @@ static int f7188x_gpio_remove(struct platform_device *pdev)
 static int __init f7188x_find(int addr, struct f7188x_sio *sio)
 {
 	int err;
+	u16 logical_device_address;
 	u16 devid;
 
 	err = superio_enter(addr);
@@ -403,12 +406,25 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio)
 		pr_info(DRVNAME ": Unsupported Fintek device 0x%04x\n", devid);
 		goto err;
 	}
-	sio->addr = addr;
+	superio_select(addr, SIO_LD_GPIO);
+
+	if (!(superio_inb(addr, SIO_ENABLE) & 0x01)) {
+		pr_warn("Device not activated\n");
+		goto err;
+	}
+
+	logical_device_address = superio_inw(addr, SIO_ADDR);
+	if (logical_device_address == 0) {
+		pr_warn("Base address not set\n");
+		goto err;
+	}
+	logical_device_address &= ~(REGION_LENGTH - 1);	/* Ignore 3 LSB */
+	sio->addr = logical_device_address;
 	err = 0;
 
 	pr_info(DRVNAME ": Found %s at %#x, revision %d\n",
 		f7188x_names[sio->type],
-		(unsigned int) addr,
+		(unsigned int) logical_device_address,
 		(int) superio_inb(addr, SIO_DEVREV));
 
 err:
@@ -421,12 +437,28 @@ static struct platform_device *f7188x_gpio_pdev;
 static int __init
 f7188x_gpio_device_add(const struct f7188x_sio *sio)
 {
+	struct resource res = {
+		.start	= sio->addr,
+		.end	= sio->addr + REGION_LENGTH - 1,
+		.flags	= IORESOURCE_IO,
+	};
 	int err;
 
 	f7188x_gpio_pdev = platform_device_alloc(DRVNAME, -1);
 	if (!f7188x_gpio_pdev)
 		return -ENOMEM;
 
+	res.name = f7188x_gpio_pdev->name;
+	err = acpi_check_resource_conflict(&res);
+	if (err)
+		goto err;
+
+	err = platform_device_add_resources(f7188x_gpio_pdev, &res, 1);
+	if (err) {
+		pr_err("Device resource addition failed\n");
+		goto err;
+	}
+
 	err = platform_device_add_data(f7188x_gpio_pdev,
 				       sio, sizeof(*sio));
 	if (err) {
@@ -467,6 +499,8 @@ static int __init f7188x_gpio_init(void)
 	int err;
 	struct f7188x_sio sio;
 
+	memset(&sio, 0, sizeof(sio));
+
 	if (f7188x_find(0x2e, &sio) &&
 	    f7188x_find(0x4e, &sio))
 		return -ENODEV;
-- 
2.5.0

[2/4] gpio: gpio-f7188x: GPIO bank 0 bit 0 is not available on f71869a

[Vincent Pelletier]

In version 0.19P (2011/10) of the datasheet, GPIO00 is marked as available
on the global register map (page 52), but marked as reserved in detailed
register map (page 112).

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
 drivers/gpio/gpio-f7188x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
index 2915f8d..9a56afe 100644
--- a/drivers/gpio/gpio-f7188x.c
+++ b/drivers/gpio/gpio-f7188x.c
@@ -186,7 +186,7 @@ static struct f7188x_gpio_bank f71869_gpio_bank[] = {
 };
 
 static struct f7188x_gpio_bank f71869a_gpio_bank[] = {
-	F7188X_GPIO_BANK(0, 6, 0xF0),
+	F7188X_GPIO_BANK(1, 5, 0xF0),
 	F7188X_GPIO_BANK(10, 8, 0xE0),
 	F7188X_GPIO_BANK(20, 8, 0xD0),
 	F7188X_GPIO_BANK(30, 8, 0xC0),
-- 
2.5.0

[3/4] gpio: gpio-f7188x: "get" should retrieve sensed level when available.

[Vincent Pelletier]

As of struct gpio_chip "get" member documentation:
 * @get: returns value for signal "offset"; for output signals this
 *      returns either the value actually sensed, or zero
As in this chip, read and driven values are controlled by distinct
registers, unconditionally expose sensed values.

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
 drivers/gpio/gpio-f7188x.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
index 9a56afe..d59cccd 100644
--- a/drivers/gpio/gpio-f7188x.c
+++ b/drivers/gpio/gpio-f7188x.c
@@ -236,15 +236,10 @@ static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset)
 	struct f7188x_gpio_bank *bank =
 		container_of(chip, struct f7188x_gpio_bank, chip);
 	struct f7188x_sio *sio = bank->data->sio;
-	u8 dir, data;
+	u8 data;
 
 	mutex_lock(&sio->lock);
-	dir = f7188x_read8(sio, gpio_dir(bank->regbase));
-	dir = !!(dir & (1 << offset));
-	if (dir)
-		data = f7188x_read8(sio, gpio_data_out(bank->regbase));
-	else
-		data = f7188x_read8(sio, gpio_data_in(bank->regbase));
+	data = f7188x_read8(sio, gpio_data_in(bank->regbase));
 	mutex_unlock(&sio->lock);
 
 	return !!(data & 1 << offset);
-- 
2.5.0

[4/4] gpio: gpio-f7188x: Implement get_direction.

[Vincent Pelletier]

Avoids gpiolib assumptions on initial pin direction, allowing user to observe
power-on settings.
---
 drivers/gpio/gpio-f7188x.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
index d59cccd..ec121a3 100644
--- a/drivers/gpio/gpio-f7188x.c
+++ b/drivers/gpio/gpio-f7188x.c
@@ -147,6 +147,7 @@ static void f7188x_write8(struct f7188x_sio *data, u8 reg, u8 val)
  * GPIO chip.
  */
 
+static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset);
 static int f7188x_gpio_direction_in(struct gpio_chip *chip, unsigned offset);
 static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset);
 static int f7188x_gpio_direction_out(struct gpio_chip *chip,
@@ -158,6 +159,7 @@ static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value);
 		.chip = {						\
 			.label            = DRVNAME,			\
 			.owner            = THIS_MODULE,		\
+			.get_direction    = f7188x_gpio_get_direction,	\
 			.direction_input  = f7188x_gpio_direction_in,	\
 			.get              = f7188x_gpio_get,		\
 			.direction_output = f7188x_gpio_direction_out,	\
@@ -215,6 +217,20 @@ static struct f7188x_gpio_bank f71889_gpio_bank[] = {
 	F7188X_GPIO_BANK(70, 8, 0x80),
 };
 
+static int f7188x_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
+{
+	struct f7188x_gpio_bank *bank =
+		container_of(chip, struct f7188x_gpio_bank, chip);
+	struct f7188x_sio *sio = bank->data->sio;
+	u8 dir;
+
+	mutex_lock(&sio->lock);
+	dir = f7188x_read8(sio, gpio_dir(bank->regbase));
+	mutex_unlock(&sio->lock);
+
+	return !(dir & 1 << offset);
+}
+
 static int f7188x_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
 {
 	struct f7188x_gpio_bank *bank =
-- 
2.5.0

Re: [1/4] gpio: gpio-f7188x: Use mutex for access serialisation.

[Simon Guinot]

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

On Thu, Aug 20, 2015 at 08:03:26PM +0200, Vincent Pelletier wrote:

Hi Vincent,

This patch breaks GPIO support for the Super-I/Os f71882fg and f71889f.

> For some (unclear to me) reason, superio_enter/superio_exit, although
> calling request_muxed_region/release_region (respectively), are not
> sufficient on an multi-processor system to guarantee serialised access,
> leading to the following errors:
> [ 1210.586990] Trying to free nonexistent resource <000000000000002e-000000000000002f>
> [ 1211.227414] Trying to free nonexistent resource <000000000000002e-000000000000002f>
> [ 1211.867890] Trying to free nonexistent resource <000000000000002e-000000000000002f>
> [ 1212.508299] Trying to free nonexistent resource <000000000000002e-000000000000002f>
> [ 1213.148734] Trying to free nonexistent resource <000000000000002e-000000000000002f>
> [ 1213.789172] Trying to free nonexistent resource <000000000000002e-000000000000002f>
> [ 1214.429607] Trying to free nonexistent resource <000000000000002e-000000000000002f>

I am quite surprised by this warnings. Although I have been using this
driver intensively with the f71882fg and f71889f Super-I/Os on a large
range of boards, I have never experimented such warnings.

IMHO, understand *clearly* the issue could be a good start in order to
fix it efficiently.

Please, could you describe a setup (as simple as possible) allowing to
reproduce the issue ? I'll try it on my side.

> 
> Disabling all but one core make the error disappear, and re-enabling it
> make it reappear.
> 
> Loaded modules for this SuperIO chip on this system are:
>   8250_fintek (not actively used)
>   fintek_cir (not actively used)
>   f71882fg (polled every 2 seconds from userland)
>   gpio_f7188x
> 
> Loaded modules using GPIO functionality:
>   gpio_keys_polled (20ms polling period)
>   leds_gpio (usb-host and 500ms timer triggers)

Please try to make the module list needed to reproduce the issue as
short as possible. Ideally only gpio_f7188x would be needed.

> 
> This change replaces frequent superio_enter/superio_exit and accesses via the
> common GPIO IO region (0x2e..0x2f or 0x4e..0x4f) with mutex locking and
> accesses via GPIO-dedicated IO region (pre-configured on chip).

Unfortunately the f71882fg and f71889f Super-I/Os don't provide an
I/O region dedicated to GPIOs. If it was the case, I would have used
this way. But for this Super-I/O models, the GPIOs have to be configured
through the global registers. That's why your patch breaks support with
this models. However, accordingly to the datasheet, this feature seems
to be supported by the f71869a model.

Thanks,

Simon

> 
> Code inspired from hwmon/f71882fg .
> 
> Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
> ---
>  drivers/gpio/gpio-f7188x.c | 126 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 80 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-f7188x.c b/drivers/gpio/gpio-f7188x.c
> index dbda843..2915f8d 100644
> --- a/drivers/gpio/gpio-f7188x.c
> +++ b/drivers/gpio/gpio-f7188x.c
> @@ -16,6 +16,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/gpio.h>
> +#include <linux/mutex.h>
> +#include <linux/acpi.h>
>  
>  #define DRVNAME "gpio-f7188x"
>  
> @@ -26,11 +28,17 @@
>  #define SIO_DEVID		0x20	/* Device ID (2 bytes) */
>  #define SIO_DEVREV		0x22	/* Device revision */
>  #define SIO_MANID		0x23	/* Fintek ID (2 bytes) */
> +#define SIO_ENABLE		0x30	/* Logical device enable */
> +#define SIO_ADDR		0x60	/* Logical device address (2 bytes) */
>  
>  #define SIO_LD_GPIO		0x06	/* GPIO logical device */
>  #define SIO_UNLOCK_KEY		0x87	/* Key to enable Super-I/O */
>  #define SIO_LOCK_KEY		0xAA	/* Key to disable Super-I/O */
>  
> +#define REGION_LENGTH		8
> +#define ADDR_REG_OFFSET		5
> +#define DATA_REG_OFFSET		6
> +
>  #define SIO_FINTEK_ID		0x1934	/* Manufacturer ID */
>  #define SIO_F71869_ID		0x0814	/* F71869 chipset ID */
>  #define SIO_F71869A_ID		0x1007	/* F71869A chipset ID */
> @@ -47,8 +55,9 @@ static const char * const f7188x_names[] = {
>  };
>  
>  struct f7188x_sio {
> -	int addr;
> +	u16 addr;
>  	enum chips type;
> +	struct mutex lock;
>  };
>  
>  struct f7188x_gpio_bank {
> @@ -118,6 +127,22 @@ static inline void superio_exit(int base)
>  	release_region(base, 2);
>  }
>  
> +static u8 f7188x_read8(struct f7188x_sio *data, u8 reg)
> +{
> +	u8 val;
> +
> +	outb(reg, data->addr + ADDR_REG_OFFSET);
> +	val = inb(data->addr + DATA_REG_OFFSET);
> +
> +	return val;
> +}
> +
> +static void f7188x_write8(struct f7188x_sio *data, u8 reg, u8 val)
> +{
> +	outb(reg, data->addr + ADDR_REG_OFFSET);
> +	outb(val, data->addr + DATA_REG_OFFSET);
> +}
> +
>  /*
>   * GPIO chip.
>   */
> @@ -192,47 +217,35 @@ static struct f7188x_gpio_bank f71889_gpio_bank[] = {
>  
>  static int f7188x_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
>  {
> -	int err;
>  	struct f7188x_gpio_bank *bank =
>  		container_of(chip, struct f7188x_gpio_bank, chip);
>  	struct f7188x_sio *sio = bank->data->sio;
>  	u8 dir;
>  
> -	err = superio_enter(sio->addr);
> -	if (err)
> -		return err;
> -	superio_select(sio->addr, SIO_LD_GPIO);
> -
> -	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
> +	mutex_lock(&sio->lock);
> +	dir = f7188x_read8(sio, gpio_dir(bank->regbase));
>  	dir &= ~(1 << offset);
> -	superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
> -
> -	superio_exit(sio->addr);
> +	f7188x_write8(sio, gpio_dir(bank->regbase), dir);
> +	mutex_unlock(&sio->lock);
>  
>  	return 0;
>  }
>  
>  static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset)
>  {
> -	int err;
>  	struct f7188x_gpio_bank *bank =
>  		container_of(chip, struct f7188x_gpio_bank, chip);
>  	struct f7188x_sio *sio = bank->data->sio;
>  	u8 dir, data;
>  
> -	err = superio_enter(sio->addr);
> -	if (err)
> -		return err;
> -	superio_select(sio->addr, SIO_LD_GPIO);
> -
> -	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
> +	mutex_lock(&sio->lock);
> +	dir = f7188x_read8(sio, gpio_dir(bank->regbase));
>  	dir = !!(dir & (1 << offset));
>  	if (dir)
> -		data = superio_inb(sio->addr, gpio_data_out(bank->regbase));
> +		data = f7188x_read8(sio, gpio_data_out(bank->regbase));
>  	else
> -		data = superio_inb(sio->addr, gpio_data_in(bank->regbase));
> -
> -	superio_exit(sio->addr);
> +		data = f7188x_read8(sio, gpio_data_in(bank->regbase));
> +	mutex_unlock(&sio->lock);
>  
>  	return !!(data & 1 << offset);
>  }
> @@ -240,54 +253,42 @@ static int f7188x_gpio_get(struct gpio_chip *chip, unsigned offset)
>  static int f7188x_gpio_direction_out(struct gpio_chip *chip,
>  				     unsigned offset, int value)
>  {
> -	int err;
>  	struct f7188x_gpio_bank *bank =
>  		container_of(chip, struct f7188x_gpio_bank, chip);
>  	struct f7188x_sio *sio = bank->data->sio;
>  	u8 dir, data_out;
>  
> -	err = superio_enter(sio->addr);
> -	if (err)
> -		return err;
> -	superio_select(sio->addr, SIO_LD_GPIO);
> -
> -	data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase));
> +	mutex_lock(&sio->lock);
> +	data_out = f7188x_read8(sio, gpio_data_out(bank->regbase));
>  	if (value)
>  		data_out |= (1 << offset);
>  	else
>  		data_out &= ~(1 << offset);
> -	superio_outb(sio->addr, gpio_data_out(bank->regbase), data_out);
> +	f7188x_write8(sio, gpio_data_out(bank->regbase), data_out);
>  
> -	dir = superio_inb(sio->addr, gpio_dir(bank->regbase));
> +	dir = f7188x_read8(sio, gpio_dir(bank->regbase));
>  	dir |= (1 << offset);
> -	superio_outb(sio->addr, gpio_dir(bank->regbase), dir);
> -
> -	superio_exit(sio->addr);
> +	f7188x_write8(sio, gpio_dir(bank->regbase), dir);
> +	mutex_unlock(&sio->lock);
>  
>  	return 0;
>  }
>  
>  static void f7188x_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>  {
> -	int err;
>  	struct f7188x_gpio_bank *bank =
>  		container_of(chip, struct f7188x_gpio_bank, chip);
>  	struct f7188x_sio *sio = bank->data->sio;
>  	u8 data_out;
>  
> -	err = superio_enter(sio->addr);
> -	if (err)
> -		return;
> -	superio_select(sio->addr, SIO_LD_GPIO);
> -
> -	data_out = superio_inb(sio->addr, gpio_data_out(bank->regbase));
> +	mutex_lock(&sio->lock);
> +	data_out = f7188x_read8(sio, gpio_data_out(bank->regbase));
>  	if (value)
>  		data_out |= (1 << offset);
>  	else
>  		data_out &= ~(1 << offset);
> -	superio_outb(sio->addr, gpio_data_out(bank->regbase), data_out);
> -
> -	superio_exit(sio->addr);
> +	f7188x_write8(sio, gpio_data_out(bank->regbase), data_out);
> +	mutex_unlock(&sio->lock);
>  }
>  
>  /*
> @@ -326,6 +327,7 @@ static int f7188x_gpio_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  	data->sio = sio;
> +	mutex_init(&sio->lock);
>  
>  	platform_set_drvdata(pdev, data);
>  
> @@ -372,6 +374,7 @@ static int f7188x_gpio_remove(struct platform_device *pdev)
>  static int __init f7188x_find(int addr, struct f7188x_sio *sio)
>  {
>  	int err;
> +	u16 logical_device_address;
>  	u16 devid;
>  
>  	err = superio_enter(addr);
> @@ -403,12 +406,25 @@ static int __init f7188x_find(int addr, struct f7188x_sio *sio)
>  		pr_info(DRVNAME ": Unsupported Fintek device 0x%04x\n", devid);
>  		goto err;
>  	}
> -	sio->addr = addr;
> +	superio_select(addr, SIO_LD_GPIO);
> +
> +	if (!(superio_inb(addr, SIO_ENABLE) & 0x01)) {
> +		pr_warn("Device not activated\n");
> +		goto err;
> +	}
> +
> +	logical_device_address = superio_inw(addr, SIO_ADDR);
> +	if (logical_device_address == 0) {
> +		pr_warn("Base address not set\n");
> +		goto err;
> +	}
> +	logical_device_address &= ~(REGION_LENGTH - 1);	/* Ignore 3 LSB */
> +	sio->addr = logical_device_address;
>  	err = 0;
>  
>  	pr_info(DRVNAME ": Found %s at %#x, revision %d\n",
>  		f7188x_names[sio->type],
> -		(unsigned int) addr,
> +		(unsigned int) logical_device_address,
>  		(int) superio_inb(addr, SIO_DEVREV));
>  
>  err:
> @@ -421,12 +437,28 @@ static struct platform_device *f7188x_gpio_pdev;
>  static int __init
>  f7188x_gpio_device_add(const struct f7188x_sio *sio)
>  {
> +	struct resource res = {
> +		.start	= sio->addr,
> +		.end	= sio->addr + REGION_LENGTH - 1,
> +		.flags	= IORESOURCE_IO,
> +	};
>  	int err;
>  
>  	f7188x_gpio_pdev = platform_device_alloc(DRVNAME, -1);
>  	if (!f7188x_gpio_pdev)
>  		return -ENOMEM;
>  
> +	res.name = f7188x_gpio_pdev->name;
> +	err = acpi_check_resource_conflict(&res);
> +	if (err)
> +		goto err;
> +
> +	err = platform_device_add_resources(f7188x_gpio_pdev, &res, 1);
> +	if (err) {
> +		pr_err("Device resource addition failed\n");
> +		goto err;
> +	}
> +
>  	err = platform_device_add_data(f7188x_gpio_pdev,
>  				       sio, sizeof(*sio));
>  	if (err) {
> @@ -467,6 +499,8 @@ static int __init f7188x_gpio_init(void)
>  	int err;
>  	struct f7188x_sio sio;
>  
> +	memset(&sio, 0, sizeof(sio));
> +
>  	if (f7188x_find(0x2e, &sio) &&
>  	    f7188x_find(0x4e, &sio))
>  		return -ENODEV;
> -- 
> 2.5.0

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [1/4] gpio: gpio-f7188x: Use mutex for access serialisation.

[Vincent Pelletier]

Hello Simon,

Thanks for reviewing my patch.

On Fri, 21 Aug 2015 19:52:16 +0200, Simon Guinot
<simon.guinot@sequanux.org> wrote:
> IMHO, understand *clearly* the issue could be a good start in order to
> fix it efficiently.

I totally agree, but have no idea how to debug further.
Could you suggest any mechanism to debug *_resource ?

> Please, could you describe a setup (as simple as possible) allowing to
> reproduce the issue ? I'll try it on my side.
[...]
> Please try to make the module list needed to reproduce the issue as
> short as possible. Ideally only gpio_f7188x would be needed.

The simplest I could find so far needs gpio-input-polled with 20ms
polling period (I didn't try to change polling period), and a shell loop
writing to gpioXX/value.

I'm using the following platform driver to declare a single
gpio-input-polled key:
  https://github.com/vpelletier/linux/blob/free_nonexistent_resource/drivers/platform/x86/qnap-tsx51.c
Full version of this driver:
  https://github.com/vpelletier/linux/blob/ts651/drivers/platform/x86/qnap-tsx51.c
  (ignore code surrounded with '#if QNAP_TSX51_GPIOD', it is
  still broken).

It should be easy to create a variant fitting another board, as long
as you can recycle two GPIOs.

Once I've unloaded all other listed drivers for this chip and loaded
the stripped-down qnap-tsx51 version:
Terminal 0:
  echo 0 > /sys/devices/system/cpu/cpu1/online
Terminal 1:
  cd /sys/class/gpio/
  echo 62 > export
  cd gpio62
  echo "out" > direction
  while :; echo 0 > value; echo 1 > value; done
No error so far.
Terminal 1:
  echo 1 > /sys/devices/cpu/cpu1/online
After a few (10 to 30) seconds, tty is flooded with the error I
reported.

Stopping the loop stop the error.
I have seen rare cases where a few extra messages could occur within
the next minute or so, but this was before I stripped most out of the
platform driver.

With just two such loops poking at (out) GPIOs, it does not happen.

I'm testing this on v4.1.4 with the following patch applied (the error
happened before and after that patch, so it likely does not matter):
  https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7e08117de6ee17ae6c8f2983999a98cb95eb9bc2

CPU is an intel celeron J1800, dual-core at 2.41GHz.
Exact board is this machine's motherboard:
  https://www.qnap.com/i/fr/product/model.php?II=144

> Unfortunately the f71882fg and f71889f Super-I/Os don't provide an
> I/O region dedicated to GPIOs. If it was the case, I would have used
> this way. But for this Super-I/O models, the GPIOs have to be configured
> through the global registers. That's why your patch breaks support with
> this models.

Wow, I didn't expect such difference between models otherwise handled
the same way in hwmon/f71882fg.c . I guess the GPIO function works
differently from hwmon function in these models (hwmon still having
dedicated IO range, not GPIO).

Regards,
-- 
Vincent Pelletier

Re: [1/4] gpio: gpio-f7188x: Use mutex for access serialisation.

[Vincent Pelletier]

On Fri, 21 Aug 2015 22:48:24 +0200, Vincent Pelletier
<plr.vincent@gmail.com> wrote:
> With just two such loops poking at (out) GPIOs, it does not happen.

Re-reading my mail, I realise this was not clear: this is without
input-gpio-polled but just two shell loops switching a GPIO each.

Regards,
-- 
Vincent Pelletier

Re: [1/4] gpio: gpio-f7188x: Use mutex for access serialisation.

[Vincent Pelletier]

Hi,

On Fri, 21 Aug 2015 22:48:24 +0200, Vincent Pelletier
<plr.vincent@gmail.com> wrote:
> On Fri, 21 Aug 2015 19:52:16 +0200, Simon Guinot
> <simon.guinot@sequanux.org> wrote:
> > Please, could you describe a setup (as simple as possible) allowing to
> > reproduce the issue ? I'll try it on my side.
> [...]
> > Please try to make the module list needed to reproduce the issue as
> > short as possible. Ideally only gpio_f7188x would be needed.
> 
> The simplest I could find so far needs gpio-input-polled with 20ms
> polling period (I didn't try to change polling period), and a shell loop
> writing to gpioXX/value.

Have you had a chance to reproduce this issue ?
Can I do something to help ?

Regards,
-- 
Vincent Pelletier

Re: [1/4] gpio: gpio-f7188x: Use mutex for access serialisation.

[Simon Guinot]

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

On Thu, Sep 03, 2015 at 08:05:40PM +0200, Vincent Pelletier wrote:
> Hi,
> 
> On Fri, 21 Aug 2015 22:48:24 +0200, Vincent Pelletier
> <plr.vincent@gmail.com> wrote:
> > On Fri, 21 Aug 2015 19:52:16 +0200, Simon Guinot
> > <simon.guinot@sequanux.org> wrote:
> > > Please, could you describe a setup (as simple as possible) allowing to
> > > reproduce the issue ? I'll try it on my side.
> > [...]
> > > Please try to make the module list needed to reproduce the issue as
> > > short as possible. Ideally only gpio_f7188x would be needed.
> > 
> > The simplest I could find so far needs gpio-input-polled with 20ms
> > polling period (I didn't try to change polling period), and a shell loop
> > writing to gpioXX/value.
> 
> Have you had a chance to reproduce this issue ?
> Can I do something to help ?

Still not but it is on my todo list.

Regards,

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [1/4] gpio: gpio-f7188x: Use mutex for access serialisation.

[Simon Guinot]

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

On Thu, Sep 03, 2015 at 08:05:40PM +0200, Vincent Pelletier wrote:
> Hi,
> 
> On Fri, 21 Aug 2015 22:48:24 +0200, Vincent Pelletier
> <plr.vincent@gmail.com> wrote:
> > On Fri, 21 Aug 2015 19:52:16 +0200, Simon Guinot
> > <simon.guinot@sequanux.org> wrote:
> > > Please, could you describe a setup (as simple as possible) allowing to
> > > reproduce the issue ? I'll try it on my side.
> > [...]
> > > Please try to make the module list needed to reproduce the issue as
> > > short as possible. Ideally only gpio_f7188x would be needed.
> > 
> > The simplest I could find so far needs gpio-input-polled with 20ms
> > polling period (I didn't try to change polling period), and a shell loop
> > writing to gpioXX/value.
> 
> Have you had a chance to reproduce this issue ?
> Can I do something to help ?

Hi Vincent,

Vincent (Donnefort) finally succeeds to reproduce the issue. The setup
is quite simple. You only have to flood the gpio-f7188x driver via the
sysfs GPIO interface. Nothing more is needed.

After some debugging we discovered that the problem comes from the
__request_region function which don't handle very well concurrent
requests on a muxed region.

I will send a patch as a reply to this email. Please, can you test it ?

Thanks,

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Simon Guinot]

In __request_region, if a conflict with a BUSY and MUXED resource is
detected, then the caller goes to sleep and waits for the resource to
be released. A pointer on the conflicting resource is kept. At wake-up
this pointer is used as a parent to retry to request the region. A first
problem is that this pointer might well be invalid (if for example the
conflicting resource have already been freed). An another problem is
that the next call to __request_region() fails to detect a remaining
conflict. The previously conflicting resource is passed as a parameter
and __request_region() will look for a conflict among the children of
this resource and not at the resource itself. It is likely to succeed
anyway, even if there is still a conflict. Instead, the parent of the
conflicting resource should be passed to __request_region().

As a fix attempt, this patch don't update the parent resource pointer in
the case we have to wait for a muxed region right after.

Reported-by: Vincent Pelletier <plr.vincent@gmail.com>
Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
Tested-by: Vincent Donnefort <vdonnefort@gmail.com>
---
 kernel/resource.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index fed052a1bc9f..b8c84804db6a 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1072,9 +1072,10 @@ struct resource * __request_region(struct resource *parent,
 		if (!conflict)
 			break;
 		if (conflict != parent) {
-			parent = conflict;
-			if (!(conflict->flags & IORESOURCE_BUSY))
+			if (!(conflict->flags & IORESOURCE_BUSY)) {
+				parent = conflict;
 				continue;
+			}
 		}
 		if (conflict->flags & flags & IORESOURCE_MUXED) {
 			add_wait_queue(&muxed_resource_wait, &wait);
-- 
2.1.4

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Vincent Pelletier]

Hello,

I finally got around to rebasing some patches, and realised that the
patch from Simon Guinot below still gets rebased over torvalds' v4.4 .

Any reason it was not applied ?
Or was the issue fixed in another, non-git-conflicting way ? (I see
nothing recent in git log kernel/resource.c)

I do not find a trace of a mail confirming that I tested it and that it
fixes the issue. So here goes:
Tested-by: Vincent Pelletier <plr.vincent@gmail.com>

Testing details: bug reproduced on 4.1, patch applied over 4.1 and bug
disappeared. After rebasing this patch (along with others) over 4.4,
bug does not reappear. I did not try to reproduce bug with 4.4, but if
preferred I can give it a go.

On Thu, 10 Sep 2015 00:15:18 +0200, Simon Guinot
<simon.guinot@sequanux.org> wrote:
> In __request_region, if a conflict with a BUSY and MUXED resource is
> detected, then the caller goes to sleep and waits for the resource to
> be released. A pointer on the conflicting resource is kept. At wake-up
> this pointer is used as a parent to retry to request the region. A first
> problem is that this pointer might well be invalid (if for example the
> conflicting resource have already been freed). An another problem is
> that the next call to __request_region() fails to detect a remaining
> conflict. The previously conflicting resource is passed as a parameter
> and __request_region() will look for a conflict among the children of
> this resource and not at the resource itself. It is likely to succeed
> anyway, even if there is still a conflict. Instead, the parent of the
> conflicting resource should be passed to __request_region().
> 
> As a fix attempt, this patch don't update the parent resource pointer in
> the case we have to wait for a muxed region right after.
> 
> Reported-by: Vincent Pelletier <plr.vincent@gmail.com>
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> Tested-by: Vincent Donnefort <vdonnefort@gmail.com>
> ---
>  kernel/resource.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index fed052a1bc9f..b8c84804db6a 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1072,9 +1072,10 @@ struct resource * __request_region(struct resource *parent,
>  		if (!conflict)
>  			break;
>  		if (conflict != parent) {
> -			parent = conflict;
> -			if (!(conflict->flags & IORESOURCE_BUSY))
> +			if (!(conflict->flags & IORESOURCE_BUSY)) {
> +				parent = conflict;
>  				continue;
> +			}
>  		}
>  		if (conflict->flags & flags & IORESOURCE_MUXED) {
>  			add_wait_queue(&muxed_resource_wait, &wait);

Regards,
-- 
Vincent Pelletier

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Jesse Barnes]

+Linus (the de-facto resource guy).

On 02/19/2016 01:10 PM, Vincent Pelletier wrote:
> Hello,
> 
> I finally got around to rebasing some patches, and realised that the
> patch from Simon Guinot below still gets rebased over torvalds' v4.4 .
> 
> Any reason it was not applied ?
> Or was the issue fixed in another, non-git-conflicting way ? (I see
> nothing recent in git log kernel/resource.c)
> 
> I do not find a trace of a mail confirming that I tested it and that it
> fixes the issue. So here goes:
> Tested-by: Vincent Pelletier <plr.vincent@gmail.com>
> 
> Testing details: bug reproduced on 4.1, patch applied over 4.1 and bug
> disappeared. After rebasing this patch (along with others) over 4.4,
> bug does not reappear. I did not try to reproduce bug with 4.4, but if
> preferred I can give it a go.
> 
> On Thu, 10 Sep 2015 00:15:18 +0200, Simon Guinot
> <simon.guinot@sequanux.org> wrote:
>> In __request_region, if a conflict with a BUSY and MUXED resource is
>> detected, then the caller goes to sleep and waits for the resource to
>> be released. A pointer on the conflicting resource is kept. At wake-up
>> this pointer is used as a parent to retry to request the region. A first
>> problem is that this pointer might well be invalid (if for example the
>> conflicting resource have already been freed). An another problem is
>> that the next call to __request_region() fails to detect a remaining
>> conflict. The previously conflicting resource is passed as a parameter
>> and __request_region() will look for a conflict among the children of
>> this resource and not at the resource itself. It is likely to succeed
>> anyway, even if there is still a conflict. Instead, the parent of the
>> conflicting resource should be passed to __request_region().
>>
>> As a fix attempt, this patch don't update the parent resource pointer in
>> the case we have to wait for a muxed region right after.
>>
>> Reported-by: Vincent Pelletier <plr.vincent@gmail.com>
>> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
>> Tested-by: Vincent Donnefort <vdonnefort@gmail.com>
>> ---
>>  kernel/resource.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index fed052a1bc9f..b8c84804db6a 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -1072,9 +1072,10 @@ struct resource * __request_region(struct resource *parent,
>>  		if (!conflict)
>>  			break;
>>  		if (conflict != parent) {
>> -			parent = conflict;
>> -			if (!(conflict->flags & IORESOURCE_BUSY))
>> +			if (!(conflict->flags & IORESOURCE_BUSY)) {
>> +				parent = conflict;
>>  				continue;
>> +			}
>>  		}
>>  		if (conflict->flags & flags & IORESOURCE_MUXED) {
>>  			add_wait_queue(&muxed_resource_wait, &wait);
> 
> Regards,
> 

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Linus Torvalds]

On Fri, Feb 19, 2016 at 3:25 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> +Linus (the de-facto resource guy).
>
> On 02/19/2016 01:10 PM, Vincent Pelletier wrote:
>>
>> I finally got around to rebasing some patches, and realised that the
>> patch from Simon Guinot below still gets rebased over torvalds' v4.4 .
>>
>> Any reason it was not applied ?
>> Or was the issue fixed in another, non-git-conflicting way ? (I see
>> nothing recent in git log kernel/resource.c)
>>
>> I do not find a trace of a mail confirming that I tested it and that it
>> fixes the issue. So here goes:
>> Tested-by: Vincent Pelletier <plr.vincent@gmail.com>

Hmm.

So I'm not entirely happy with the patch, because I think the problem
with using a possibly free'd parent resource at restart still exists.

As far as I can tell, if we hit the IORESOURCE_MUXED case *after* we
have successfully delved into a resource that wasn't busy, we will
have updated "parent" in a previous iteration of the loop, and we'll
not use the original parent when we then re-start after the sleep. So
quite frankly, I suspect any user of MUXED memory regions is still
fundamentally buggy, and IORESOURCE_MUXED has always been a hacky and
broken thing.

That said, I ended up applying the patch anyway, even if I despise it.
For all I know, muxed users never end up having those non-busy
sub-resources in practice, and maybe there is some serialization at
the top level for the drivers that use it. So if testing has shown
that it helps some actual case, I'll believe the testing. But the code
still looks rather debatable, and the whole IORESOURCE_MUXED approach
looks broken.

Jesse, that came in through you and the drm tree, I think. Alan is
marked as author, and there are other people who actually use and can
test the code. Can you guys think about the code a bit more.

I'm wondering if the *real* fix ends up being to reset the 'parent'
pointer to the original top-level parent after the sleep?

To recap: the patch is in my tree, but I'm not all that happy about it.

                    Linus

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Jesse Barnes]

On February 20, 2016 9:12:01 AM Linus Torvalds 
<torvalds@linux-foundation.org> wrote:

> On Fri, Feb 19, 2016 at 3:25 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> +Linus (the de-facto resource guy).
>>
>> On 02/19/2016 01:10 PM, Vincent Pelletier wrote:
>>> Tested-by: Vincent Pelletier <plr.vincent@gmail.com>
>
> Hmm.
>
> So I'm not entirely happy with the patch, because I think the problem
> with using a possibly free'd parent resource at restart still exists.
>
> As far as I can tell, if we hit the IORESOURCE_MUXED case *after* we
> have successfully delved into a resource that wasn't busy, we will
> have updated "parent" in a previous iteration of the loop, and we'll
> not use the original parent when we then re-start after the sleep. So
> quite frankly, I suspect any user of MUXED memory regions is still
> fundamentally buggy, and IORESOURCE_MUXED has always been a hacky and
> broken thing.
>
> That said, I ended up applying the patch anyway, even if I despise it.
> For all I know, muxed users never end up having those non-busy
> sub-resources in practice, and maybe there is some serialization at
> the top level for the drivers that use it. So if testing has shown
> that it helps some actual case, I'll believe the testing. But the code
> still looks rather debatable, and the whole IORESOURCE_MUXED approach
> looks broken.
>
> Jesse, that came in through you and the drm tree, I think. Alan is
> marked as author, and there are other people who actually use and can
> test the code. Can you guys think about the code a bit more.
>
> I'm wondering if the *real* fix ends up being to reset the 'parent'
> pointer to the original top-level parent after the sleep?
>
> To recap: the patch is in my tree, but I'm not all that happy about it.

Thanks, yeah i think testing wins in this case.  I'll revisit the muxed 
stuff; I do
remember being dubious at the time, but iirc Alan needed it for something,
and others had been pushing for these sorts of usages for awhile even though
we have some good alternatives in the form of bus and platform drivers that
can manage the appropriate serialization and keep things from stomping
on one another.

(And sorry if this message comes across in some bullshit format, I'm trying
out a new ChromeOS based mail client for fun here.)

Thanks,
Jesse

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Jesse Barnes]

On February 20, 2016 9:12:01 AM Linus Torvalds 
<torvalds@linux-foundation.org> wrote:

> On Fri, Feb 19, 2016 at 3:25 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> +Linus (the de-facto resource guy).
>>
>> On 02/19/2016 01:10 PM, Vincent Pelletier wrote:
>>> Tested-by: Vincent Pelletier <plr.vincent@gmail.com>
>
> Hmm.
>
> So I'm not entirely happy with the patch, because I think the problem
> with using a possibly free'd parent resource at restart still exists.
>
> As far as I can tell, if we hit the IORESOURCE_MUXED case *after* we
> have successfully delved into a resource that wasn't busy, we will
> have updated "parent" in a previous iteration of the loop, and we'll
> not use the original parent when we then re-start after the sleep. So
> quite frankly, I suspect any user of MUXED memory regions is still
> fundamentally buggy, and IORESOURCE_MUXED has always been a hacky and
> broken thing.
>
> That said, I ended up applying the patch anyway, even if I despise it.
> For all I know, muxed users never end up having those non-busy
> sub-resources in practice, and maybe there is some serialization at
> the top level for the drivers that use it. So if testing has shown
> that it helps some actual case, I'll believe the testing. But the code
> still looks rather debatable, and the whole IORESOURCE_MUXED approach
> looks broken.
>
> Jesse, that came in through you and the drm tree, I think. Alan is
> marked as author, and there are other people who actually use and can
> test the code. Can you guys think about the code a bit more.
>
> I'm wondering if the *real* fix ends up being to reset the 'parent'
> pointer to the original top-level parent after the sleep?
>
> To recap: the patch is in my tree, but I'm not all that happy about it.

Thanks, yeah i think testing wins in this case.  I'll revisit the muxed 
stuff; I do
remember being dubious at the time, but iirc Alan needed it for something,
and others had been pushing for these sorts of usages for awhile even though
we have some good alternatives in the form of bus and platform drivers that
can manage the appropriate serialization and keep things from stomping
on one another.

(And sorry if this message comes across in some bullshit format, I'm trying
out a new ChromeOS based mail client for fun here.)

Thanks,
Jesse

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Alan Cox]

> we have some good alternatives in the form of bus and platform
> drivers that
> can manage the appropriate serialization and keep things from
> stomping
> on one another.

It's not used much, especially nowdays. The use case is basically multi
I/O chips on the ISA/LPC bus with magic shared config register ports.

We have sufficiently few of those we could give muxed the boot and
special case them if preferred.

Alan

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Jesse Barnes]

On 02/22/2016 05:49 AM, Alan Cox wrote:
>> we have some good alternatives in the form of bus and platform
>> drivers that
>> can manage the appropriate serialization and keep things from
>> stomping
>> on one another.
> 
> It's not used much, especially nowdays. The use case is basically multi
> I/O chips on the ISA/LPC bus with magic shared config register ports.
> 
> We have sufficiently few of those we could give muxed the boot and
> special case them if preferred.

Ah that's right, now I remember the context.  So where should we go from here then?  Just leave the ugly fix in or hack on old stuff and hope not to break it?

Jesse

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Simon Guinot]

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

On Mon, Feb 22, 2016 at 12:46:09PM -0800, Jesse Barnes wrote:
> On 02/22/2016 05:49 AM, Alan Cox wrote:
> >> we have some good alternatives in the form of bus and platform
> >> drivers that
> >> can manage the appropriate serialization and keep things from
> >> stomping
> >> on one another.
> > 
> > It's not used much, especially nowdays. The use case is basically multi
> > I/O chips on the ISA/LPC bus with magic shared config register ports.
> > 
> > We have sufficiently few of those we could give muxed the boot and
> > special case them if preferred.
> 
> Ah that's right, now I remember the context.  So where should we go from here then?  Just leave the ugly fix in or hack on old stuff and hope not to break it?

Hi Jesse,

The fix is not ugly but only incomplete. And I have to say that the
whole IORESOURCE_MUXED thing is not shiny either :)

The main problem in __request_region() is that we are dropping the
spinlock of the resource list while holding a reference on a resource,
waiting for a muxed resource to become available.

From here, I can see two bugs:

1 - At wake-up, the next __request_resource() iteration will not detect
    a remaining conflict. To work properly, __request_resource() needs
    to be called with a parent of the conflicting resource. Instead we
    are passing the conflicting resource itself...
2 - At wake-up the resource pointer we are holding could have been
    freed. Since we are just about to use this pointer to insert a new
    resource in the linked list, it is broken...

My patch fixes 1 and makes things better for 2.

But I think Linus is right. If at wake-up we use the original parent
resource to check again for a conflict, then we are safe.

If you want, I can propose a such patch.

Note that IORESOURCE_MUXED is mostly used by Super-I/Os drivers. A
Super-I/O is a legacy I/O controller embedded on x86 motherboards. It is
used to connect the low-bandwidth devices such as parallel ports, serial
ports, keyboard controllers, hardware monitoring controllers, GPIO
controllers, etc. While each logical device have its own memory region,
a shared memory region is used for some configuration purpose.
IORESOURCE_MUXED is a convenient way to deal with that. For some code
examples you can look at the superio_* functions in the IT87 drivers:
gpio/gpio-it87.c, hwmon/it87.c and watchdog/it87_wdt.c.

I am not aware of any other users for IORESOURCE_MUXED.

Let me know how you want to go and if you need my help.

Simon

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

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Jesse Barnes]

On 02/23/2016 08:19 AM, Simon Guinot wrote:
> On Mon, Feb 22, 2016 at 12:46:09PM -0800, Jesse Barnes wrote:
>> On 02/22/2016 05:49 AM, Alan Cox wrote:
>>>> we have some good alternatives in the form of bus and platform
>>>> drivers that
>>>> can manage the appropriate serialization and keep things from
>>>> stomping
>>>> on one another.
>>>
>>> It's not used much, especially nowdays. The use case is basically multi
>>> I/O chips on the ISA/LPC bus with magic shared config register ports.
>>>
>>> We have sufficiently few of those we could give muxed the boot and
>>> special case them if preferred.
>>
>> Ah that's right, now I remember the context.  So where should we go from here then?  Just leave the ugly fix in or hack on old stuff and hope not to break it?
> 
> Hi Jesse,
> 
> The fix is not ugly but only incomplete. And I have to say that the
> whole IORESOURCE_MUXED thing is not shiny either :)

Yeah definitely, I'm not casting stones at you, this whole problem is an
ugly one. :)

As Alan said, muxed is really intended for a pretty limited set of
cases.  The "right" solution is a lot more work than its worth, so we
have this instead, which is fine.  But obviously it's been a little
trickier to put in place than we hoped. :)

> The main problem in __request_region() is that we are dropping the
> spinlock of the resource list while holding a reference on a resource,
> waiting for a muxed resource to become available.
> 
> From here, I can see two bugs:
> 
> 1 - At wake-up, the next __request_resource() iteration will not detect
>     a remaining conflict. To work properly, __request_resource() needs
>     to be called with a parent of the conflicting resource. Instead we
>     are passing the conflicting resource itself...
> 2 - At wake-up the resource pointer we are holding could have been
>     freed. Since we are just about to use this pointer to insert a new
>     resource in the linked list, it is broken...
> 
> My patch fixes 1 and makes things better for 2.
> 
> But I think Linus is right. If at wake-up we use the original parent
> resource to check again for a conflict, then we are safe.
> 
> If you want, I can propose a such patch.
> 
> Note that IORESOURCE_MUXED is mostly used by Super-I/Os drivers. A
> Super-I/O is a legacy I/O controller embedded on x86 motherboards. It is
> used to connect the low-bandwidth devices such as parallel ports, serial
> ports, keyboard controllers, hardware monitoring controllers, GPIO
> controllers, etc. While each logical device have its own memory region,
> a shared memory region is used for some configuration purpose.
> IORESOURCE_MUXED is a convenient way to deal with that. For some code
> examples you can look at the superio_* functions in the IT87 drivers:
> gpio/gpio-it87.c, hwmon/it87.c and watchdog/it87_wdt.c.
> 
> I am not aware of any other users for IORESOURCE_MUXED.
> 
> Let me know how you want to go and if you need my help.

I'd be happy if you proposed a patch.  It would also be nice if we could
somehow more formally limit the MUXED usage to these super I/O devices,
just so other users don't get into trouble thinking it's more general
than it is.

Thanks,
Jesse

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[One Thousand Gnomes]

> > IORESOURCE_MUXED is a convenient way to deal with that. For some code
> > examples you can look at the superio_* functions in the IT87 drivers:
> > gpio/gpio-it87.c, hwmon/it87.c and watchdog/it87_wdt.c.
> > 
> > I am not aware of any other users for IORESOURCE_MUXED.
> > 
> > Let me know how you want to go and if you need my help.  
> 
> I'd be happy if you proposed a patch.  It would also be nice if we could
> somehow more formally limit the MUXED usage to these super I/O devices,
> just so other users don't get into trouble thinking it's more general
> than it is.

Today I think the problem wouldn't exist. We'd tell the authors of the
drivers to create an mfd or similar to manage the resources and load the
appropriate extra goodies.

Alan

[PATCH] kernel/resource.c: ensure parent is not freed in __request_region()

[Simon Guinot]

The commit 59ceeaaf355fa0fb16558ef7c24413c804932ada
("kernel/resource.c: fix muxed resource handling in __request_region()")
don't address properly the case where the current parent resource has
been freed while sleeping, waiting for a muxed resource to be available.

This patch fixes the issue by resetting the parent pointer to the root
parent resource when sleeping is needed.

Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
Cc: stable@vger.kernel.org
---
 kernel/resource.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 3669d1bfc425..14a7f9d66259 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1052,16 +1052,17 @@ static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
 
 /**
  * __request_region - create a new busy resource region
- * @parent: parent resource descriptor
+ * @root: root resource descriptor
  * @start: resource start address
  * @n: resource region size
  * @name: reserving caller's ID string
  * @flags: IO resource flags
  */
-struct resource * __request_region(struct resource *parent,
+struct resource * __request_region(struct resource *root,
 				   resource_size_t start, resource_size_t n,
 				   const char *name, int flags)
 {
+	struct resource *parent = root;
 	DECLARE_WAITQUEUE(wait, current);
 	struct resource *res = alloc_resource(GFP_KERNEL);
 
@@ -1089,6 +1090,7 @@ struct resource * __request_region(struct resource *parent,
 			}
 		}
 		if (conflict->flags & flags & IORESOURCE_MUXED) {
+			parent = root;
 			add_wait_queue(&muxed_resource_wait, &wait);
 			write_unlock(&resource_lock);
 			set_current_state(TASK_UNINTERRUPTIBLE);
-- 
2.1.4

Re: [PATCH] kernel/resource.c: fix muxed resource handling in __request_region()

[Vincent Pelletier]

On Mon, 22 Feb 2016 13:49:12 +0000, Alan Cox <alan@linux.intel.com>
wrote:
> It's not used much, especially nowdays. The use case is basically multi
> I/O chips on the ISA/LPC bus with magic shared config register ports.

This is precisely a super I/O driver (gpio-f7188x) which, when used
with concurrent accesses on an SMP machine triggered the issue which
prompted this patch.

In case information on the original issue is desired:
My original report (ignore attached patch, it was rejected as it
breaks other chips supported by this driver):
  http://permalink.gmane.org/gmane.linux.kernel.gpio/10204
My test procedure (second half of the mail), which I used to validate
the patch against 4.1:
  http://permalink.gmane.org/gmane.linux.kernel.gpio/10216
Simon Guinot & Vincent Donnefort debugging results:
  http://permalink.gmane.org/gmane.linux.kernel.gpio/10521

Regards,
-- 
Vincent Pelletier

Re: [1/4] gpio: gpio-f7188x: Use mutex for access serialisation.

[Vincent Pelletier]

Hello Simon,

On Thu, 10 Sep 2015 00:01:40 +0200, Simon Guinot
<simon.guinot@sequanux.org> wrote:
> Vincent (Donnefort) finally succeeds to reproduce the issue. The setup
> is quite simple. You only have to flood the gpio-f7188x driver via the
> sysfs GPIO interface. Nothing more is needed.
> 
> After some debugging we discovered that the problem comes from the
> __request_region function which don't handle very well concurrent
> requests on a muxed region.
> 
> I will send a patch as a reply to this email. Please, can you test it ?

I reverted my mutex-adding commit, applied given patch, and could not
reproduce the error after a few minutes with my test-case, so I think
this solves the issue.

Tested-by: Vincent Pelletier <plr.vincent@gmail.com>


I rebased my others gpio patches, unrelated to this issue:

gpio: gpio-f7188x: Implement get_direction.
gpio: gpio-f7188x: "get" should retrieve sensed level when available.
gpio: gpio-f7188x: GPIO bank 0 bit 0 is not available on f71869a

Should I resend ?
I have not checked other model's datasheets, FWIW.

Regards,
-- 
Vincent Pelletier

Re: [1/4] gpio: gpio-f7188x: Use mutex for access serialisation.

[Vincent Donnefort]

Hi Vincent.

I'm working with Simon and I'm trying to reproduce the error you
encountered with the f7188x driver.

On Fri, Aug 21, 2015 at 10:48:24PM +0200, Vincent Pelletier wrote:
> Hello Simon,
> 
> Thanks for reviewing my patch.
> 
> On Fri, 21 Aug 2015 19:52:16 +0200, Simon Guinot
> <simon.guinot@sequanux.org> wrote:
> > IMHO, understand *clearly* the issue could be a good start in order to
> > fix it efficiently.
> 
> I totally agree, but have no idea how to debug further.
> Could you suggest any mechanism to debug *_resource ?
> 
> > Please, could you describe a setup (as simple as possible) allowing to
> > reproduce the issue ? I'll try it on my side.
> [...]
> > Please try to make the module list needed to reproduce the issue as
> > short as possible. Ideally only gpio_f7188x would be needed.
> 
> The simplest I could find so far needs gpio-input-polled with 20ms
> polling period (I didn't try to change polling period), and a shell loop
> writing to gpioXX/value.
> 
> I'm using the following platform driver to declare a single
> gpio-input-polled key:
>   https://github.com/vpelletier/linux/blob/free_nonexistent_resource/drivers/platform/x86/qnap-tsx51.c
> Full version of this driver:
>   https://github.com/vpelletier/linux/blob/ts651/drivers/platform/x86/qnap-tsx51.c
>   (ignore code surrounded with '#if QNAP_TSX51_GPIOD', it is
>   still broken).
> 
> It should be easy to create a variant fitting another board, as long
> as you can recycle two GPIOs.
> 
> Once I've unloaded all other listed drivers for this chip and loaded
> the stripped-down qnap-tsx51 version:
> Terminal 0:
>   echo 0 > /sys/devices/system/cpu/cpu1/online
> Terminal 1:
>   cd /sys/class/gpio/
>   echo 62 > export
>   cd gpio62
>   echo "out" > direction
>   while :; echo 0 > value; echo 1 > value; done
> No error so far.
> Terminal 1:
>   echo 1 > /sys/devices/cpu/cpu1/online
> After a few (10 to 30) seconds, tty is flooded with the error I
> reported.
> 
> Stopping the loop stop the error.
> I have seen rare cases where a few extra messages could occur within
> the next minute or so, but this was before I stripped most out of the
> platform driver.
> 

I didn't manage to see the error you were talking about. To
make sure I understood correctly your previous e-mail and since I saw
some problems with commands you gave, here's what I tested on a
4.2-rc7:

$ echo 62 > /sys/class/gpio/export
$ cd /sys/class/gpio/gpio62
$ while true;do echo 0 > value; echo 1 > value; done

... I let the loop running during several minutes without having any error.
Let me know if these commands are enough for you to reproduce the issue.

I also tried as you, to remove/add CPUs but it didn't triggered
anything more.

$ for i in $(seq 1 3); do echo 0 > /sys/devices/system/cpu/cpu$i/online; done
$ echo 1 > /sys/devices/system/cpu/cpu1/online

The last thing I tested is to perform SuperI/O access through the
hmon driver while running the gpio loop. But once again no problem
appeared.

> With just two such loops poking at (out) GPIOs, it does not happen.
> 
> I'm testing this on v4.1.4 with the following patch applied (the error
> happened before and after that patch, so it likely does not matter):
>   https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7e08117de6ee17ae6c8f2983999a98cb95eb9bc2
> 
> CPU is an intel celeron J1800, dual-core at 2.41GHz.
> Exact board is this machine's motherboard:
>   https://www.qnap.com/i/fr/product/model.php?II=144
> 
> > Unfortunately the f71882fg and f71889f Super-I/Os don't provide an
> > I/O region dedicated to GPIOs. If it was the case, I would have used
> > this way. But for this Super-I/O models, the GPIOs have to be configured
> > through the global registers. That's why your patch breaks support with
> > this models.
> 
> Wow, I didn't expect such difference between models otherwise handled
> the same way in hwmon/f71882fg.c . I guess the GPIO function works
> differently from hwmon function in these models (hwmon still having
> dedicated IO range, not GPIO).
> 
> Regards,
> -- 
> Vincent Pelletier

Re: [1/4] gpio: gpio-f7188x: Use mutex for access serialisation.

[Vincent Pelletier]

Hi Vincent,

On Fri, 4 Sep 2015 15:48:54 +0200, Vincent Donnefort
<vdonnefort@gmail.com> wrote:
> ... I let the loop running during several minutes without having any error.
> Let me know if these commands are enough for you to reproduce the issue.

Did you also have gpio-input-polled active on another pin on the same
chip ?

Regards,
-- 
Vincent Pelletier