[PATCH 00/18] pwm: Provide devm_pwmchip_alloc() function

[PATCH 00/18] pwm: Provide devm_pwmchip_alloc() function

[Uwe Kleine-König]

Hello,

my eventual goal is to provide a chardev API to PWMs similar to the gpioctl
API. For that to work flawlessly it's required that a pwmchip stays
around while the corresponding device is opened even if the respective
lowlevel driver goes away. See Wolfram's EOSS talk[1] for some more
details.

This series provides a new function devm_pwmchip_alloc() that allocates
a struct pwm_chip together with driver data. Currently this is still
using devm_kzalloc and so goes away when the device is unbound from the
driver. However this can be changed without having to touch all drivers
again.

The function devm_pwmchip_alloc() is modelled after similar functions
from spi, counter and networking code. The first patch provides the
allocator function and an accessor for driver data, the following
patches convert a subset of the available drivers to this new API.

The series is fully bisectable and the only interdependency is that
patch #1 is needed for all other patches. The idea is to complete
conversion of all remaining drivers and then add a struct device to
struct pwm_chip and so make pwm_chip reference counted. There are still
a few more drivers to convert, but I thought to send out the current
patch set to get some early feedback.

The base for this series is v6.5-rc1 plus the following series:

[PATCH v2 0/8] pwm: Get rid of pwm_[sg]et_chip_data()
        20230705080650.2353391-1-u.kleine-koenig@pengutronix.de
[PATCH 0/2] pwm: stm32: A (small) fix and a cleanup
        20230713155142.2454010-1-u.kleine-koenig@pengutronix.de
[PATCH 00/10] pwm: Constistenly name pwm_chip variables "chip"
        20230714205623.2496590-1-u.kleine-koenig@pengutronix.de
[PATCH] staging: greybus: pwm: Drop unused member from driver struct
        20230714201622.2490792-1-u.kleine-koenig@pengutronix.de
[PATCH] pwm: lpc18xx-sct: Simplify using devm_clk_get_enabled()
        20230718144128.371818-1-u.kleine-koenig@pengutronix.de
[PATCH] pwm: lpc32xx: remove handling of PWM channels
        20230717155257.2568627-1-u.kleine-koenig@pengutronix.de
[PATCH] pwm: pxa: Don't reimplement of_device_get_match_data()
        20230718150657.1728166-1-u.kleine-koenig@pengutronix.de
[PATCH] pwm: ntxec: Drop a write-only variable from driver data
        20230718152327.2583886-1-u.kleine-koenig@pengutronix.de
[PATCH] gpio: mvebu: Make use of devm_pwmchip_add
        20230717142743.2555739-1-u.kleine-koenig@pengutronix.de
[PATCH] pwm: ntxec: Use device_set_of_node_from_dev()
        20230718175310.3946687-1-u.kleine-koenig@pengutronix.de
[PATCH] pwm: berlin: Simplify using devm functions
        20230718175545.3946935-1-u.kleine-koenig@pengutronix.de

I'm not sure the build bots can properly handle that, so it would be
great to get these base series into next soon.

Best regards
Uwe

[1] https://static.sched.com/hosted_files/eoss2023/e3/LifecycleIssues_WolframSang_2023.pdf

