| Message ID | 20181204124303.8233-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: devres: Handle nonexclusive GPIOs | expand |
Hi Linus, On 2018-12-04 13:43, Linus Walleij wrote: > When we get a nonexeclusive GPIO descriptor using managed > resources, we should only add it to the list of managed > resources once: on the first user. Augment the > devm_gpiod_get_index() and devm_gpiod_get_from_of_node() > calls to account for this by checking if the descriptor > is already resource managed before we proceed to allocate > a new resource management struct. > > Cc: Mark Brown <broonie@kernel.org> > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Fixes: b0ce7b29bfcd ("regulator/gpio: Allow nonexclusive GPIO access") > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > This fix is in response to an issue Marek saw in the fixups > for resource-managed GPIO descriptors used with ena_gpiod > in the regulator subsystem. I will merge this as a fix so > the other fixes can assume it is in place once I have > a confirmation is solves the problem. gpiod_get_from_of_node() still needs a fix for non-exclusive access (current linux-next lacks code for it), but I assume it will be handled by separate patch. The only problem I predict is the lack of refcounting. Assuming that you would like to continue 'Regulator ena_gpiod fixups' approach, the first call to devm_gpiod_unhinge() will remove 'all allocated instances', so if the same descriptor will be used for more than one regulator, it must be somehow handled... > --- > drivers/gpio/gpiolib-devres.c | 50 ++++++++++++++++++++++++++--------- > 1 file changed, 38 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c > index e35751bf0ea8..304ad54dabb7 100644 > --- a/drivers/gpio/gpiolib-devres.c > +++ b/drivers/gpio/gpiolib-devres.c > @@ -98,15 +98,28 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev, > struct gpio_desc **dr; > struct gpio_desc *desc; > > + desc = gpiod_get_index(dev, con_id, idx, flags); > + if (IS_ERR(desc)) > + return desc; > + > + /* > + * For non-exclusive GPIO descriptors, check if this descriptor is > + * already under resource management by this device. > + */ > + if (flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) { > + struct devres *dres; > + > + dres = devres_find(dev, devm_gpiod_release, > + devm_gpiod_match, desc); > + if (dres) > + return desc; > + } > + > dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *), > GFP_KERNEL); > - if (!dr) > + if (!dr) { > + gpiod_put(desc); > return ERR_PTR(-ENOMEM); > - > - desc = gpiod_get_index(dev, con_id, idx, flags); > - if (IS_ERR(desc)) { > - devres_free(dr); > - return desc; > } > > *dr = desc; > @@ -140,15 +153,28 @@ struct gpio_desc *devm_gpiod_get_from_of_node(struct device *dev, > struct gpio_desc **dr; > struct gpio_desc *desc; > > + desc = gpiod_get_from_of_node(node, propname, index, dflags, label); > + if (IS_ERR(desc)) > + return desc; > + > + /* > + * For non-exclusive GPIO descriptors, check if this descriptor is > + * already under resource management by this device. > + */ > + if (dflags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) { > + struct devres *dres; > + > + dres = devres_find(dev, devm_gpiod_release, > + devm_gpiod_match, desc); > + if (dres) > + return desc; > + } > + > dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *), > GFP_KERNEL); > - if (!dr) > + if (!dr) { > + gpiod_put(desc); > return ERR_PTR(-ENOMEM); > - > - desc = gpiod_get_from_of_node(node, propname, index, dflags, label); > - if (IS_ERR(desc)) { > - devres_free(dr); > - return desc; > } > > *dr = desc; Best regards
On Tue, Dec 4, 2018 at 2:16 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > gpiod_get_from_of_node() still needs a fix for non-exclusive access > (current linux-next lacks code for it), but I assume it will be handled > by separate patch. Yes but since that is not a regression I am just including it in the repost of the fixup series. > The only problem I predict is the lack of refcounting. Assuming that you > would like to continue 'Regulator ena_gpiod fixups' approach, the first > call to devm_gpiod_unhinge() will remove 'all allocated instances', so > if the same descriptor will be used for more than one regulator, it must > be somehow handled... I looked closer at it... so if you try to remove a resource from devres handling that is not handled by devres, it will return -ENOENT which spits a warning, so I will augment the patch adding the gpiod_unhinge() so it ignores this error. Since at that point we will immediately handle over the first user of the GPIO descriptor to the regulator core, the first call effectively transfers the ownership of *all* users of this descriptor to the regulator core. When the second descriptor is unhinged, nothing will happen (as it is no longer under managed resources) then it is passed to the regulator core that will identify that it is already managing this descriptor, and simply increase the refcount to 1. This happens in regulator_ena_gpio_request() where the regulator_ena_gpio_list is traversed and if the gpio desc is already in used it simply results in pin->request_count++. So AFAICT it is going to work just fine if I just make sure that devm_gpiod_unhinge() ignores -ENOENT. Adding this! Yours, Linus Walleij
Hi Linus, On 2018-12-04 14:38, Linus Walleij wrote: > On Tue, Dec 4, 2018 at 2:16 PM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: > >> gpiod_get_from_of_node() still needs a fix for non-exclusive access >> (current linux-next lacks code for it), but I assume it will be handled >> by separate patch. > Yes but since that is not a regression I am just including it in > the repost of the fixup series. > >> The only problem I predict is the lack of refcounting. Assuming that you >> would like to continue 'Regulator ena_gpiod fixups' approach, the first >> call to devm_gpiod_unhinge() will remove 'all allocated instances', so >> if the same descriptor will be used for more than one regulator, it must >> be somehow handled... > I looked closer at it... so if you try to remove a resource from > devres handling that is not handled by devres, it will return -ENOENT > which spits a warning, so I will augment the patch adding > the gpiod_unhinge() so it ignores this error. > > Since at that point we will immediately handle over the first > user of the GPIO descriptor to the regulator core, the first call > effectively transfers the ownership of *all* users of this descriptor > to the regulator core. > > When the second descriptor is unhinged, nothing will happen > (as it is no longer under managed resources) then it is passed > to the regulator core that will identify that it is already managing > this descriptor, and simply increase the refcount to 1. > > This happens in regulator_ena_gpio_request() where the > regulator_ena_gpio_list is traversed and if the gpio desc > is already in used it simply results in > pin->request_count++. > > So AFAICT it is going to work just fine if I just make sure > that devm_gpiod_unhinge() ignores -ENOENT. > > Adding this! It is still something wrong there, because I get a warning from devm_gpiod_unhinge() two times. If I got it right, I should only get one such warning with this patch. Best regards
On Tue, Dec 4, 2018 at 2:55 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > It is still something wrong there, because I get a warning from > devm_gpiod_unhinge() two times. If I got it right, I should only get one > such warning with this patch. So they should be coming from this: arch/arm/boot/dts/exynos3250-artik5.dtsi ldo11_reg: LDO11 { /* VDD74 ~ VDD75 */ regulator-name = "VLDO11_1.8V"; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <1800000>; samsung,ext-control-gpios = <&gpk0 2 GPIO_ACTIVE_HIGH>; }; ldo12_reg: LDO12 { /* VDD72 ~ VDD73 */ regulator-name = "VLDO12_2.8V"; regulator-min-microvolt = <2800000>; regulator-max-microvolt = <2800000>; samsung,ext-control-gpios = <&gpk0 2 GPIO_ACTIVE_HIGH>; }; It would make sense on arch/arm/boot/dts/exynos3250-monk.dts that shares the same enable line three times. My patch now looks like below: +/** + * devm_gpiod_unhinge - Remove resource management from a gpio descriptor + * @dev: GPIO consumer + * @desc: GPIO descriptor to remove resource management from + * + * Remove resource management from a GPIO descriptor. This is needed when + * you want to hand over lifecycle management of a descriptor to another + * mechanism. + */ + +void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc) +{ + int ret; + + if (IS_ERR_OR_NULL(desc)) + return; + ret = devres_destroy(dev, devm_gpiod_release, + devm_gpiod_match, desc); + /* + * If the GPIO descriptor is requested as nonexclusive, we + * may call this function several times on the same descriptor + * so it is OK if devres_destroy() returns -ENOENT. + */ + if (ret == -ENOENT) + return; + /* Anything else we should warn about */ + WARN_ON(ret); +} +EXPORT_SYMBOL(devm_gpiod_unhinge); I guess I should just resend the series. If -ENOENT appears more than once we might need to introduce some prints to figure out what's going on. Should I put the fix that I'm sending upstream for the nonexclusive descriptors in front of the series, with some [DO NOT APPLY] in the subject, so we just know that is required by will soon be upstream? Yours, Linus Walleij
diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c index e35751bf0ea8..304ad54dabb7 100644 --- a/drivers/gpio/gpiolib-devres.c +++ b/drivers/gpio/gpiolib-devres.c @@ -98,15 +98,28 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev, struct gpio_desc **dr; struct gpio_desc *desc; + desc = gpiod_get_index(dev, con_id, idx, flags); + if (IS_ERR(desc)) + return desc; + + /* + * For non-exclusive GPIO descriptors, check if this descriptor is + * already under resource management by this device. + */ + if (flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) { + struct devres *dres; + + dres = devres_find(dev, devm_gpiod_release, + devm_gpiod_match, desc); + if (dres) + return desc; + } + dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *), GFP_KERNEL); - if (!dr) + if (!dr) { + gpiod_put(desc); return ERR_PTR(-ENOMEM); - - desc = gpiod_get_index(dev, con_id, idx, flags); - if (IS_ERR(desc)) { - devres_free(dr); - return desc; } *dr = desc; @@ -140,15 +153,28 @@ struct gpio_desc *devm_gpiod_get_from_of_node(struct device *dev, struct gpio_desc **dr; struct gpio_desc *desc; + desc = gpiod_get_from_of_node(node, propname, index, dflags, label); + if (IS_ERR(desc)) + return desc; + + /* + * For non-exclusive GPIO descriptors, check if this descriptor is + * already under resource management by this device. + */ + if (dflags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) { + struct devres *dres; + + dres = devres_find(dev, devm_gpiod_release, + devm_gpiod_match, desc); + if (dres) + return desc; + } + dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *), GFP_KERNEL); - if (!dr) + if (!dr) { + gpiod_put(desc); return ERR_PTR(-ENOMEM); - - desc = gpiod_get_from_of_node(node, propname, index, dflags, label); - if (IS_ERR(desc)) { - devres_free(dr); - return desc; } *dr = desc;
When we get a nonexeclusive GPIO descriptor using managed resources, we should only add it to the list of managed resources once: on the first user. Augment the devm_gpiod_get_index() and devm_gpiod_get_from_of_node() calls to account for this by checking if the descriptor is already resource managed before we proceed to allocate a new resource management struct. Cc: Mark Brown <broonie@kernel.org> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> Cc: Marek Szyprowski <m.szyprowski@samsung.com> Fixes: b0ce7b29bfcd ("regulator/gpio: Allow nonexclusive GPIO access") Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- This fix is in response to an issue Marek saw in the fixups for resource-managed GPIO descriptors used with ena_gpiod in the regulator subsystem. I will merge this as a fix so the other fixes can assume it is in place once I have a confirmation is solves the problem. --- drivers/gpio/gpiolib-devres.c | 50 ++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 12 deletions(-)