[PATCH 1/3] regulator: wm8994: Revert back to using devres

[PATCH 1/3] regulator: wm8994: Revert back to using devres

[Charles Keepax]

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 <m.szyprowski@samsung.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/regulator/wm8994-regulator.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
index 46e6b4ee1491a..d7fec533c4032 100644
--- a/drivers/regulator/wm8994-regulator.c
+++ b/drivers/regulator/wm8994-regulator.c
@@ -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;
@@ -187,7 +184,6 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
 	return 0;
 
 err:
-	gpiod_put(gpiod);
 	return ret;
 }
 
-- 
2.11.0

[PATCH 2/3] regulator: Only free GPIOs if the core requested them

[Charles Keepax]

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 <m.szyprowski@samsung.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/regulator/core.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index dbe2f2e6e6254..9da7d27c7145e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -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;
@@ -2270,7 +2272,8 @@ static void regulator_ena_gpio_free(struct regulator_dev *rdev)
 		if (pin->gpiod == rdev->ena_pin->gpiod) {
 			if (pin->request_count <= 1) {
 				pin->request_count = 0;
-				gpiod_put(pin->gpiod);
+				if (pin->locally_requested)
+					gpiod_put(pin->gpiod);
 				list_del(&pin->list);
 				kfree(pin);
 				rdev->ena_pin = NULL;
-- 
2.11.0

Re: [PATCH 2/3] regulator: Only free GPIOs if the core requested them

[Linus Walleij]

On Thu, Nov 22, 2018 at 6:30 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:

> 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 <m.szyprowski@samsung.com>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

This patch is slightly prettier than mine.

Thanks!
Linus Walleij

Re: [PATCH 2/3] regulator: Only free GPIOs if the core requested them

[Marek Szyprowski]

Hi Charles,

On 2018-11-22 18:30, Charles Keepax wrote:
> 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 <m.szyprowski@samsung.com>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
>  drivers/regulator/core.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index dbe2f2e6e6254..9da7d27c7145e 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -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;
> @@ -2270,7 +2272,8 @@ static void regulator_ena_gpio_free(struct regulator_dev *rdev)
>  		if (pin->gpiod == rdev->ena_pin->gpiod) {
>  			if (pin->request_count <= 1) {
>  				pin->request_count = 0;
> -				gpiod_put(pin->gpiod);
> +				if (pin->locally_requested)
> +					gpiod_put(pin->gpiod);
>  				list_del(&pin->list);
>  				kfree(pin);
>  				rdev->ena_pin = NULL;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 2/3] regulator: Only free GPIOs if the core requested them

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

On Thu, Nov 22, 2018 at 05:30:14PM +0000, Charles Keepax wrote:
> 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.

I think this is fine in conjunction with patch 3 in that adding that
patch sorts out the double free problems with shared regulators but
without that pushing the GPIO management into the individual regulator
drivers is going to create trouble as something needs to coordinate to
make sure that we only free when the last user is gone.

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] regulator: Only free GPIOs if the core requested them

[Linus Walleij]

On Fri, Nov 23, 2018 at 3:25 PM Mark Brown <broonie@kernel.org> wrote:

> 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.

I have made the "real" (or so I think) solution to this problem which
essentially hardens the core so that it very explicitly takes over the
descriptor from the driver.

I'm just having it spin a bit around the 0day build servers (they
don't seem to respond right now). I built all patched drivers
I could easily configure in locally.

I am planning to respin the rest of the cleanups making the core only
handle descriptors on top of it.

Yours,
Linus Walleij

[PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[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.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/gpio/gpiolib.c | 13 ++++++++++++-
 drivers/gpio/gpiolib.h |  2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 230e41562462b..42ba86fb495a5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 087d865286a0c..f96eda90281a3 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -230,6 +230,8 @@ struct gpio_desc {
 	const char		*label;
 	/* Name of the GPIO */
 	const char		*name;
+
+	unsigned int		n_users;
 };
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
-- 
2.11.0

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Marek Szyprowski]

Hi Charles,

On 2018-11-22 18:30, Charles Keepax wrote:
> 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 <m.szyprowski@samsung.com>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpio/gpiolib.c | 13 ++++++++++++-
>  drivers/gpio/gpiolib.h |  2 ++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 230e41562462b..42ba86fb495a5 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -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);
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 087d865286a0c..f96eda90281a3 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -230,6 +230,8 @@ struct gpio_desc {
>  	const char		*label;
>  	/* Name of the GPIO */
>  	const char		*name;
> +
> +	unsigned int		n_users;
>  };
>  
>  int gpiod_request(struct gpio_desc *desc, const char *label);

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Linus Walleij]

On Thu, Nov 22, 2018 at 6:30 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:

> 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 <m.szyprowski@samsung.com>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

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

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Charles Keepax]

