All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration"
@ 2016-07-03 16:32 Johan Hovold
  2016-07-04 13:16 ` Linus Walleij
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Johan Hovold @ 2016-07-03 16:32 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Alexandre Courbot, Laurent Pinchart, linux-gpio, linux-kernel,
	Johan Hovold

This reverts commit 923b93e451db876d1479d3e4458fce14fec31d1c.

Make sure consumers do not overwrite gpio flags for pins that have
already been claimed.

While adding support for gpio drivers to refuse a request using
unsupported flags, the order of when the requested flag was checked and
the new flags were applied was reversed to that consumers could
overwrite flags for already requested gpios.

This not only affects device-tree setups where two drivers could request
the same gpio using conflicting configurations, but also allowed user
space to clear gpio flags for already claimed pins simply by attempting
to export them through the sysfs interface. By for example clearing the
FLAG_ACTIVE_LOW flag this way, user space could effectively change the
polarity of a signal.

Reverting this change obviously prevents gpio drivers from doing sanity
checks on the flags in their request callbacks. Fortunately only one
recently added driver (gpio-tps65218 in v4.6) appears to do this, and a
follow up patch could restore this functionality through a different
interface.

Cc: stable <stable@vger.kernel.org>	# 4.4
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-legacy.c |  8 +++----
 drivers/gpio/gpiolib.c        | 52 +++++++++++++------------------------------
 2 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/drivers/gpio/gpiolib-legacy.c b/drivers/gpio/gpiolib-legacy.c
index 3a5c7011ad3b..8b830996fe02 100644
--- a/drivers/gpio/gpiolib-legacy.c
+++ b/drivers/gpio/gpiolib-legacy.c
@@ -28,6 +28,10 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 	if (!desc && gpio_is_valid(gpio))
 		return -EPROBE_DEFER;
 
+	err = gpiod_request(desc, label);
+	if (err)
+		return err;
+
 	if (flags & GPIOF_OPEN_DRAIN)
 		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
 
@@ -37,10 +41,6 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 	if (flags & GPIOF_ACTIVE_LOW)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
-	err = gpiod_request(desc, label);
-	if (err)
-		return err;
-
 	if (flags & GPIOF_DIR_IN)
 		err = gpiod_direction_input(desc);
 	else
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 570771ed19e6..be74bd370f1f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1352,14 +1352,6 @@ static int __gpiod_request(struct gpio_desc *desc, const char *label)
 		spin_lock_irqsave(&gpio_lock, flags);
 	}
 done:
-	if (status < 0) {
-		/* Clear flags that might have been set by the caller before
-		 * requesting the GPIO.
-		 */
-		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
-		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
-		clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
-	}
 	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
@@ -2587,28 +2579,13 @@ struct gpio_desc *__must_check gpiod_get_optional(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(gpiod_get_optional);
 
-/**
- * gpiod_parse_flags - helper function to parse GPIO lookup flags
- * @desc:	gpio to be setup
- * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
- *		of_get_gpio_hog()
- *
- * Set the GPIO descriptor flags based on the given GPIO lookup flags.
- */
-static void gpiod_parse_flags(struct gpio_desc *desc, unsigned long lflags)
-{
-	if (lflags & GPIO_ACTIVE_LOW)
-		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
-	if (lflags & GPIO_OPEN_DRAIN)
-		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
-	if (lflags & GPIO_OPEN_SOURCE)
-		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
-}
 
 /**
  * gpiod_configure_flags - helper function to configure a given GPIO
  * @desc:	gpio whose value will be assigned
  * @con_id:	function within the GPIO consumer
+ * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
+ *		of_get_gpio_hog()
  * @dflags:	gpiod_flags - optional GPIO initialization flags
  *
  * Return 0 on success, -ENOENT if no GPIO has been assigned to the
@@ -2616,10 +2593,17 @@ static void gpiod_parse_flags(struct gpio_desc *desc, unsigned long lflags)
  * occurred while trying to acquire the GPIO.
  */
 static int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
-				 enum gpiod_flags dflags)
+		unsigned long lflags, enum gpiod_flags dflags)
 {
 	int status;
 
+	if (lflags & GPIO_ACTIVE_LOW)
+		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+	if (lflags & GPIO_OPEN_DRAIN)
+		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
+	if (lflags & GPIO_OPEN_SOURCE)
+		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+
 	/* No particular flag request, return here... */
 	if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
 		pr_debug("no flags found for %s\n", con_id);
@@ -2686,13 +2670,11 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 		return desc;
 	}
 
-	gpiod_parse_flags(desc, lookupflags);
-
 	status = gpiod_request(desc, con_id);
 	if (status < 0)
 		return ERR_PTR(status);
 
-	status = gpiod_configure_flags(desc, con_id, flags);
+	status = gpiod_configure_flags(desc, con_id, lookupflags, flags);
 	if (status < 0) {
 		dev_dbg(dev, "setup of GPIO %s failed\n", con_id);
 		gpiod_put(desc);
@@ -2748,6 +2730,10 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	if (IS_ERR(desc))
 		return desc;
 
+	ret = gpiod_request(desc, NULL);
+	if (ret)
+		return ERR_PTR(ret);
+
 	if (active_low)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
@@ -2758,10 +2744,6 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
 	}
 
-	ret = gpiod_request(desc, NULL);
-	if (ret)
-		return ERR_PTR(ret);
-
 	return desc;
 }
 EXPORT_SYMBOL_GPL(fwnode_get_named_gpiod);
@@ -2814,8 +2796,6 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 	chip = gpiod_to_chip(desc);
 	hwnum = gpio_chip_hwgpio(desc);
 
-	gpiod_parse_flags(desc, lflags);
-
 	local_desc = gpiochip_request_own_desc(chip, hwnum, name);
 	if (IS_ERR(local_desc)) {
 		status = PTR_ERR(local_desc);
@@ -2824,7 +2804,7 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 		return status;
 	}
 
-	status = gpiod_configure_flags(desc, name, dflags);
+	status = gpiod_configure_flags(desc, name, lflags, dflags);
 	if (status < 0) {
 		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed, %d\n",
 		       name, chip->label, hwnum, status);
-- 
2.7.3

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

* Re: [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration"
  2016-07-03 16:32 [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration" Johan Hovold
@ 2016-07-04 13:16 ` Linus Walleij
  2016-07-04 20:30   ` Laurent Pinchart
  2016-07-04 14:54 ` Linus Walleij
  2016-07-04 20:33 ` Laurent Pinchart
  2 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2016-07-04 13:16 UTC (permalink / raw)
  To: Johan Hovold, Laurent Pinchart
  Cc: Alexandre Courbot, linux-gpio, linux-kernel

On Sun, Jul 3, 2016 at 6:32 PM, Johan Hovold <johan@kernel.org> wrote:

> This reverts commit 923b93e451db876d1479d3e4458fce14fec31d1c.

Laurent, can we have you comment on this?

Getting close to v4.7 final so we need to quickly see if
this is the solution or if it will cause further regressions on
your side if I revert it...

Yours,
Linus Walleij

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

* Re: [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration"
  2016-07-03 16:32 [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration" Johan Hovold
  2016-07-04 13:16 ` Linus Walleij