Uwe Kleine-König (18):
  pwm: Provide devm_pwmchip_alloc() function
  pwm: ab8500: Make use of devm_pwmchip_alloc() function
  pwm: apple: Make use of devm_pwmchip_alloc() function
  pwm: berlin: Make use of devm_pwmchip_alloc() function
  pwm: clk: Make use of devm_pwmchip_alloc() function
  pwm: fsl-ftm: Make use of devm_pwmchip_alloc() function
  pwm: hibvt: Make use of devm_pwmchip_alloc() function
  pwm: imx1: Make use of devm_pwmchip_alloc() function
  pwm: imx27: Make use of devm_pwmchip_alloc() function
  pwm: jz4740: Make use of devm_pwmchip_alloc() function
  pwm: keembay: Make use of devm_pwmchip_alloc() function
  pwm: lpc18xx-sct: Make use of devm_pwmchip_alloc() function
  pwm: lpc32xx: Make use of devm_pwmchip_alloc() function
  pwm: mxs: Make use of devm_pwmchip_alloc() function
  pwm: ntxec: Make use of devm_pwmchip_alloc() function
  pwm: pxa: Make use of devm_pwmchip_alloc() function
  pwm: stm32: Make use of devm_pwmchip_alloc() function
  gpio: mvebu: Make use of devm_pwmchip_alloc() function

 .../driver-api/driver-model/devres.rst        |  1 +
 Documentation/driver-api/pwm.rst              | 10 ++--
 drivers/gpio/gpio-mvebu.c                     | 17 +++----
 drivers/pwm/core.c                            | 23 +++++++++
 drivers/pwm/pwm-ab8500.c                      | 17 +++----
 drivers/pwm/pwm-apple.c                       | 17 +++----
 drivers/pwm/pwm-berlin.c                      | 28 ++++++-----
 drivers/pwm/pwm-clk.c                         | 26 +++++-----
 drivers/pwm/pwm-fsl-ftm.c                     | 47 ++++++++++---------
 drivers/pwm/pwm-hibvt.c                       | 26 +++++-----
 drivers/pwm/pwm-imx1.c                        | 16 +++----
 drivers/pwm/pwm-imx27.c                       | 19 ++++----
 drivers/pwm/pwm-jz4740.c                      | 25 +++++-----
 drivers/pwm/pwm-keembay.c                     | 16 +++----
 drivers/pwm/pwm-lpc18xx-sct.c                 | 24 +++++-----
 drivers/pwm/pwm-lpc32xx.c                     | 18 +++----
 drivers/pwm/pwm-mxs.c                         | 19 ++++----
 drivers/pwm/pwm-ntxec.c                       | 27 +++++------
 drivers/pwm/pwm-pxa.c                         | 20 ++++----
 drivers/pwm/pwm-stm32.c                       | 42 +++++++++++------
 include/linux/pwm.h                           |  4 ++
 21 files changed, 249 insertions(+), 193 deletions(-)


base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
prerequisite-patch-id: c856e0baabfc22d250b7ce881427cdb74613e69a
prerequisite-patch-id: 2a5f9c04e4b5794b5a5d0b30280f75b76a05092b
prerequisite-patch-id: a48b05b94b61f8029b4bb9a78ae1f2cd8c476d80
prerequisite-patch-id: c7dfd3798f024b27b6e236da1eae40b79bb3e281
prerequisite-patch-id: 313ada72ab57438a8d54df0df9c0926bb4f69b36
prerequisite-patch-id: 318824c08f3e7d6500e3c5a47a11c5daffaea34a
prerequisite-patch-id: ea75bfd48ea4d0132c637172564bc1a57061377a
prerequisite-patch-id: 278b25a8d5fef49f7e5c46b627d4862d0b24baaf
prerequisite-patch-id: a3b11e0a7c8f6564e668e2ed1b637351e5fd9dd2
prerequisite-patch-id: 55588b25ea7ce69d716a33a7aaed662d75bb9687
prerequisite-patch-id: 399d7e94bafc5dc970a0f213745e4982066b8583
prerequisite-patch-id: 87aca1beb6efdaab95ddce7e2d9eed89a89252a1
prerequisite-patch-id: e13e3db807f2c5b49180e24492f496f8e945e42d
prerequisite-patch-id: 8712b17dee2a5f2441908e2de260ceec8ce6ff37
prerequisite-patch-id: 987b0a351801b1e361932beeeedbc5245540037c
prerequisite-patch-id: 869d3932983f3ae6a8982e186b27df11c56e9e5e
prerequisite-patch-id: 4f60b0afb435011b58b7cc6bead1d385db7d8e11
prerequisite-patch-id: 0e86da52c02300d407d4c0b6f2b9f8293e3320dc
prerequisite-patch-id: a731856f5d7dee7f3465f021e5c56981a803b22d
prerequisite-patch-id: b759d036cf578083ec76aa7ba01dd8643667d4f6
prerequisite-patch-id: 9abd8b16e74625e2630655da7d22e75bbe0c6231
prerequisite-patch-id: 6ad03ceb505a293f2308235161c54ff6b508e59f
prerequisite-patch-id: cb6d75be9b72cc04069b6952ba9e5ac80a26a1ab
prerequisite-patch-id: c8fa7296a736f42ec49ab1334cb19947d647a2b4
prerequisite-patch-id: b601ea695815fb65ed704349302ba66442277fc3
prerequisite-patch-id: 7a0daa2918f8a317333e57c1d1698078a1968720
prerequisite-patch-id: b6762f6deeb74aaf73afe2d8dd816d48cec4e1ee
prerequisite-patch-id: a0c8d63424241e64c3e5f9991ea04018a51fcc94
-- 
2.39.2