On Fri, Nov 23, 2018 at 10:40:57AM +0100, Linus Walleij wrote:
> On Thu, Nov 22, 2018 at 6:30 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> 
> > 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 <m.szyprowski@samsung.com>
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> 
> This patch is not fixing anything right now, correct?
> 

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
GPIOs, which means that if those drivers are unbound both of them
will attempt to free the GPIO causing the same double free
warning we saw here. Indeed there is a probably worse problem
that if one is unbound it will free the GPIO and break the second
driver.

Basically the current handling of non-exclusive GPIOs in the GPIO
core requires multiple calls to request the GPIO but only a
single call to free the GPIO. If we want to stick with that
approach, then the regulator core does need to take ownership of
the lifetime of the enable GPIOs and the end drivers can't use
devm, as per my initial patch for wm8994. But it makes getting
the error paths in regulator_register sensible really tricky, and
feels like a less "clean" solution.

> 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.
> 

I don't greatly mind delaying it as I don't currently look after
any systems that share enable GPIOs on regulators, but free of
those GPIOs is currently really broken so it feels like it
shouldn't be kicked too far down the road.

Thanks,
Charles

> (Patch 1+2 should be fine anyway.)
> 
> Yours,
> Linus Walleij

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Fri, Nov 23, 2018 at 10:57:29AM +0000, Charles Keepax wrote:

> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Charles Keepax]

On Fri, Nov 23, 2018 at 01:25:22PM +0000, Mark Brown wrote:
> On Fri, Nov 23, 2018 at 10:57:29AM +0000, Charles Keepax wrote:
> 
> > 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.

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?

Thanks,
Charles

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Mon, Nov 26, 2018 at 01:00:01PM +0000, Charles Keepax wrote:
> On Fri, Nov 23, 2018 at 01:25:22PM +0000, Mark Brown wrote:

> > 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?

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Charles Keepax]

On Mon, Nov 26, 2018 at 02:09:27PM +0000, Mark Brown wrote:
> On Mon, Nov 26, 2018 at 01:00:01PM +0000, Charles Keepax wrote:
> > On Fri, Nov 23, 2018 at 01:25:22PM +0000, Mark Brown wrote:
> 
> > > 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?
> 
> 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.

Thanks,
Charles

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 391 bytes --]

On Mon, Nov 26, 2018 at 02:30:28PM +0000, Charles Keepax wrote:

> 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 :/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Charles Keepax]

On Mon, Nov 26, 2018 at 02:54:28PM +0000, Mark Brown wrote:
> On Mon, Nov 26, 2018 at 02:30:28PM +0000, Charles Keepax wrote:
> 
> > 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 :/

Sorry yes it seems I misread what you had written earlier, I will
have a quick look see what a patch to do this might look like.

Thanks,
Charles

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Linus Walleij]

On Fri, Nov 23, 2018 at 2:25 PM Mark Brown <broonie@kernel.org> wrote:

> 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.

I think we see some problems with devm_* managed resources here
and I see Charles' and your point.

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.

If we try to push the regulator requests into the regulator core
it becomes complex, because that requires at least two different
methods as we move away from the global GPIO numberspace,
and the drivers usually knows best how to request it.

Indeed I imagined it like so when adding descriptor support,
as we do this mostly like that for other resources.

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.

Yours,
Linus Walleij

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Charles Keepax]

On Mon, Nov 26, 2018 at 10:53:40PM +0100, Linus Walleij wrote:
> On Fri, Nov 23, 2018 at 2:25 PM Mark Brown <broonie@kernel.org> 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.

This issue I ran into following that path is it becomes rather
fiddly to have regulator_register behave sensibly with respect to
the GPIO. Obviously in the success case the core now owns the
GPIO and no issues. However, in the failure case it's not clear if
we want to core to release the GPIO or not, and consistently
doing either is fairly hard. But if your happy to have a go at it
I don't have any objections to it as a solution.

Thanks,
Charles

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Linus Walleij]

On Mon, Nov 26, 2018 at 10:53 PM Linus Walleij <linus.walleij@linaro.org> 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.

I cooked a series like this, just taking it for a ride on the
0day build server before sending it out.

Yours,
Linus Walleij

Re: [PATCH 3/3] gpio: Add reference counting for non-exclusive GPIOs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

On Mon, Nov 26, 2018 at 10:53:40PM +0100, Linus Walleij wrote:

> 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.

If a driver is handing ownership of an object over to something else
then devm is always unsuitable.

> 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.

You're always going to need unmanaged versions of functions for use with
things like this I think.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] regulator: wm8994: Revert back to using devres

[Marek Szyprowski]

Hi Charles,

On 2018-11-22 18:30, Charles Keepax wrote:
> 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 <m.szyprowski@samsung.com>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks for sorting this out! Works fine on Trats2:

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
>  drivers/regulator/wm8994-regulator.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
> index 46e6b4ee1491a..d7fec533c4032 100644
> --- a/drivers/regulator/wm8994-regulator.c
> +++ b/drivers/regulator/wm8994-regulator.c
> @@ -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;
> @@ -187,7 +184,6 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err:
> -	gpiod_put(gpiod);
>  	return ret;
>  }
>  

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland