| Message ID | 1467563525-30227-1-git-send-email-johan@kernel.org |
|---|---|
| State | New |
| Headers | show |
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ?
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);
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)