[PATCH 18/18] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm sub-driver to further changes of the pwm core
outlined in the commit introducing devm_pwmchip_alloc(). There is no
intended semantical change and the driver should behave as before.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/gpio/gpio-mvebu.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index a35958e7adf6..9557cac807f9 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -98,7 +98,6 @@ struct mvebu_pwm {
 	u32			 offset;
 	unsigned long		 clk_rate;
 	struct gpio_desc	*gpiod;
-	struct pwm_chip		 chip;
 	spinlock_t		 lock;
 	struct mvebu_gpio_chip	*mvchip;
 
@@ -614,7 +613,7 @@ static const struct regmap_config mvebu_gpio_regmap_config = {
  */
 static struct mvebu_pwm *to_mvebu_pwm(struct pwm_chip *chip)
 {
-	return container_of(chip, struct mvebu_pwm, chip);
+	return pwmchip_priv(chip);
 }
 
 static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -789,6 +788,7 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 {
 	struct device *dev = &pdev->dev;
 	struct mvebu_pwm *mvpwm;
+	struct pwm_chip *chip;
 	void __iomem *base;
 	u32 offset;
 	u32 set;
@@ -813,9 +813,11 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 	if (IS_ERR(mvchip->clk))
 		return PTR_ERR(mvchip->clk);
 
-	mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
-	if (!mvpwm)
+	chip = devm_pwmchip_alloc(dev, sizeof(struct mvebu_pwm));
+	if (!chip)
 		return -ENOMEM;
+	mvpwm = pwmchip_priv(chip);
+
 	mvchip->mvpwm = mvpwm;
 	mvpwm->mvchip = mvchip;
 	mvpwm->offset = offset;
@@ -868,13 +870,12 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	mvpwm->chip.dev = dev;
-	mvpwm->chip.ops = &mvebu_pwm_ops;
-	mvpwm->chip.npwm = mvchip->chip.ngpio;
+	chip->ops = &mvebu_pwm_ops;
+	chip->npwm = mvchip->chip.ngpio;
 
 	spin_lock_init(&mvpwm->lock);
 
-	return devm_pwmchip_add(dev, &mvpwm->chip);
+	return devm_pwmchip_add(dev, chip);
 }
 
 #ifdef CONFIG_DEBUG_FS
-- 
2.39.2

Re: [PATCH 18/18] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Bartosz Golaszewski]

On Tue, Jul 18, 2023 at 8:19 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> This prepares the pwm sub-driver to further changes of the pwm core
> outlined in the commit introducing devm_pwmchip_alloc(). There is no
> intended semantical change and the driver should behave as before.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/gpio/gpio-mvebu.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index a35958e7adf6..9557cac807f9 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -98,7 +98,6 @@ struct mvebu_pwm {
>         u32                      offset;
>         unsigned long            clk_rate;
>         struct gpio_desc        *gpiod;
> -       struct pwm_chip          chip;
>         spinlock_t               lock;
>         struct mvebu_gpio_chip  *mvchip;
>
> @@ -614,7 +613,7 @@ static const struct regmap_config mvebu_gpio_regmap_config = {
>   */
>  static struct mvebu_pwm *to_mvebu_pwm(struct pwm_chip *chip)
>  {
> -       return container_of(chip, struct mvebu_pwm, chip);
> +       return pwmchip_priv(chip);
>  }
>
>  static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -789,6 +788,7 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
>  {
>         struct device *dev = &pdev->dev;
>         struct mvebu_pwm *mvpwm;
> +       struct pwm_chip *chip;
>         void __iomem *base;
>         u32 offset;
>         u32 set;
> @@ -813,9 +813,11 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
>         if (IS_ERR(mvchip->clk))
>                 return PTR_ERR(mvchip->clk);
>
> -       mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
> -       if (!mvpwm)
> +       chip = devm_pwmchip_alloc(dev, sizeof(struct mvebu_pwm));
> +       if (!chip)
>                 return -ENOMEM;
> +       mvpwm = pwmchip_priv(chip);
> +
>         mvchip->mvpwm = mvpwm;
>         mvpwm->mvchip = mvchip;
>         mvpwm->offset = offset;
> @@ -868,13 +870,12 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
>                 return -EINVAL;
>         }
>
> -       mvpwm->chip.dev = dev;
> -       mvpwm->chip.ops = &mvebu_pwm_ops;
> -       mvpwm->chip.npwm = mvchip->chip.ngpio;
> +       chip->ops = &mvebu_pwm_ops;
> +       chip->npwm = mvchip->chip.ngpio;
>
>         spin_lock_init(&mvpwm->lock);
>
> -       return devm_pwmchip_add(dev, &mvpwm->chip);
> +       return devm_pwmchip_add(dev, chip);
>  }
>
>  #ifdef CONFIG_DEBUG_FS
> --
> 2.39.2
>

Looks good to me (although I have my reservations about the concept of
foo_alloc() for subsystems in the kernel...). How do you want this to
go upstream?

Bart

Re: [PATCH 18/18] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Bartosz,

On Sat, Jul 29, 2023 at 04:09:40PM +0200, Bartosz Golaszewski wrote:
> Looks good to me (although I have my reservations about the concept of
> foo_alloc() for subsystems in the kernel...).

Wolfram's EOSS talk[1] mentioned "__cleanup__ + kref as suggested by Bartosz?
Paradigm shift, probably looong way to go". I guess that's what you'd
prefer? Do you have a link for me to read about this?

> How do you want this to go upstream?

I haven't thought about that yet. I first will have to convince
Thierry that this is a good idea I guess. This version will not be
merged for sure.

Best regards
Uwe

[1] https://static.sched.com/hosted_files/eoss2023/e3/LifecycleIssues_WolframSang_2023.pdf

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 18/18] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Bartosz Golaszewski]