@ 2016-07-04 14:54 ` Linus Walleij
  2016-07-04 20:33 ` Laurent Pinchart
  2 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2016-07-04 14:54 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Alexandre Courbot, Laurent Pinchart, linux-gpio, linux-kernel

On Sun, Jul 3, 2016 at 6:32 PM, Johan Hovold <johan@kernel.org> wrote:

> This reverts commit 923b93e451db876d1479d3e4458fce14fec31d1c.
>
> Make sure consumers do not overwrite gpio flags for pins that have
> already been claimed.

Patch applied for fixes and pushed for build server for
testing for now. It looks innocent to me.

Yours,
Linus Walleij

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

* Re: [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration"
  2016-07-04 13:16 ` Linus Walleij
@ 2016-07-04 20:30   ` Laurent Pinchart
  2016-07-05  6:52     ` Linus Walleij
  0 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2016-07-04 20:30 UTC (permalink / raw)
  To: Linus Walleij; +Cc: Johan Hovold, Alexandre Courbot, linux-gpio, linux-kernel

On Monday 04 Jul 2016 15:16:40 Linus Walleij wrote:
> On Sun, Jul 3, 2016 at 6:32 PM, Johan Hovold <johan@kernel.org> wrote:
> > This reverts commit 923b93e451db876d1479d3e4458fce14fec31d1c.
> 
> Laurent, can we have you comment on this?
> 
> Getting close to v4.7 final so we need to quickly see if
> this is the solution or if it will cause further regressions on
> your side if I revert it...

Given that the offending patch dates from October 2015, why is the v4.7 
schedule an issue ?

-- 
Regards,

Laurent Pinchart


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

* Re: [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration"
  2016-07-03 16:32 [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration" Johan Hovold
  2016-07-04 13:16 ` Linus Walleij
  2016-07-04 14:54 ` Linus Walleij
@ 2016-07-04 20:33 ` Laurent Pinchart
  2016-07-05  6:54   ` Linus Walleij
  2016-07-05  8:56   ` Johan Hovold
  2 siblings, 2 replies; 10+ messages in thread
From: Laurent Pinchart @ 2016-07-04 20:33 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-kernel

Hi Johan,

Thank you for the patch.

On Sunday 03 Jul 2016 18:32:05 Johan Hovold wrote:
> This reverts commit 923b93e451db876d1479d3e4458fce14fec31d1c.
> 
> Make sure consumers do not overwrite gpio flags for pins that have
> already been claimed.
> 
> While adding support for gpio drivers to refuse a request using
> unsupported flags, the order of when the requested flag was checked and
> the new flags were applied was reversed to that consumers could
> overwrite flags for already requested gpios.
> 
> This not only affects device-tree setups where two drivers could request
> the same gpio using conflicting configurations, but also allowed user
> space to clear gpio flags for already claimed pins simply by attempting
> to export them through the sysfs interface. By for example clearing the
> FLAG_ACTIVE_LOW flag this way, user space could effectively change the
> polarity of a signal.
> 
> Reverting this change obviously prevents gpio drivers from doing sanity
> checks on the flags in their request callbacks. Fortunately only one
> recently added driver (gpio-tps65218 in v4.6) appears to do this, and a
> follow up patch could restore this functionality through a different
> interface.

As we're not dealing with a v4.7 regression that would need to be applied this 
week, can't you propose a proper fix instead of a revert ?

> Cc: stable <stable@vger.kernel.org>	# 4.4
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/gpio/gpiolib-legacy.c |  8 +++----
>  drivers/gpio/gpiolib.c        | 52
> +++++++++++++------------------------------ 2 files changed, 20
> insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-legacy.c b/drivers/gpio/gpiolib-legacy.c
> index 3a5c7011ad3b..8b830996fe02 100644
> --- a/drivers/gpio/gpiolib-legacy.c
> +++ b/drivers/gpio/gpiolib-legacy.c
> @@ -28,6 +28,10 @@ int gpio_request_one(unsigned gpio, unsigned long flags,
> const char *label) if (!desc && gpio_is_valid(gpio))
>  		return -EPROBE_DEFER;
> 
> +	err = gpiod_request(desc, label);
> +	if (err)
> +		return err;
> +
>  	if (flags & GPIOF_OPEN_DRAIN)
>  		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> 
> @@ -37,10 +41,6 @@ int gpio_request_one(unsigned gpio, unsigned long flags,
> const char *label) if (flags & GPIOF_ACTIVE_LOW)
>  		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> 
> -	err = gpiod_request(desc, label);
> -	if (err)
> -		return err;
> -
>  	if (flags & GPIOF_DIR_IN)
>  		err = gpiod_direction_input(desc);
>  	else
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 570771ed19e6..be74bd370f1f 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1352,14 +1352,6 @@ static int __gpiod_request(struct gpio_desc *desc,
> const char *label) spin_lock_irqsave(&gpio_lock, flags);
>  	}
>  done:
> -	if (status < 0) {
> -		/* Clear flags that might have been set by the caller before
> -		 * requesting the GPIO.
> -		 */
> -		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
> -		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
> -		clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
> -	}
>  	spin_unlock_irqrestore(&gpio_lock, flags);
>  	return status;
>  }
> @@ -2587,28 +2579,13 @@ struct gpio_desc *__must_check
> gpiod_get_optional(struct device *dev, }
>  EXPORT_SYMBOL_GPL(gpiod_get_optional);
> 
> -/**
> - * gpiod_parse_flags - helper function to parse GPIO lookup flags
> - * @desc:	gpio to be setup
> - * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
> - *		of_get_gpio_hog()
> - *
> - * Set the GPIO descriptor flags based on the given GPIO lookup flags.
> - */
> -static void gpiod_parse_flags(struct gpio_desc *desc, unsigned long lflags)
> -{
> -	if (lflags & GPIO_ACTIVE_LOW)
> -		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> -	if (lflags & GPIO_OPEN_DRAIN)
> -		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> -	if (lflags & GPIO_OPEN_SOURCE)
> -		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> -}
> 
>  /**
>   * gpiod_configure_flags - helper function to configure a given GPIO
>   * @desc:	gpio whose value will be assigned
>   * @con_id:	function within the GPIO consumer
> + * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
> + *		of_get_gpio_hog()
>   * @dflags:	gpiod_flags - optional GPIO initialization flags
>   *
>   * Return 0 on success, -ENOENT if no GPIO has been assigned to the
> @@ -2616,10 +2593,17 @@ static void gpiod_parse_flags(struct gpio_desc
> *desc, unsigned long lflags) * occurred while trying to acquire the GPIO.
>   */
>  static int gpiod_configure_flags(struct gpio_desc *desc, const char
> *con_id, -				 enum gpiod_flags dflags)
> +		unsigned long lflags, enum gpiod_flags dflags)
>  {
>  	int status;
> 
> +	if (lflags & GPIO_ACTIVE_LOW)
> +		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +	if (lflags & GPIO_OPEN_DRAIN)
> +		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> +	if (lflags & GPIO_OPEN_SOURCE)
> +		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> +
>  	/* No particular flag request, return here... */
>  	if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
>  		pr_debug("no flags found for %s\n", con_id);
> @@ -2686,13 +2670,11 @@ struct gpio_desc *__must_check
> gpiod_get_index(struct device *dev, return desc;
>  	}
> 
> -	gpiod_parse_flags(desc, lookupflags);
> -
>  	status = gpiod_request(desc, con_id);
>  	if (status < 0)
>  		return ERR_PTR(status);
> 
> -	status = gpiod_configure_flags(desc, con_id, flags);
> +	status = gpiod_configure_flags(desc, con_id, lookupflags, flags);
>  	if (status < 0) {
>  		dev_dbg(dev, "setup of GPIO %s failed\n", con_id);
>  		gpiod_put(desc);
> @@ -2748,6 +2730,10 @@ struct gpio_desc *fwnode_get_named_gpiod(struct
> fwnode_handle *fwnode, if (IS_ERR(desc))
>  		return desc;
> 
> +	ret = gpiod_request(desc, NULL);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	if (active_low)
>  		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> 
> @@ -2758,10 +2744,6 @@ struct gpio_desc *fwnode_get_named_gpiod(struct
> fwnode_handle *fwnode, set_bit(FLAG_OPEN_SOURCE, &desc->flags);
>  	}
> 
> -	ret = gpiod_request(desc, NULL);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
>  	return desc;
>  }
>  EXPORT_SYMBOL_GPL(fwnode_get_named_gpiod);
> @@ -2814,8 +2796,6 @@ int gpiod_hog(struct gpio_desc *desc, const char
> *name, chip = gpiod_to_chip(desc);
>  	hwnum = gpio_chip_hwgpio(desc);
> 
> -	gpiod_parse_flags(desc, lflags);
> -
>  	local_desc = gpiochip_request_own_desc(chip, hwnum, name);
>  	if (IS_ERR(local_desc)) {
>  		status = PTR_ERR(local_desc);
> @@ -2824,7 +2804,7 @@ int gpiod_hog(struct gpio_desc *desc, const char
> *name, return status;
>  	}
> 
> -	status = gpiod_configure_flags(desc, name, dflags);
> +	status = gpiod_configure_flags(desc, name, lflags, dflags);
>  	if (status < 0) {
>  		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed, 
%d\n",
>  		       name, chip->label, hwnum, status);

-- 
Regards,

Laurent Pinchart


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

* Re: [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration"
  2016-07-04 20:30   ` Laurent Pinchart
@ 2016-07-05  6:52     ` Linus Walleij
  0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2016-07-05  6:52 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Johan Hovold, Alexandre Courbot, linux-gpio, linux-kernel

On Mon, Jul 4, 2016 at 10:30 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 04 Jul 2016 15:16:40 Linus Walleij wrote:
>> On Sun, Jul 3, 2016 at 6:32 PM, Johan Hovold <johan@kernel.org> wrote:
>> > This reverts commit 923b93e451db876d1479d3e4458fce14fec31d1c.
>>
>> Laurent, can we have you comment on this?
>>
>> Getting close to v4.7 final so we need to quickly see if
>> this is the solution or if it will cause further regressions on
>> your side if I revert it...
>
> Given that the offending patch dates from October 2015, why is the v4.7
> schedule an issue ?

Just convenient to cut a clean kernel and say that if people use
v4.7 or later they don't see the issue. Else you have to tell them to
use one or another stable kernel (which they should, for products etc)
which complicates things.

But you're right, it's no *big* deal in that sense.

Johan does use some alarmin language like destroyed hardware in
the patch description making me feel the issue is urgent though.
Given how long it has been there I don't know if it is really urgent
in practice and not only in theory.

Yours,
Linus Walleij

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

* Re: [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration"
  2016-07-04 20:33 ` Laurent Pinchart
@ 2016-07-05  6:54   ` Linus Walleij
  2016-07-05  9:12     ` Johan Hovold
  2016-07-05  8:56   ` Johan Hovold
  1 sibling, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2016-07-05  6:54 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Johan Hovold, Alexandre Courbot, linux-gpio, linux-kernel

On Mon, Jul 4, 2016 at 10:33 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> As we're not dealing with a v4.7 regression that would need to be applied this
> week, can't you propose a proper fix instead of a revert ?

AFAICT the proper fix is to simply move the gpiod_request() sites earlier,
I can propose something if that's all right?

Yours,
Linus Walleij

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

* Re: [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration"
  2016-07-04 20:33 ` Laurent Pinchart
  2016-07-05  6:54   ` Linus Walleij
@ 2016-07-05  8:56   ` Johan Hovold
  1 sibling, 0 replies; 10+ messages in thread
From: Johan Hovold @ 2016-07-05  8:56 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Johan Hovold, Linus Walleij, Alexandre Courbot, linux-gpio, linux-kernel

On Mon, Jul 04, 2016 at 11:33:16PM +0300, Laurent Pinchart wrote:
> Hi Johan,
> 
> Thank you for the patch.
> 
> On Sunday 03 Jul 2016 18:32:05 Johan Hovold wrote:
> > This reverts commit 923b93e451db876d1479d3e4458fce14fec31d1c.
> > 
> > Make sure consumers do not overwrite gpio flags for pins that have
> > already been claimed.
> > 
> > While adding support for gpio drivers to refuse a request using
> > unsupported flags, the order of when the requested flag was checked and
> > the new flags were applied was reversed to that consumers could
> > overwrite flags for already requested gpios.
> > 
> > This not only affects device-tree setups where two drivers could request
> > the same gpio using conflicting configurations, but also allowed user
> > space to clear gpio flags for already claimed pins simply by attempting
> > to export them through the sysfs interface. By for example clearing the
> > FLAG_ACTIVE_LOW flag this way, user space could effectively change the
> > polarity of a signal.
> > 
> > Reverting this change obviously prevents gpio drivers from doing sanity
> > checks on the flags in their request callbacks. Fortunately only one
> > recently added driver (gpio-tps65218 in v4.6) appears to do this, and a
> > follow up patch could restore this functionality through a different
> > interface.
> 
> As we're not dealing with a v4.7 regression that would need to be
> applied this week, can't you propose a proper fix instead of a
> revert ?

The patch that caused this regression is just plain broken, and AFAICT
trying to fix that will amount to something that is hardly stable
material (e.g. modifying the request callback prototype for all gpio
drivers, after effectively reverting most of the patch anyway).

On the up-side no one seems to be relying on the new feature of allowing
gpio drivers to nak requests using unsupported flags before 4.6, and in
4.6 only the one driver mentioned above does use it.

So by reverting now, before 4.7 is out, we have unbroken all gpio
drivers in all released kernels at the expense of a removed sanity check
from one driver in 4.6.

This new feature can then be implemented properly in the 4.8-rc
time frame (or even before 4.7 is out if you're fast).

As to the severity of the regression, it's bad enough to have an
unrelated driver or user space be able to overwrite the flags of an
already requested pin. Inverting polarity, for example, could lead to
all sorts of interesting breakage and in the worst case even damage
hardware.

The bug is also made harder to track down due to the fact that the
corrupted flags will typically only take effect on subsequent state
changes after the pin has first been initialised. This means that a
system can appear to work for example up until a suspend cycle where a
regulator is disabled (or so you thought).

I hit this myself over the weekend, and it's possible others have too
even if this regression hasn't been reported until now.

Thanks,
Johan

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

* Re: [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration"
  2016-07-05  6:54   ` Linus Walleij
@ 2016-07-05  9:12     ` Johan Hovold
  2016-07-05 13:50       ` Linus Walleij
  0 siblings, 1 reply; 10+ messages in thread
From: Johan Hovold @ 2016-07-05  9:12 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Laurent Pinchart, Johan Hovold, Alexandre Courbot, linux-gpio,
	linux-kernel

On Tue, Jul 05, 2016 at 08:54:36AM +0200, Linus Walleij wrote:
> On Mon, Jul 4, 2016 at 10:33 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> 
> > As we're not dealing with a v4.7 regression that would need to be applied this
> > week, can't you propose a proper fix instead of a revert ?
> 
> AFAICT the proper fix is to simply move the gpiod_request() sites earlier,
> I can propose something if that's all right?

How would that work?

You still need to claim the pin before touching the descriptor flags,
and that isn't done until gpiod_request(). Furthermore, it's that very
function that calls the gpio driver request() callback, which in turn
was supposed to do the sanity check.

So what you need to do is to pass the desired flags down the call stack,
and make sure not to apply them before claiming the pin.

This may work without modifying the request callback prototype (which I
mentioned in my reply to Laurent), but it could still get a bit
invasive and hence not suitable for stable (and we really need this to
be fixed in 4.[456] stable).

Your call. :)

Thanks,
Johan

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

* Re: [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration"
  2016-07-05  9:12     ` Johan Hovold
@ 2016-07-05 13:50       ` Linus Walleij
  0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2016-07-05 13:50 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Laurent Pinchart, Alexandre Courbot, linux-gpio, linux-kernel

On Tue, Jul 5, 2016 at 11:12 AM, Johan Hovold <johan@kernel.org> wrote:

> Your call. :)

The patch is on my fixes branch and will go to Torvalds
by the end of the week unless I'm convinced otherwise ...

It turns out the patch is very isolated and doesn't collide with
any gpio development since 4.4 (which is impressive in itself)
so reverting is not a problem at all, as I thought earlier.

Yours,
Linus Walleij

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

end of thread, other threads:[~2016-07-05 13:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-03 16:32 [PATCH] Revert "gpiolib: Split GPIO flags parsing and GPIO configuration" Johan Hovold
2016-07-04 13:16 ` Linus Walleij
2016-07-04 20:30   ` Laurent Pinchart
2016-07-05  6:52     ` Linus Walleij
2016-07-04 14:54 ` Linus Walleij
2016-07-04 20:33 ` Laurent Pinchart
2016-07-05  6:54   ` Linus Walleij
2016-07-05  9:12     ` Johan Hovold
2016-07-05 13:50       ` Linus Walleij
2016-07-05  8:56   ` Johan Hovold

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.