regulator: wm8994: Revert back to using devres

Currently, the regulator core will take ownership of any GPIO passed into it. Makes end driver code fairly error prone as the normal devm_ patterns of allocation don't work. Update the regulator core to only free the GPIO if it requested it, this allows the drivers to manage the GPIO lifetime as they normally would.

Reported-by Marek Szyprowski <[email protected]>

Signed-off-by Charles Keepax <[email protected]>

Currently, the regulator core will take ownership of any GPIO passed into it. Makes end driver code fairly error prone as the normal devm_ patterns of allocation don't work. Update the regulator core to only free the GPIO if it requested it, this allows the drivers to manage the GPIO lifetime as they normally would.

Reported-by Marek Szyprowski <[email protected]>

Signed-off-by Charles Keepax <[email protected]>

---
 drivers/regulator/core.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

drivers/regulator/core.c +9 -7

index dbe2f2e6e6254..9da7d27c7145e 100644
@@ -83,6 +83,7 @@ struct regulator_enable_gpio {
 	u32 enable_count;	/* a number of enabled shared GPIO */
 	u32 request_count;	/* a number of requested shared GPIO */
 	unsigned int ena_gpio_invert:1;
+	unsigned int locally_requested:1;
 };
 
 /*

@@ -2233,19 +2234,20 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
 		}
 	}
 
+	pin = kzalloc(sizeof(struct regulator_enable_gpio), GFP_KERNEL);
+	if (pin == NULL)
+		return -ENOMEM;
+
 	if (!config->ena_gpiod) {
 		ret = gpio_request_one(config->ena_gpio,
 				       GPIOF_DIR_OUT | config->ena_gpio_flags,
 				       rdev_get_name(rdev));
-		if (ret)
+		if (ret) {
+			kfree(pin);
 			return ret;
-	}
+		}
 
-	pin = kzalloc(sizeof(struct regulator_enable_gpio), GFP_KERNEL);
-	if (pin == NULL) {
-		if (!config->ena_gpiod)
-			gpio_free(config->ena_gpio);
-		return -ENOMEM;
+		pin->locally_requested = 1;
 	}
 
 	pin->gpiod = gpiod;

Currently, the regulator core will take ownership of any GPIO passed into it. Makes end driver code fairly error prone as the normal devm_ patterns of allocation don't work. Update the regulator core to only free the GPIO if it requested it, this allows the drivers to manage the GPIO lifetime as they normally would.

However even with patch 3 I think it'd be better to base this off the rest of Linus' series for converting to descriptors (which is currently sitting waiting for some more testing) since that will convert everything to descriptors and so remove the code that's doing requests in the core entirely.

Currently, a GPIO can be requested multiple times when the NONEXCLUSIVE flag is set, however it must still be freed a single time. This makes client code rather complex, since multiple drivers may request the GPIO but only a single one can free it. Rather than manually handling this in each driver add some basic reference counting into the core. Currently, this is fairly primitive but so is the support for the NONEXCLUSIVE flag and the implementation covers those use-cases.

Reported-by Marek Szyprowski <[email protected]>

Signed-off-by Charles Keepax <[email protected]>

---
 drivers/gpio/gpiolib.c | 13 ++++++++++++-
 drivers/gpio/gpiolib.h |  2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

drivers/gpio/gpiolib.c +12 -1

index 230e41562462b..42ba86fb495a5 100644
@@ -2407,6 +2407,13 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 
 	might_sleep();
 
+	if (desc->n_users > 1) {
+		desc->n_users--;
+		return true;
+	} else {
+		desc->n_users = 0;
+	}
+
 	gpiod_unexport(desc);
 
 	spin_lock_irqsave(&gpio_lock, flags);

@@ -4142,7 +4149,8 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 			 */
 			dev_info(dev, "nonexclusive access to GPIO for %s\n",
 				 con_id ? con_id : devname);
-			return desc;
+
+			goto done;
 		} else {
 			return ERR_PTR(status);
 		}

@@ -4155,6 +4163,9 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 		return ERR_PTR(status);
 	}
 
+done:
+	desc->n_users++;
+
 	return desc;
 }
 EXPORT_SYMBOL_GPL(gpiod_get_index);