On Sat, Jul 29, 2023 at 11:37 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Bartosz,
>
> On Sat, Jul 29, 2023 at 04:09:40PM +0200, Bartosz Golaszewski wrote:
> > Looks good to me (although I have my reservations about the concept of
> > foo_alloc() for subsystems in the kernel...).
>
> Wolfram's EOSS talk[1] mentioned "__cleanup__ + kref as suggested by Bartosz?
> Paradigm shift, probably looong way to go". I guess that's what you'd
> prefer? Do you have a link for me to read about this?
>

For now I prefer the gpiolib model. One structure allocated and
controlled by the driver (struct gpio_chip) which needs to live only
as long as the device is bound to a driver and a second structure
private to the subsystem, allocated and controlled by the subsystem
(struct gpio_device) which also contains the referenced counted struct
device and is only released by the device's release callback.

IMO there shouldn't be any need for PWM drivers to dereference struct
device held by struct pwm_chip. If anything - it should be passed to
the drivers in subsystem callbacks.

I may be wrong of course, I don't know this subsystem very well but it
seems to follow a pattern that's pretty common in the kernel and
causes ownership confusion.

Bart

> > How do you want this to go upstream?
>
> I haven't thought about that yet. I first will have to convince
> Thierry that this is a good idea I guess. This version will not be
> merged for sure.
>
> Best regards
> Uwe
>
> [1] https://static.sched.com/hosted_files/eoss2023/e3/LifecycleIssues_WolframSang_2023.pdf
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 18/18] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Bart,

On Sun, Jul 30, 2023 at 12:07:33PM +0200, Bartosz Golaszewski wrote:
> On Sat, Jul 29, 2023 at 11:37 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Sat, Jul 29, 2023 at 04:09:40PM +0200, Bartosz Golaszewski wrote:
> > > Looks good to me (although I have my reservations about the concept of
> > > foo_alloc() for subsystems in the kernel...).
> >
> > Wolfram's EOSS talk[1] mentioned "__cleanup__ + kref as suggested by Bartosz?
> > Paradigm shift, probably looong way to go". I guess that's what you'd
> > prefer? Do you have a link for me to read about this?
> >
> 
> For now I prefer the gpiolib model. One structure allocated and
> controlled by the driver (struct gpio_chip) which needs to live only
> as long as the device is bound to a driver and a second structure
> private to the subsystem, allocated and controlled by the subsystem
> (struct gpio_device) which also contains the referenced counted struct
> device and is only released by the device's release callback.
> 
> IMO there shouldn't be any need for PWM drivers to dereference struct
> device held by struct pwm_chip. If anything - it should be passed to
> the drivers in subsystem callbacks.
> 
> I may be wrong of course, I don't know this subsystem very well but it
> seems to follow a pattern that's pretty common in the kernel and
> causes ownership confusion.

