diff mbox series

gpio: devres: Handle nonexclusive GPIOs

Message ID 20181204124303.8233-1-linus.walleij@linaro.org
State Accepted
Commit cb28ee388e465a956b05ada682f9ef90e776a9b7
Headers show
Series gpio: devres: Handle nonexclusive GPIOs | expand

Commit Message

Linus Walleij Dec. 4, 2018, 12:43 p.m. UTC 
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(-)

-- 
2.19.2

Comments

Marek Szyprowski Dec. 4, 2018, 1:16 p.m. UTC | #1 
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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Linus Walleij Dec. 4, 2018, 1:38 p.m. UTC | #2 
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
Marek Szyprowski Dec. 4, 2018, 1:55 p.m. UTC | #3 
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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Linus Walleij Dec. 4, 2018, 2:15 p.m. UTC | #4 
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 mbox series

Patch

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;