drivers/gpio/gpiolib.h +0 -0

index 087d865286a0c..f96eda90281a3 100644

Currently, a GPIO can be requested multiple times when the NONEXCLUSIVE flag is set, however it must still be freed a single time. This makes client code rather complex, since multiple drivers may request the GPIO but only a single one can free it. Rather than manually handling this in each driver add some basic reference counting into the core. Currently, this is fairly primitive but so is the support for the NONEXCLUSIVE flag and the implementation covers those use-cases.

Reported-by Marek Szyprowski <[email protected]>

Signed-off-by Charles Keepax <[email protected]>

Quoted 9 lines · Charles Keepax

Currently, a GPIO can be requested multiple times when the NONEXCLUSIVE flag is set, however it must still be freed a single time. This makes client code rather complex, since multiple drivers may request the GPIO but only a single one can free it. Rather than manually handling this in each driver add some basic reference counting into the core. Currently, this is fairly primitive but so is the support for the NONEXCLUSIVE flag and the implementation covers those use-cases.

Quoted 2 lines

Reported-by Marek Szyprowski <[email protected]>

Signed-off-by Charles Keepax <[email protected]>

This patch is not fixing anything right now, correct?

I discussed the notion of pulling reference counting for nonexclusive GPIOs into gpiolib with Mark but the benefit is a bit unclear: if the subsystem using nonexeclusive GPIOs (currently only regulators) would still have to keep its own reference count or somehow semantically know when the last user is gone, the point is kind of moot.

I haven't looked closely at the regulators case but I got the impression that it is more complex than just reference counting so, currently I don't know if this is such a good idea.

Anyway I would like to push this until we have cleaned up with the rest of the series I have boiling, if you don't mind.

(Patch 1+2 should be fine anyway.)

Yours,
Linus Walleij

It's fixing something in the case of two regulators using the same GPIO. The direction this patch chain takes is that the end drivers own the GPIOs not the regulator core (except for the legacy case). So both of the end drivers will devm_ request their

Quoted 4 lines · Charles Keepax

It's fixing something in the case of two regulators using the same GPIO. The direction this patch chain takes is that the end drivers own the GPIOs not the regulator core (except for the legacy case). So both of the end drivers will devm_ request their

The situation with descriptor based GPIOs is a bug - the reason why the core does the request in the legacy case is precisely the problem Linus identified. Once a GPIO is shared all the users of the GPIO need to coordinate with each other in order to set the value so the refcount by itself in the GPIO core isn't super useful unless it can also serve to help the multiple users find each other somehow. I think what we want to do here is either push the gpiod requests into the regulator core or change things so that once the regulator is registered with the regulator core the regulator core owns and is responsible for freeing the regulator.

Quoted 5 lines · Mark Brown

help the multiple users find each other somehow. I think what we want to do here is either push the gpiod requests into the regulator core or change things so that once the regulator is registered with the regulator core the regulator core owns and is responsible for freeing the regulator.

On the co-ordinating do we expect that the behaviour will be that the GPIO should in the "enabled" state whenever any regulator is requesting it? IE. the GPIO state is an OR of the regulator states. Or are we expecting to handle more complex interaction?

Quoted 8 lines · Charles Keepax

Quoted 5 lines · Mark Brown

help the multiple users find each other somehow. I think what we want to do here is either push the gpiod requests into the regulator core or change things so that once the regulator is registered with the regulator core the regulator core owns and is responsible for freeing the regulator.

Quoted 5 lines

On the co-ordinating do we expect that the behaviour will be that the GPIO should in the "enabled" state whenever any regulator is requesting it? IE. the GPIO state is an OR of the regulator states. Or are we expecting to handle more complex interaction?

For the regulators that's what we do, yes - it's like they're all sharing a single regulator. That probably won't be true in general for all GPIO users.

Would there perhaps be milage in looking at just making the regulator core request the GPIO, rather than the end drivers? Gives us a single request/free point. We don't need any special flags in the GPIO layer, as its just a single user as far as GPIO is concerned.

Quoted 5 lines · Charles Keepax