A difficulty I see is that as of now the ops-pointer is maintained in
driver-allocated data. So it's not possible to call the .free callback
once the driver is gone. So either unbinding the driver must be delayed
until all consumers are gone, or the reference to a PWM that a consumer
holds must be invalidated. Both options are not optimal. But I have to
admit that I didn't grasp the device core completely (yet?), so I might
well miss something.

Also I like the concept of "..._alloc" and find it clear enough. I'm not
aware of "ownership confusions". I'm open to hear about these if you
have something to point out.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 18/18] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Bart,

On Sun, Jul 30, 2023 at 12:07:33PM +0200, Bartosz Golaszewski wrote:
> On Sat, Jul 29, 2023 at 11:37 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello Bartosz,
> >
> > On Sat, Jul 29, 2023 at 04:09:40PM +0200, Bartosz Golaszewski wrote:
> > > Looks good to me (although I have my reservations about the concept of
> > > foo_alloc() for subsystems in the kernel...).
> >
> > Wolfram's EOSS talk[1] mentioned "__cleanup__ + kref as suggested by Bartosz?
> > Paradigm shift, probably looong way to go". I guess that's what you'd
> > prefer? Do you have a link for me to read about this?
> >
> 
> For now I prefer the gpiolib model. One structure allocated and
> controlled by the driver (struct gpio_chip) which needs to live only
> as long as the device is bound to a driver and a second structure
> private to the subsystem, allocated and controlled by the subsystem
> (struct gpio_device) which also contains the referenced counted struct
> device and is only released by the device's release callback.

The issue I want to fix for pwm (but don't know yet how to do) is: What
should happen to PWMs that are requested by a consumer when the PWM
driver goes away.

I looked into how gpio does it, and I think the "solution" there is:

	dev_crit(&gdev->dev,
		 "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");

introduced in e1db1706c86e ("gpio: gpiolib: set gpiochip_remove retval
to void"). (But the problem is actually older because returning -EBUSY
as done before is bad, too) I'd hope this could be done better?!

While trying to understand how gpio works, I found a few issues that are
(I think) fixable with the gpiolib model:

 - gpiochip_add_data_with_key() calls device_initialize(&gdev->dev) and
   has later error paths that don't do device_put() but kfree gdev.

 - the locking scheme in gpiod_request_commit() looks strange. gpio_lock
   is released and retaken possibly several times. I wonder what it
   actually protects there. Maybe doing

	diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
	index edab00c9cb3c..496b1cebba58 100644
	--- a/drivers/gpio/gpiolib.c
	+++ b/drivers/gpio/gpiolib.c
	@@ -2064,13 +2064,11 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
				goto out_free_unlock;
			}
		}
	+	spin_unlock_irqrestore(&gpio_lock, flags);
		if (gc->get_direction) {
			/* gc->get_direction may sleep */
	-		spin_unlock_irqrestore(&gpio_lock, flags);
			gpiod_get_direction(desc);
	-		spin_lock_irqsave(&gpio_lock, flags);
		}
	-	spin_unlock_irqrestore(&gpio_lock, flags);
		return 0;
	 
	 out_free_unlock:

   simplifies the code and given that gpiod_get_direction() rechecks
   gc->get_direction unlocked I don't think we'd loose anything here.

 - there is a race condition: gpiochip_remove() takes &gdev->sem when
   invalidating gdev->chip and calling gpiochip_set_data(), but the
   various gpio API functions calling VALIDATE_DESC_VOID don't hold
   &gdev->sem, so gpiochip_remove() might clean gdev->chip just between
   a consumer calling VALIDATE_DESC_VOID(desc) and
   WARN_ON(desc->gdev->chip->can_sleep) (e.g. in gpiod_set_value).

> IMO there shouldn't be any need for PWM drivers to dereference struct
> device held by struct pwm_chip. If anything - it should be passed to
> the drivers in subsystem callbacks.

I don't understand this. I think we agree that a PWM driver shouldn't
have to care about the devices's lifetimes. It's difficult enough to get
this right on the subsystem level.

> I may be wrong of course, I don't know this subsystem very well but it
> seems to follow a pattern that's pretty common in the kernel and
> causes ownership confusion.

Yes that's common. I think another thing that's common though is that
device lifetime isn't properly handled, and while I don't consider
myself as an expert here, the above makes me consider that gpio is no
exception here. So I doubt it serves as a good example to copy from.

Having said that I think the ..._alloc approach is easy enough for
subsystem drivers. Also for pwm we only need a devm_... variant, so
getting the driver part right is really easy.

And given that ..._alloc makes it easier for a subsystem core to do
things right (as it only has to handle a single data structure that
lives long enough) that's what I did here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 18/18] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Andy Shevchenko]

On Thu, Aug 03, 2023 at 11:42:12AM +0200, Uwe Kleine-König wrote:
> On Sun, Jul 30, 2023 at 12:07:33PM +0200, Bartosz Golaszewski wrote:

...

>  - the locking scheme in gpiod_request_commit() looks strange. gpio_lock
>    is released and retaken possibly several times. I wonder what it
>    actually protects there. Maybe doing
> 
> 	diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> 	index edab00c9cb3c..496b1cebba58 100644
> 	--- a/drivers/gpio/gpiolib.c
> 	+++ b/drivers/gpio/gpiolib.c
> 	@@ -2064,13 +2064,11 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> 				goto out_free_unlock;
> 			}
> 		}
> 	+	spin_unlock_irqrestore(&gpio_lock, flags);
> 		if (gc->get_direction) {
> 			/* gc->get_direction may sleep */
> 	-		spin_unlock_irqrestore(&gpio_lock, flags);
> 			gpiod_get_direction(desc);
> 	-		spin_lock_irqsave(&gpio_lock, flags);
> 		}
> 	-	spin_unlock_irqrestore(&gpio_lock, flags);
> 		return 0;
> 	 
> 	 out_free_unlock:
> 
>    simplifies the code and given that gpiod_get_direction() rechecks
>    gc->get_direction unlocked I don't think we'd loose anything here.

Wouldn't this break sleeping bus accesses (I2C gpio expanders, etc)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 18/18] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Andy,

On Thu, Aug 03, 2023 at 02:51:40PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:42:12AM +0200, Uwe Kleine-König wrote:
> > On Sun, Jul 30, 2023 at 12:07:33PM +0200, Bartosz Golaszewski wrote:
> 
> ...
> 
> >  - the locking scheme in gpiod_request_commit() looks strange. gpio_lock
> >    is released and retaken possibly several times. I wonder what it
> >    actually protects there. Maybe doing
> > 
> > 	diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > 	index edab00c9cb3c..496b1cebba58 100644
> > 	--- a/drivers/gpio/gpiolib.c
> > 	+++ b/drivers/gpio/gpiolib.c
> > 	@@ -2064,13 +2064,11 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> > 				goto out_free_unlock;
> > 			}
> > 		}
> > 	+	spin_unlock_irqrestore(&gpio_lock, flags);
> > 		if (gc->get_direction) {
> > 			/* gc->get_direction may sleep */
> > 	-		spin_unlock_irqrestore(&gpio_lock, flags);
> > 			gpiod_get_direction(desc);
> > 	-		spin_lock_irqsave(&gpio_lock, flags);
> > 		}
> > 	-	spin_unlock_irqrestore(&gpio_lock, flags);
> > 		return 0;
> > 	 
> > 	 out_free_unlock:
> > 
> >    simplifies the code and given that gpiod_get_direction() rechecks
> >    gc->get_direction unlocked I don't think we'd loose anything here.
> 
> Wouldn't this break sleeping bus accesses (I2C gpio expanders, etc)?

This question is too short for me to understand what you think. The
only difference my patch does is that gc->get_direction is checked
without holding the lock and a lock+unlock pair. I don't see how this is
relevant to sleeping bus accesses.

	lock()
	...
	if (A) {
		unlock()
		something()
		lock()
	}
	unlock()

is nearly identical to:

	lock()
	...
	unlock()
	if (A) {
		something()
	}
	lock()
	unlock()

which in turn is nearly identical to

	lock()
	...
	unlock()
	if (A) {
		something()
	}

. But I might well miss something, as the "nearly"s above sometimes are
relevant.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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