Would there perhaps be milage in looking at just making the regulator core request the GPIO, rather than the end drivers? Gives us a single request/free point. We don't need any special flags in the GPIO layer, as its just a single user as far as GPIO is concerned.

Yes, I did suggest that earlier in the thread :/

The situation with descriptor based GPIOs is a bug - the reason why the core does the request in the legacy case is precisely the problem Linus identified. Once a GPIO is shared all the users of the GPIO need to coordinate with each other in order to set the value so the refcount by itself in the GPIO core isn't super useful unless it can also serve to help the multiple users find each other somehow. I think what we want to do here is either push the gpiod requests into the regulator core or change things so that once the regulator is registered with the regulator core the regulator core owns and is responsible for freeing the regulator.

On Fri, Nov 23, 2018 at 2:25 PM Mark Brown <[email protected]> wrote:
I suspect maybe the lesser evil is to bite the bullet, invent gpiod_get_from_of_node() which is the missing API (we currently only have devm_gpiod_get_from_of_node()) and simply
fix up the converted regulator drivers to avoid devm_* retrieveal in the same manner as wm8994 (the already
queued patch). This will make the regulator core own the refcounting as it does today.

It's a bit unelegant but it's very straight forward and I know I can fix it up qucikly.

Unless anyone thinks it's a bad idea I will try to make a small fix series like that and a GPIO patch you can also carry in the regulator tree with it.

I suspect maybe the lesser evil is to bite the bullet, invent gpiod_get_from_of_node() which is the missing API (we currently only have devm_gpiod_get_from_of_node()) and simply fix up the converted regulator drivers to avoid devm_* retrieveal in the same manner as wm8994 (the already queued patch). This will make the regulator core own the refcounting as it does today.

It's a bit unelegant but it's very straight forward and I know I can fix it up qucikly.

The managed resources pretty much assume
that you tie resources to the device model and let kref inside the kobject in struct device do all refcounting and that essentially collides with the refcounting inside the regulator core, they both want to control this now.

I suspect maybe the lesser evil is to bite the bullet, invent gpiod_get_from_of_node() which is the missing API (we currently only have devm_gpiod_get_from_of_node()) and simply fix up the converted regulator drivers to avoid devm_* retrieveal in the same manner as wm8994 (the already queued patch). This will make the regulator core own the refcounting as it does today.

This reverts commit 466affa06703 ("regulator: wm8994: Don't use devres for enable GPIOs"). Whilst that did work around the issue in question there are complications on the error paths of regulator_register. In the success case clearly the regulator core owns the GPIO, however, in the failure case it is unclear what should happen. The regulator core could always free the GPIO but that may not make sense from the perspective of the calling code. Alternatively the regulator core could never free the GPIO but that makes the handling in the regulator core rather complex. Ultimately it makes more sense to handle the shared GPIO in the core.

Reported-by Marek Szyprowski <[email protected]>

Signed-off-by Charles Keepax <[email protected]>

---
 drivers/regulator/wm8994-regulator.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

drivers/regulator/wm8994-regulator.c +5 -8

index 46e6b4ee1491a..d7fec533c4032 100644
@@ -147,14 +147,11 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
 	config.regmap = wm8994->regmap;
 	config.init_data = &ldo->init_data;
 
-	/*
-	 * Look up LDO enable GPIO from the parent device node, we don't
-	 * use devm because the regulator core will free the GPIO
-	 */
-	gpiod = gpiod_get_optional(pdev->dev.parent,
-				   id ? "wlf,ldo2ena" : "wlf,ldo1ena",
-				   GPIOD_OUT_LOW |
-				   GPIOD_FLAGS_BIT_NONEXCLUSIVE);
+	/* Look up LDO enable GPIO from the parent device node */
+	gpiod = devm_gpiod_get_optional(pdev->dev.parent,
+					id ? "wlf,ldo2ena" : "wlf,ldo1ena",
+					GPIOD_OUT_LOW |
+					GPIOD_FLAGS_BIT_NONEXCLUSIVE);
 	if (IS_ERR(gpiod))
 		return PTR_ERR(gpiod);
 	config.ena_gpiod = gpiod;