[PATCH v3 000/108] pwm: Fix lifetime issues for pwm_chips

[PATCH v3 000/108] pwm: Fix lifetime issues for pwm_chips

[Uwe Kleine-König]

Hello,

this is v3 of the series improving life-time tracking for PWM chips. The
urgency is gone as device links now work as expected and so all
in-kernel users are fine since commit 2e84dc379200 ("driver core:
Release all resources during unbind before updating device links").

However proper lifetime tracking is a precondition to have robust
character device support, as we cannot kill a userspace process if the
used pwm driver goes away.

Changes since v2:

 - Cc: the relevant maintainers for wider testing/review audience
 - Rebase to v6.7-rc1 + https://lore.kernel.org/linux-pwm/20231121112029.gyv3gqirlycysyr4@pengutronix.de
 - Improvements for things pointed out during review and my own
   findings here and there.
 - Implementation for a few more ioctls in the WIP commit that adds
   character support

To go forward I'd like to get in patches up to #103 (i.e. adding
pwmchip_parent() (#2), devm_pwmchip_alloc() (#37) and the conversions of
the drivers to make use of these additions).

The few commits that touch drivers not living in drivers/pwm (i.e. #36,
#100-#103) can go in either via the pwm tree with the rest, or later
---when the used functions are in---via their trees.

After all in-tree drivers are prepared with the patches up to #103, we
can think about when and how we go on with the remaining bits.

Note that patch #104 breaks all drivers that don't use
devm_pwmchip_alloc(), so this is the commit that needs coordination with the
maintainers of

 drivers/gpio/gpio-mvebu.c
 drivers/gpu/drm/bridge/ti-sn65dsi86.c
 drivers/leds/rgb/leds-qcom-lpg.c
 drivers/staging/greybus/pwm.c

The motivation for this series is the last patch. It allows to control a
pwm device via ioctl. Compared to the sysfs API this already now has
some advantages:

- It changes all parameters in a single call.
  This simplifies things similar to the introduction of
  pwm_apply_state(). With sysfs it can happen that you want to
  switch polarity but that's refused because 

	pwm_get_state(mypwm, &state);
	state.polarity = new_value;

  sometimes yield an invalid state, e.g. because state.period is
  in some cases 0 after bootup. Theoretically it can even happen that you have
  to change two parameters before reaching an applicable state, then you're
  stuck with sysfs.

- It's faster than sysfs. In my measurements with stm32 about a factor
  4.

A userspace lib to make use of this can be found at
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git/ .
It makes use of the character devices if available and falls back to
sysfs. So it's somewhat useful already now.

Best regards
Uwe

Uwe Kleine-König (108):
  pwm: cros-ec: Change prototype of helper to prepare further changes
  pwm: Provide a macro to get the parent device of a given chip
  pwm: ab8500: Make use of pwmchip_parent() macro
  pwm: atmel: Make use of pwmchip_parent() macro
  pwm: atmel-tcb: Make use of pwmchip_parent() macro
  pwm: bcm-kona: Make use of pwmchip_parent() macro
  pwm: crc: Make use of pwmchip_parent() macro
  pwm: cros-ec: Make use of pwmchip_parent() macro
  pwm: dwc: Make use of pwmchip_parent() macro
  pwm: ep93xx: Make use of pwmchip_parent() macro
  pwm: fsl-ftm: Make use of pwmchip_parent() macro
  pwm: img: Make use of parent device pointer in driver data
  pwm: imx27: Make use of pwmchip_parent() macro
  pwm: jz4740: Make use of pwmchip_parent() macro
  pwm: lpc18xx-sct: Make use of parent device pointer in driver data
  pwm: lpss: Make use of pwmchip_parent() macro
  pwm: mediatek: Make use of pwmchip_parent() macro
  pwm: meson: Make use of pwmchip_parent() macro
  pwm: mtk-disp: Make use of pwmchip_parent() macro
  pwm: omap: Make use of pwmchip_parent() macro
  pwm: pca9685: Store parent device in driver data
  pwm: raspberrypi-poe: Make use of pwmchip_parent() macro
  pwm: rcar: Make use of pwmchip_parent() macro
  pwm: rz-mtu3: Make use of pwmchip_parent() macro
  pwm: samsung: Make use of pwmchip_parent() macro
  pwm: sifive: Make use of pwmchip_parent() macro
  pwm: stm32-lp: Make use of pwmchip_parent() macro
  pwm: stm32: Make use of pwmchip_parent() macro
  pwm: stmpe: Make use of pwmchip_parent() macro
  pwm: sun4i: Make use of pwmchip_parent() macro
  pwm: tiecap: Make use of pwmchip_parent() macro
  pwm: tiehrpwm: Make use of pwmchip_parent() macro
  pwm: twl-led: Make use of pwmchip_parent() macro
  pwm: twl: Make use of pwmchip_parent() macro
  pwm: vt8500: Make use of pwmchip_parent() macro
  staging: greybus: pwm: Make use of pwmchip_parent() macro
  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: atmel-hlcdc: Make use of devm_pwmchip_alloc() function
  pwm: atmel: Make use of devm_pwmchip_alloc() function
  pwm: atmel-tcb: Make use of devm_pwmchip_alloc() function
  pwm: bcm2835: Make use of devm_pwmchip_alloc() function
  pwm: bcm-iproc: Make use of devm_pwmchip_alloc() function
  pwm: bcm-kona: Make use of devm_pwmchip_alloc() function
  pwm: berlin: Make use of devm_pwmchip_alloc() function
  pwm: brcmstb: Make use of devm_pwmchip_alloc() function
  pwm: clk: Make use of devm_pwmchip_alloc() function
  pwm: clps711x: Make use of devm_pwmchip_alloc() function
  pwm: crc: Make use of devm_pwmchip_alloc() function
  pwm: cros-ec: Make use of devm_pwmchip_alloc() function
  pwm: dwc: Make use of devm_pwmchip_alloc() function
  pwm: ep93xx: 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: img: 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: imx-tpm: Make use of devm_pwmchip_alloc() function
  pwm: intel-lgm: Make use of devm_pwmchip_alloc() function
  pwm: iqs620a: 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: lp3943: 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: lpss-*: Make use of devm_pwmchip_alloc() function
  pwm: mediatek: Make use of devm_pwmchip_alloc() function
  pwm: meson: Make use of devm_pwmchip_alloc() function
  pwm: microchip-core: Make use of devm_pwmchip_alloc() function
  pwm: mtk-disp: 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: omap-dmtimer: Make use of devm_pwmchip_alloc() function
  pwm: pca9685: Make use of devm_pwmchip_alloc() function
  pwm: pxa: Make use of devm_pwmchip_alloc() function
  pwm: raspberrypi-poe: Make use of devm_pwmchip_alloc() function
  pwm: rcar: Make use of devm_pwmchip_alloc() function
  pwm: renesas-tpu: Make use of devm_pwmchip_alloc() function
  pwm: rockchip: Make use of devm_pwmchip_alloc() function
  pwm: rz-mtu3: Make use of devm_pwmchip_alloc() function
  pwm: samsung: Make use of devm_pwmchip_alloc() function
  pwm: sifive: Make use of devm_pwmchip_alloc() function
  pwm: sl28cpld: Make use of devm_pwmchip_alloc() function
  pwm: spear: Make use of devm_pwmchip_alloc() function
  pwm: sprd: Make use of devm_pwmchip_alloc() function
  pwm: sti: Make use of devm_pwmchip_alloc() function
  pwm: stm32-lp: Make use of devm_pwmchip_alloc() function
  pwm: stm32: Make use of devm_pwmchip_alloc() function
  pwm: stmpe: Make use of devm_pwmchip_alloc() function
  pwm: sun4i: Make use of devm_pwmchip_alloc() function
  pwm: sunplus: Make use of devm_pwmchip_alloc() function
  pwm: tegra: Make use of devm_pwmchip_alloc() function
  pwm: tiecap: Make use of devm_pwmchip_alloc() function
  pwm: twl-led: Make use of devm_pwmchip_alloc() function
  pwm: twl: Make use of devm_pwmchip_alloc() function
  pwm: visconti: Make use of devm_pwmchip_alloc() function
  pwm: vt8500: Make use of devm_pwmchip_alloc() function
  pwm: xilinx: Make use of devm_pwmchip_alloc() function
  gpio: mvebu: Make use of devm_pwmchip_alloc() function
  drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function
  leds: qcom-lpg: Make use of devm_pwmchip_alloc() function
  staging: greybus: pwm: Make use of devm_pwmchip_alloc() function
  pwm: Ensure that pwm_chips are allocated using pwmchip_alloc()
  pwm: Ensure a struct pwm has the same lifetime as its pwm_chip
  pwm: Ensure the memory backing a PWM chip isn't freed while used
  pwm: Add more locking
  WIP: pwm: Add support for pwmchip devices for faster and easier
    userspace access

 .../driver-api/driver-model/devres.rst        |   1 +
 Documentation/driver-api/pwm.rst              |  10 +-
 drivers/gpio/gpio-mvebu.c                     |  18 +-
 drivers/gpu/drm/bridge/ti-sn65dsi86.c         |  25 +-
 drivers/leds/rgb/leds-qcom-lpg.c              |  30 +-
 drivers/pwm/Kconfig                           |   4 -
 drivers/pwm/Makefile                          |   3 +-
 drivers/pwm/core.c                            | 480 +++++++++++++++---
 drivers/pwm/pwm-ab8500.c                      |  36 +-
 drivers/pwm/pwm-apple.c                       |  18 +-
 drivers/pwm/pwm-atmel-hlcdc.c                 |  35 +-
 drivers/pwm/pwm-atmel-tcb.c                   |  26 +-
 drivers/pwm/pwm-atmel.c                       |  37 +-
 drivers/pwm/pwm-bcm-iproc.c                   |  19 +-
 drivers/pwm/pwm-bcm-kona.c                    |  21 +-
 drivers/pwm/pwm-bcm2835.c                     |  17 +-
 drivers/pwm/pwm-berlin.c                      |  29 +-
 drivers/pwm/pwm-brcmstb.c                     |  17 +-
 drivers/pwm/pwm-clk.c                         |  27 +-
 drivers/pwm/pwm-clps711x.c                    |  21 +-
 drivers/pwm/pwm-crc.c                         |  26 +-
 drivers/pwm/pwm-cros-ec.c                     |  51 +-
 drivers/pwm/pwm-dwc-core.c                    |  25 +-
 drivers/pwm/pwm-dwc.c                         |  18 +-
 drivers/pwm/pwm-dwc.h                         |   9 +-
 drivers/pwm/pwm-ep93xx.c                      |  21 +-
 drivers/pwm/pwm-fsl-ftm.c                     |  48 +-
 drivers/pwm/pwm-hibvt.c                       |  25 +-
 drivers/pwm/pwm-img.c                         |  51 +-
 drivers/pwm/pwm-imx-tpm.c                     |  34 +-
 drivers/pwm/pwm-imx1.c                        |  17 +-
 drivers/pwm/pwm-imx27.c                       |  26 +-
 drivers/pwm/pwm-intel-lgm.c                   |  17 +-
 drivers/pwm/pwm-iqs620a.c                     |  37 +-
 drivers/pwm/pwm-jz4740.c                      |  35 +-
 drivers/pwm/pwm-keembay.c                     |  17 +-
 drivers/pwm/pwm-lp3943.c                      |  17 +-
 drivers/pwm/pwm-lpc18xx-sct.c                 |  35 +-
 drivers/pwm/pwm-lpc32xx.c                     |  19 +-
 drivers/pwm/pwm-lpss-pci.c                    |  10 +-
 drivers/pwm/pwm-lpss-platform.c               |  10 +-
 drivers/pwm/pwm-lpss.c                        |  34 +-
 drivers/pwm/pwm-lpss.h                        |   1 -
 drivers/pwm/pwm-mediatek.c                    |  28 +-
 drivers/pwm/pwm-meson.c                       |  57 ++-
 drivers/pwm/pwm-microchip-core.c              |  17 +-
 drivers/pwm/pwm-mtk-disp.c                    |  25 +-
 drivers/pwm/pwm-mxs.c                         |  32 +-
 drivers/pwm/pwm-ntxec.c                       |  30 +-
 drivers/pwm/pwm-omap-dmtimer.c                |  46 +-
 drivers/pwm/pwm-pca9685.c                     |  98 ++--
 drivers/pwm/pwm-pxa.c                         |  21 +-
 drivers/pwm/pwm-raspberrypi-poe.c             |  20 +-
 drivers/pwm/pwm-rcar.c                        |  25 +-
 drivers/pwm/pwm-renesas-tpu.c                 |  18 +-
 drivers/pwm/pwm-rockchip.c                    |  24 +-
 drivers/pwm/pwm-rz-mtu3.c                     |  38 +-
 drivers/pwm/pwm-samsung.c                     |  56 +-
 drivers/pwm/pwm-sifive.c                      |  30 +-
 drivers/pwm/pwm-sl28cpld.c                    |  13 +-
 drivers/pwm/pwm-spear.c                       |  17 +-
 drivers/pwm/pwm-sprd.c                        |  50 +-
 drivers/pwm/pwm-sti.c                         |  34 +-
 drivers/pwm/pwm-stm32-lp.c                    |  29 +-
 drivers/pwm/pwm-stm32.c                       |  53 +-
 drivers/pwm/pwm-stmpe.c                       |  58 ++-
 drivers/pwm/pwm-sun4i.c                       |  38 +-
 drivers/pwm/pwm-sunplus.c                     |  17 +-
 drivers/pwm/pwm-tegra.c                       |  27 +-
 drivers/pwm/pwm-tiecap.c                      |  55 +-
 drivers/pwm/pwm-tiehrpwm.c                    |  72 +--
 drivers/pwm/pwm-twl-led.c                     |  58 ++-
 drivers/pwm/pwm-twl.c                         |  50 +-
 drivers/pwm/pwm-visconti.c                    |  17 +-
 drivers/pwm/pwm-vt8500.c                      |  41 +-
 drivers/pwm/pwm-xilinx.c                      |  34 +-
 drivers/pwm/sysfs.c                           |  64 +--
 drivers/staging/greybus/pwm.c                 | 130 ++---
 include/linux/platform_data/x86/pwm-lpss.h    |   4 +-
 include/linux/pwm.h                           |  36 +-
 include/uapi/linux/pwm.h                      |  23 +
 81 files changed, 1651 insertions(+), 1291 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

base-commit: 869de350ff3834145273a6d39faedea878c6715a
-- 
2.42.0

[PATCH v3 100/108] 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 | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index a13f3c18ccd4..02c8382b4dd2 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -99,7 +99,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;
 
@@ -615,7 +614,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)
-		return -ENOMEM;
+	chip = devm_pwmchip_alloc(dev, mvchip->chip.ngpio, sizeof(struct mvebu_pwm));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	mvpwm = pwmchip_priv(chip);
+
 	mvchip->mvpwm = mvpwm;
 	mvpwm->mvchip = mvchip;
 	mvpwm->offset = offset;
@@ -868,13 +870,11 @@ 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;
 
 	spin_lock_init(&mvpwm->lock);
 
-	return devm_pwmchip_add(dev, &mvpwm->chip);
+	return devm_pwmchip_add(dev, chip);
 }
 
 #ifdef CONFIG_DEBUG_FS
-- 
2.42.0

Re: [PATCH v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Bartosz Golaszewski]

On Tue, Nov 21, 2023 at 2:52 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 | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index a13f3c18ccd4..02c8382b4dd2 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -99,7 +99,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;
>
> @@ -615,7 +614,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)
> -               return -ENOMEM;
> +       chip = devm_pwmchip_alloc(dev, mvchip->chip.ngpio, sizeof(struct mvebu_pwm));
> +       if (IS_ERR(chip))
> +               return PTR_ERR(chip);
> +       mvpwm = pwmchip_priv(chip);
> +
>         mvchip->mvpwm = mvpwm;
>         mvpwm->mvchip = mvchip;
>         mvpwm->offset = offset;
> @@ -868,13 +870,11 @@ 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;
>
>         spin_lock_init(&mvpwm->lock);
>
> -       return devm_pwmchip_add(dev, &mvpwm->chip);
> +       return devm_pwmchip_add(dev, chip);
>  }
>
>  #ifdef CONFIG_DEBUG_FS
> --
> 2.42.0
>

Eh... I had a talk at LPC where I explained why I really dislike this
approach but I guess this ship has sailed now and it's not a subsystem
where I have any say anyway.

It's not clear in the cover letter - are these patches supposed to go
through their respective subsystem trees?

Bart

Re: [PATCH v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Bart,

On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote:
> Eh... I had a talk at LPC where I explained why I really dislike this
> approach but I guess this ship has sailed now and it's not a subsystem
> where I have any say anyway.

Is there a record of your talk? I'm open to hear your arguments.
 
> It's not clear in the cover letter - are these patches supposed to go
> through their respective subsystem trees?

This patch can only go in once patch #37 is in. So for now the options
are:

 - Wait until devm_pwmchip_alloc() is in the mainline and apply this
   patch then via the gpio tree
 - Ack it and let it go in via the pwm tree with the other patches.
 
I'm not sure how quick this series will go in, so there is no rush.

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 v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Bart,

On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote:
> On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote:
> > Eh... I had a talk at LPC where I explained why I really dislike this
> > approach but I guess this ship has sailed now and it's not a subsystem
> > where I have any say anyway.
> 
> Is there a record of your talk? I'm open to hear your arguments.

I found your slides at
https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf

The main critic as I understand it about the "alloc_foo() +
register_foo()" approach is: "Breaks life-time logic - the driver
allocates the object but is not responsible for freeing it".

Yes, the driver allocates the object (via a subsystem helper). It is not
responsible for freeing the object, but the driver must drop its
reference to this object when going away. So foo_alloc() is paired by
foo_put().

The solution you present as the good way has the struct device in the
foo_wrapper. In GPIO land that's struct gpio_device, right?
gpiochip_add_data_with_key() allocates that using kzalloc() and "frees"
it with gpio_device_put() right? So your approach suffers from the same
inconsistency, the only upside is that you do that once at the subsystem
level instead of in each driver. (And in return you have two allocations
(priv + foo_wrapper) while the "alloc_foo() + register_foo()" approach
only needs one.)

Let's just rename foo_alloc() to foo_get_new() and the problem is gone?

In the implementation of foo_get_new() kzalloc() is still paired with
put_device() in foo_put(), but IMHO that's fine. The responsibility to
kfree() is traded to the struct device with device_initialize() in
return for a reference to the device. That's something you won't get rid
of while keeping the concept of reference counting.

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 v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Bartosz Golaszewski]

On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Bart,
>
> On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote:
> > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote:
> > > Eh... I had a talk at LPC where I explained why I really dislike this
> > > approach but I guess this ship has sailed now and it's not a subsystem
> > > where I have any say anyway.
> >
> > Is there a record of your talk? I'm open to hear your arguments.
>
> I found your slides at
> https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf
>

My talk is here: https://www.youtube.com/watch?v=VxaAorwL89c&t=29310s

> The main critic as I understand it about the "alloc_foo() +
> register_foo()" approach is: "Breaks life-time logic - the driver
> allocates the object but is not responsible for freeing it".
>
> Yes, the driver allocates the object (via a subsystem helper). It is not
> responsible for freeing the object, but the driver must drop its
> reference to this object when going away. So foo_alloc() is paired by
> foo_put().
>

Is it though? I don't see any pwmchip_put() being called in this
patch. I assume it's done implicitly but that's just confusing and
does break the scope.

> The solution you present as the good way has the struct device in the
> foo_wrapper. In GPIO land that's struct gpio_device, right?

Exactly.

> gpiochip_add_data_with_key() allocates that using kzalloc() and "frees"
> it with gpio_device_put() right? So your approach suffers from the same

No, the structure is allocated by kzalloc() but it's life-time is tied
with the struct device embedded in it and it's freed in the device's
.release() callback when the last reference is dropped.

> inconsistency, the only upside is that you do that once at the subsystem
> level instead of in each driver. (And in return you have two allocations
> (priv + foo_wrapper) while the "alloc_foo() + register_foo()" approach
> only needs one.)

Memory is cheap and this is not a hot path, so it isn't a big deal.

>
> Let's just rename foo_alloc() to foo_get_new() and the problem is gone?
>

Nope, because from a quick glance at PWM code, I'm convinced it will
suffer from the same hot-unplug problem I described in my talk. In
which case this rework will not fix all the issues.

> In the implementation of foo_get_new() kzalloc() is still paired with
> put_device() in foo_put(), but IMHO that's fine. The responsibility to
> kfree() is traded to the struct device with device_initialize() in
> return for a reference to the device. That's something you won't get rid
> of while keeping the concept of reference counting.
>

But if the PWM driver is unbound with users still holding references -
do you have a mechanism to handle that?

Bart

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

Re: [PATCH v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Bart,

On Wed, Nov 22, 2023 at 11:36:19AM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote:
> > > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote:
> > > > Eh... I had a talk at LPC where I explained why I really dislike this
> > > > approach but I guess this ship has sailed now and it's not a subsystem
> > > > where I have any say anyway.
> > >
> > > Is there a record of your talk? I'm open to hear your arguments.
> >
> > I found your slides at
> > https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf
> >
> 
> My talk is here: https://www.youtube.com/watch?v=VxaAorwL89c&t=29310s
> 
> > The main critic as I understand it about the "alloc_foo() +
> > register_foo()" approach is: "Breaks life-time logic - the driver
> > allocates the object but is not responsible for freeing it".
> >
> > Yes, the driver allocates the object (via a subsystem helper). It is not
> > responsible for freeing the object, but the driver must drop its
> > reference to this object when going away. So foo_alloc() is paired by
> > foo_put().
> >
> 
> Is it though? I don't see any pwmchip_put() being called in this
> patch.

It's not in this patch. Up to patch #103 I'm preparing drivers and the
code that is moved into the core isn't better than what was done before
in each driver.

Look at patch #106 which does the relevant conversion in
pwmchip_alloc(). When unbinding the mvebu gpio driver the necessary
pwmchip_put() is triggered by the devm cleanup registered in
devm_pwmchip_alloc().

> I assume it's done implicitly but that's just confusing and
> does break the scope.
> 
> > The solution you present as the good way has the struct device in the
> > foo_wrapper. In GPIO land that's struct gpio_device, right?
> 
> Exactly.
> 
> > gpiochip_add_data_with_key() allocates that using kzalloc() and "frees"
> > it with gpio_device_put() right? So your approach suffers from the same
> 
> No, the structure is allocated by kzalloc() but it's life-time is tied
> with the struct device embedded in it and it's freed in the device's
> .release() callback when the last reference is dropped.

With the complete series applied a pwmchip is allocated by
pwmchip_alloc() and it's life-time is tied with the struct device
embedded in it and it's freed in the device's .release() callback when
the last reference is dropped.

In this respect I see a certain similarity between your gpio approach
and mine for pwm. So either I don't understand your critic on my patch
set, or I don't see why it shouldn't apply to your approach, too.

Yes, gpio drivers look fine having only ..._alloc() paired with
..._free() and ..._get() with ..._put(). But that's only because you
moved that "inconsistency" of kzalloc() <-> put_device() into the gpio
core, while I kept it in the drivers.

Renaming pwmchip_alloc() to pwmchip_get_new() was a honest suggestion
that moves that inconsistency to the core, too.

> > inconsistency, the only upside is that you do that once at the subsystem
> > level instead of in each driver. (And in return you have two allocations
> > (priv + foo_wrapper) while the "alloc_foo() + register_foo()" approach
> > only needs one.)
> 
> Memory is cheap and this is not a hot path, so it isn't a big deal.

It's not only about wasting memory and the time needed to dereference
pointers. It's also about complexity that has to be grasped by humans.
Also not being in a hot path doesn't mean it's bad to pick the faster
approach. Having said that I'm not sure if the hot paths (e.g.
gpiod_set_value()) really don't suffer from having two separate
allocations.

But I guess we're both biased here to our own approach because that's
what each of us thought about in detail.

> > Let's just rename foo_alloc() to foo_get_new() and the problem is gone?
> 
> Nope, because from a quick glance at PWM code, I'm convinced it will
> suffer from the same hot-unplug problem I described in my talk. In
> which case this rework will not fix all the issues.

Please look at the state after patch #107. If you spot an issue there,
please tell me.

> > In the implementation of foo_get_new() kzalloc() is still paired with
> > put_device() in foo_put(), but IMHO that's fine. The responsibility to
> > kfree() is traded to the struct device with device_initialize() in
> > return for a reference to the device. That's something you won't get rid
> > of while keeping the concept of reference counting.
> 
> But if the PWM driver is unbound with users still holding references -
> do you have a mechanism to handle that?

Yes, should be fine starting with patch #107. In my tests (on top of
patch #108) it works fine. I held /dev/pwmchipX open and unbound the
lowlevel driver. The ioctls are caught in the core then and yield an
error and the kfree of the pwmchip struct is delayed until /dev/pwmchipX
is closed.

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 v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Thierry Reding]

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

On Wed, Nov 22, 2023 at 11:36:19AM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello Bart,
> >
> > On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote:
> > > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote:
> > > > Eh... I had a talk at LPC where I explained why I really dislike this
> > > > approach but I guess this ship has sailed now and it's not a subsystem
> > > > where I have any say anyway.
> > >
> > > Is there a record of your talk? I'm open to hear your arguments.
> >
> > I found your slides at
> > https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf
> >
> 
> My talk is here: https://www.youtube.com/watch?v=VxaAorwL89c&t=29310s

I've been watching this along with Laurent's talk from last year (and I
guess I should probably also go through Wolfram's patch from earlier
this year) and I really like what you presented. It also sounds like
there was a lot of support across various audience members, so I think
it'd be good to rally around such a common pattern so we can start to
improve things on a more wide basis.

Given that this wasn't very long ago, I wouldn't expect that much work
has happened yet on the resmgr library. However, I think it would fit
very well both with how PWM works today and with what Uwe has in mind
for the character device support.

Thierry

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

Re: [PATCH v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Bartosz Golaszewski]

On Fri, Nov 24, 2023 at 1:14 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Wed, Nov 22, 2023 at 11:36:19AM +0100, Bartosz Golaszewski wrote:
> > On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > Hello Bart,
> > >
> > > On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote:
> > > > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote:
> > > > > Eh... I had a talk at LPC where I explained why I really dislike this
> > > > > approach but I guess this ship has sailed now and it's not a subsystem
> > > > > where I have any say anyway.
> > > >
> > > > Is there a record of your talk? I'm open to hear your arguments.
> > >
> > > I found your slides at
> > > https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf
> > >
> >
> > My talk is here: https://www.youtube.com/watch?v=VxaAorwL89c&t=29310s
>
> I've been watching this along with Laurent's talk from last year (and I
> guess I should probably also go through Wolfram's patch from earlier
> this year) and I really like what you presented. It also sounds like
> there was a lot of support across various audience members, so I think
> it'd be good to rally around such a common pattern so we can start to
> improve things on a more wide basis.
>
> Given that this wasn't very long ago, I wouldn't expect that much work
> has happened yet on the resmgr library. However, I think it would fit
> very well both with how PWM works today and with what Uwe has in mind
> for the character device support.
>
> Thierry

Hi Thierry,

Thanks for the kind words. No work has been done so far other than
thinking about the possible API. I'm currently in the process of
trying to fix the object life-time and concurrent access in GPIO -
mostly improving the dire locking situation. My goal is to implement
all I spoke about in GPIO first and then try to generalize it to some
other subsystem like what Greg KH suggested.

I've already got support from Wolfram on that and we of course could
use any help we can get.

I admit I've been quite busy but I do plan on going through Uwe's
series next week and maybe running tests similar to what I have for
GPIO on it. I'm quite certain (correct me if I'm wrong) that this
series doesn't improve the locking (specifically hot-unplug events
during API calls). I think that my proposal has the advantage of
having the pointer to the implementation in the "wrapper" which can be
easily protected with RCU.

Uwe: do you have a solution for device removal concurrent with API
calls when using your approach?

Bart

Re: [PATCH v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Bart,

On Fri, Nov 24, 2023 at 10:16:40PM +0100, Bartosz Golaszewski wrote:
> On Fri, Nov 24, 2023 at 1:14 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Wed, Nov 22, 2023 at 11:36:19AM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote:
> > > > > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote:
> > > > > > Eh... I had a talk at LPC where I explained why I really dislike this
> > > > > > approach but I guess this ship has sailed now and it's not a subsystem
> > > > > > where I have any say anyway.
> > > > >
> > > > > Is there a record of your talk? I'm open to hear your arguments.
> > > >
> > > > I found your slides at
> > > > https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf
> > > >
> > >
> > > My talk is here: https://www.youtube.com/watch?v=VxaAorwL89c&t=29310s
> >
> > I've been watching this along with Laurent's talk from last year (and I
> > guess I should probably also go through Wolfram's patch from earlier
> > this year) and I really like what you presented. It also sounds like
> > there was a lot of support across various audience members, so I think
> > it'd be good to rally around such a common pattern so we can start to
> > improve things on a more wide basis.
> >
> > Given that this wasn't very long ago, I wouldn't expect that much work
> > has happened yet on the resmgr library. However, I think it would fit
> > very well both with how PWM works today and with what Uwe has in mind
> > for the character device support.
> >
> > Thierry
> 
> Hi Thierry,
> 
> Thanks for the kind words. No work has been done so far other than
> thinking about the possible API. I'm currently in the process of
> trying to fix the object life-time and concurrent access in GPIO -
> mostly improving the dire locking situation. My goal is to implement
> all I spoke about in GPIO first and then try to generalize it to some
> other subsystem like what Greg KH suggested.
> 
> I've already got support from Wolfram on that and we of course could
> use any help we can get.
> 
> I admit I've been quite busy but I do plan on going through Uwe's
> series next week and maybe running tests similar to what I have for
> GPIO on it. I'm quite certain (correct me if I'm wrong) that this
> series doesn't improve the locking (specifically hot-unplug events
> during API calls). I think that my proposal has the advantage of
> having the pointer to the implementation in the "wrapper" which can be
> easily protected with RCU.

Maybe I didn't understand the problem yet, but I think hotplugging isn't
a problem for my approach. The hardware accesses in the lowlevel driver
(probably) fail then but that's something you cannot prevent. And
because pwmchip->lock is held during calls in the lowlevel driver,
removal of the driver is delayed accordingly.

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 v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Bartosz,

On Fri, Nov 24, 2023 at 10:16:40PM +0100, Bartosz Golaszewski wrote:
> I admit I've been quite busy but I do plan on going through Uwe's
> series next week and maybe running tests similar to what I have for
> GPIO on it.

That's great. If you want to do that on my tree that already saw a few
improvements compared to what I sent out, get it at

	https://git.pengutronix.de/git/ukl/linux pwm-lifetime-tracking

. The improvements are only on the driver level, so unless you're using
one of the improved drivers, the difference wouldn't be that big I
guess. For (maybe) quicker feedback loops, you can find me on irc (e.g.
on libera's #linux-pwm) if that's a communication channel you like.

I look forward to your findings,
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 v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Bartosz Golaszewski]

On Mon, Nov 27, 2023 at 11:58 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Bartosz,
>
> On Fri, Nov 24, 2023 at 10:16:40PM +0100, Bartosz Golaszewski wrote:
> > I admit I've been quite busy but I do plan on going through Uwe's
> > series next week and maybe running tests similar to what I have for
> > GPIO on it.
>
> That's great. If you want to do that on my tree that already saw a few
> improvements compared to what I sent out, get it at
>
>         https://git.pengutronix.de/git/ukl/linux pwm-lifetime-tracking
>
> . The improvements are only on the driver level, so unless you're using
> one of the improved drivers, the difference wouldn't be that big I
> guess. For (maybe) quicker feedback loops, you can find me on irc (e.g.
> on libera's #linux-pwm) if that's a communication channel you like.
>
> I look forward to your findings,
> Uwe

I don't see anything obviously wrong with the approach. I see the
chip->operational field that is set to false on release. In my
version, we just use a NULL-pointer to carry the same information.
Interestingly you DO have a pwm_device and pwm_chip structures. I'd
say it would be more logical to have the pwm_device embed struct
device.

My approach is more about maintaining the logical scope and not
changing the ownership of objects allocated in the driver. I also
don't see a reason to expose the internals of the subsystem (struct
device) to the provider drivers other than in callbacks where it is
relevant. Subsystems should handle as much as possible and any data
structures not relevant to what the driver does should be hidden from
it.

Bart

Re: [PATCH v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Bart,

On Mon, Nov 27, 2023 at 09:22:48PM +0100, Bartosz Golaszewski wrote:
> On Mon, Nov 27, 2023 at 11:58 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Nov 24, 2023 at 10:16:40PM +0100, Bartosz Golaszewski wrote:
> > > I admit I've been quite busy but I do plan on going through Uwe's
> > > series next week and maybe running tests similar to what I have for
> > > GPIO on it.
> >
> > That's great. If you want to do that on my tree that already saw a few
> > improvements compared to what I sent out, get it at
> >
> >         https://git.pengutronix.de/git/ukl/linux pwm-lifetime-tracking
> >
> > . The improvements are only on the driver level, so unless you're using
> > one of the improved drivers, the difference wouldn't be that big I
> > guess. For (maybe) quicker feedback loops, you can find me on irc (e.g.
> > on libera's #linux-pwm) if that's a communication channel you like.
> 
> I don't see anything obviously wrong with the approach.

Is this the result of "running tests similar to what I have for GPIO on
it" or did you only find the time for some high-level code inspection?

> I see the
> chip->operational field that is set to false on release. In my
> version, we just use a NULL-pointer to carry the same information.

Yup, sounds obvious. Your usage of "just" sounds as if your variant was
better. To give the alternative view where the suggested approach sounds
better would be:

You need a pointer and I "just" a bool that even has a name implying its
function. You need to dereference the pointer in several places as the
needed information is distributed over two structures while it's all
together in a single struct for the usual foo_alloc() + foo_register()
approach.

> Interestingly you DO have a pwm_device and pwm_chip structures. I'd
> say it would be more logical to have the pwm_device embed struct
> device.

A pwm_chip represents a piece of hardware that provides (possibly)
several PWM lines. A pwm_device is the abstraction for a single PWM
line. So that's two different concepts and I wonder why you find it
interesting that we have two different structures for it.

Today the pwm framework already has a struct device for the
pwm_chip that appears in /sys/class/pwm/pwmchipX. If a PWM line is
exported in sysfs, another struct containing a struct device is
allocated (struct pwm_export) to manage /sys/class/pwm/pwmchipX/pwmY/.

I think it's good to have a struct device in the gpio_chip. I'd be open
to put a struct device into pwm_device (unconditionally, not only when
it's exported), but that's a change that is out of scope for this
series. Also note that this would change the behaviour of
/sys/class/pwm/ which I'd like to prevent (at least today until the
character support is established, available for some time and known to
be in use).

> My approach is more about maintaining the logical scope and not
> changing the ownership of objects allocated in the driver. I also
> don't see a reason to expose the internals of the subsystem (struct
> device) to the provider drivers other than in callbacks where it is
> relevant. Subsystems should handle as much as possible and any data
> structures not relevant to what the driver does should be hidden from
> it.

Drivers see struct pwm_chip today and IMHO that's fine. I also feel
little incentive to hide something from the driver in .probe() and then
have to expose (more of) it in .apply() anyhow. Also I don't think the
series would benefit from putting yet more changes into it.

Struct pwm_chip currently contains the following members:

        struct device dev;
        struct cdev cdev;
        const struct pwm_ops *ops;
        struct module *owner;
        unsigned int id;
        unsigned int npwm;

        struct pwm_device * (*of_xlate)(struct pwm_chip *chip,
                                        const struct of_phandle_args *args);
        unsigned int of_pwm_n_cells;

        /* only used internally by the PWM framework */
        struct mutex lock;
        bool uses_pwmchip_alloc;
        bool operational;
        void *drvdata;
        struct pwm_device pwms[] __counted_by(npwm);

Some of them should be moved below the "only used internally" comment.
(i.e. dev, cdev, owner, id). For me this is "hidden" good enough then.

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 v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Bartosz Golaszewski]

On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>

[snip]

>
> > I see the
> > chip->operational field that is set to false on release. In my
> > version, we just use a NULL-pointer to carry the same information.
>
> Yup, sounds obvious. Your usage of "just" sounds as if your variant was
> better. To give the alternative view where the suggested approach sounds
> better would be:
>
> You need a pointer and I "just" a bool that even has a name implying its
> function. You need to dereference the pointer in several places as the
> needed information is distributed over two structures while it's all
> together in a single struct for the usual foo_alloc() + foo_register()
> approach.
>

There's another reason we do that. I'm no longer sure if I mentioned
it in my talk (I meant to anyway).

In GPIO we have API functions that may be called from any context -
thus needing spinlocks for locking - but also driver callbacks that
may use mutexes internally or otherwise sleep. I don't know if this is
the case for PWM too but in GPIO we may end up in a situation where if
we used a spinlock to protect some kind of an "is_operational" field,
we'd end up sleeping with a spinlock taken and if we used a mutex, we
couldn't use API function from atomic contexts.

This is the reason behind locking being so broken in GPIO at the
moment and why I'm trying to fix it this release cycle.

Splitting the implementation into two structures and protecting the
pointer to the provider structure with SRCU has the benefit of not
limiting us in what locks we use underneath.

Every subsystem has its own issues and we need to find something
generic enough to cover them all (or most of them anyway). I don't
think having a single structure cuts it.

Bart

[snip]

Re: [PATCH v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

On Fri, Dec 01, 2023 at 11:14:32AM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> 
> [snip]
> 
> >
> > > I see the
> > > chip->operational field that is set to false on release. In my
> > > version, we just use a NULL-pointer to carry the same information.
> >
> > Yup, sounds obvious. Your usage of "just" sounds as if your variant was
> > better. To give the alternative view where the suggested approach sounds
> > better would be:
> >
> > You need a pointer and I "just" a bool that even has a name implying its
> > function. You need to dereference the pointer in several places as the
> > needed information is distributed over two structures while it's all
> > together in a single struct for the usual foo_alloc() + foo_register()
> > approach.
> >
> 
> There's another reason we do that. I'm no longer sure if I mentioned
> it in my talk (I meant to anyway).
> 
> In GPIO we have API functions that may be called from any context -
> thus needing spinlocks for locking - but also driver callbacks that
> may use mutexes internally or otherwise sleep. I don't know if this is
> the case for PWM too but in GPIO we may end up in a situation where if
> we used a spinlock to protect some kind of an "is_operational" field,
> we'd end up sleeping with a spinlock taken and if we used a mutex, we
> couldn't use API function from atomic contexts.
> 
> This is the reason behind locking being so broken in GPIO at the
> moment and why I'm trying to fix it this release cycle.
> 
> Splitting the implementation into two structures and protecting the
> pointer to the provider structure with SRCU has the benefit of not
> limiting us in what locks we use underneath.
> 
> Every subsystem has its own issues and we need to find something
> generic enough to cover them all (or most of them anyway). I don't
> think having a single structure cuts it.

I'm convinced it works. I introduced a wrapper pwmchip_lock() that for
now uses a mutex and once we have fast pwm_chips it uses a mutex for
sleeping pwm_chips and a spinlock for the fast ones.

That's similar to how struct irq_chip::irq_bus_lock works. For sleeping
chips that callback uses a mutex, for fast chips a spinlock.

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 v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Bartosz Golaszewski]

On Sat, Dec 2, 2023 at 1:43 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Fri, Dec 01, 2023 at 11:14:32AM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> >
> > [snip]
> >
> > >
> > > > I see the
> > > > chip->operational field that is set to false on release. In my
> > > > version, we just use a NULL-pointer to carry the same information.
> > >
> > > Yup, sounds obvious. Your usage of "just" sounds as if your variant was
> > > better. To give the alternative view where the suggested approach sounds
> > > better would be:
> > >
> > > You need a pointer and I "just" a bool that even has a name implying its
> > > function. You need to dereference the pointer in several places as the
> > > needed information is distributed over two structures while it's all
> > > together in a single struct for the usual foo_alloc() + foo_register()
> > > approach.
> > >
> >
> > There's another reason we do that. I'm no longer sure if I mentioned
> > it in my talk (I meant to anyway).
> >
> > In GPIO we have API functions that may be called from any context -
> > thus needing spinlocks for locking - but also driver callbacks that
> > may use mutexes internally or otherwise sleep. I don't know if this is
> > the case for PWM too but in GPIO we may end up in a situation where if
> > we used a spinlock to protect some kind of an "is_operational" field,
> > we'd end up sleeping with a spinlock taken and if we used a mutex, we
> > couldn't use API function from atomic contexts.
> >
> > This is the reason behind locking being so broken in GPIO at the
> > moment and why I'm trying to fix it this release cycle.
> >
> > Splitting the implementation into two structures and protecting the
> > pointer to the provider structure with SRCU has the benefit of not
> > limiting us in what locks we use underneath.
> >
> > Every subsystem has its own issues and we need to find something
> > generic enough to cover them all (or most of them anyway). I don't
> > think having a single structure cuts it.
>
> I'm convinced it works. I introduced a wrapper pwmchip_lock() that for
> now uses a mutex and once we have fast pwm_chips it uses a mutex for
> sleeping pwm_chips and a spinlock for the fast ones.
>
> That's similar to how struct irq_chip::irq_bus_lock works. For sleeping
> chips that callback uses a mutex, for fast chips a spinlock.
>

Fair enough. I'd love to see a benchmark of what's faster one day
though: two structures with dereferencing and SRCU or one structure
with mutex/spinlock.

By "fair enough" I mean: I still don't like it for the reasons I
mentioned before but I cannot point out anything technically wrong.

Bart

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

Re: [PATCH v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Bartosz Golaszewski]

On Mon, Dec 4, 2023 at 9:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Sat, Dec 2, 2023 at 1:43 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Fri, Dec 01, 2023 at 11:14:32AM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > >
> > > [snip]
> > >
> > > >
> > > > > I see the
> > > > > chip->operational field that is set to false on release. In my
> > > > > version, we just use a NULL-pointer to carry the same information.
> > > >
> > > > Yup, sounds obvious. Your usage of "just" sounds as if your variant was
> > > > better. To give the alternative view where the suggested approach sounds
> > > > better would be:
> > > >
> > > > You need a pointer and I "just" a bool that even has a name implying its
> > > > function. You need to dereference the pointer in several places as the
> > > > needed information is distributed over two structures while it's all
> > > > together in a single struct for the usual foo_alloc() + foo_register()
> > > > approach.
> > > >
> > >
> > > There's another reason we do that. I'm no longer sure if I mentioned
> > > it in my talk (I meant to anyway).
> > >
> > > In GPIO we have API functions that may be called from any context -
> > > thus needing spinlocks for locking - but also driver callbacks that
> > > may use mutexes internally or otherwise sleep. I don't know if this is
> > > the case for PWM too but in GPIO we may end up in a situation where if
> > > we used a spinlock to protect some kind of an "is_operational" field,
> > > we'd end up sleeping with a spinlock taken and if we used a mutex, we
> > > couldn't use API function from atomic contexts.
> > >
> > > This is the reason behind locking being so broken in GPIO at the
> > > moment and why I'm trying to fix it this release cycle.
> > >
> > > Splitting the implementation into two structures and protecting the
> > > pointer to the provider structure with SRCU has the benefit of not
> > > limiting us in what locks we use underneath.
> > >
> > > Every subsystem has its own issues and we need to find something
> > > generic enough to cover them all (or most of them anyway). I don't
> > > think having a single structure cuts it.
> >
> > I'm convinced it works. I introduced a wrapper pwmchip_lock() that for
> > now uses a mutex and once we have fast pwm_chips it uses a mutex for
> > sleeping pwm_chips and a spinlock for the fast ones.
> >
> > That's similar to how struct irq_chip::irq_bus_lock works. For sleeping
> > chips that callback uses a mutex, for fast chips a spinlock.
> >
>
> Fair enough. I'd love to see a benchmark of what's faster one day
> though: two structures with dereferencing and SRCU or one structure
> with mutex/spinlock.
>

Actually there is one thing that - while not technically wrong - makes
the split solution better. In case of your abstracted lock, you find
yourself in a very all-or-nothing locking situation, where all of the
structure is locked or none is. With SRCU protecting just the pointer
to implementation, we can easily factor that part out and leave
whatever fine-grained locking is required to the subsystem.

Additionally: the pointer to implementation has many readers but only
one writer. I believe this to be the same for your "operational"
field. I don't know the PWM code very well but I can only guess that
the situation is similar, where subsystem data structures are read
more often than they are modified and multiple readers could access
the structure at the same time lowering latencies.

Just another 2 cents.

Bart

> By "fair enough" I mean: I still don't like it for the reasons I
> mentioned before but I cannot point out anything technically wrong.
>
> Bart
>
> > Best regards
> > Uwe
> >
> > --
> > Pengutronix e.K.                           | Uwe Kleine-König            |
> > Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello,

On Mon, Dec 04, 2023 at 10:28:15PM +0100, Bartosz Golaszewski wrote:
> On Mon, Dec 4, 2023 at 9:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Sat, Dec 2, 2023 at 1:43 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > On Fri, Dec 01, 2023 at 11:14:32AM +0100, Bartosz Golaszewski wrote:
> > > > On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > >
> > > >
> > > > [snip]
> > > >
> > > > >
> > > > > > I see the
> > > > > > chip->operational field that is set to false on release. In my
> > > > > > version, we just use a NULL-pointer to carry the same information.
> > > > >
> > > > > Yup, sounds obvious. Your usage of "just" sounds as if your variant was
> > > > > better. To give the alternative view where the suggested approach sounds
> > > > > better would be:
> > > > >
> > > > > You need a pointer and I "just" a bool that even has a name implying its
> > > > > function. You need to dereference the pointer in several places as the
> > > > > needed information is distributed over two structures while it's all
> > > > > together in a single struct for the usual foo_alloc() + foo_register()
> > > > > approach.
> > > > >
> > > >
> > > > There's another reason we do that. I'm no longer sure if I mentioned
> > > > it in my talk (I meant to anyway).
> > > >
> > > > In GPIO we have API functions that may be called from any context -
> > > > thus needing spinlocks for locking - but also driver callbacks that
> > > > may use mutexes internally or otherwise sleep. I don't know if this is
> > > > the case for PWM too but in GPIO we may end up in a situation where if
> > > > we used a spinlock to protect some kind of an "is_operational" field,
> > > > we'd end up sleeping with a spinlock taken and if we used a mutex, we
> > > > couldn't use API function from atomic contexts.
> > > >
> > > > This is the reason behind locking being so broken in GPIO at the
> > > > moment and why I'm trying to fix it this release cycle.
> > > >
> > > > Splitting the implementation into two structures and protecting the
> > > > pointer to the provider structure with SRCU has the benefit of not
> > > > limiting us in what locks we use underneath.
> > > >
> > > > Every subsystem has its own issues and we need to find something
> > > > generic enough to cover them all (or most of them anyway). I don't
> > > > think having a single structure cuts it.
> > >
> > > I'm convinced it works. I introduced a wrapper pwmchip_lock() that for
> > > now uses a mutex and once we have fast pwm_chips it uses a mutex for
> > > sleeping pwm_chips and a spinlock for the fast ones.
> > >
> > > That's similar to how struct irq_chip::irq_bus_lock works. For sleeping
> > > chips that callback uses a mutex, for fast chips a spinlock.
> > >
> >
> > Fair enough. I'd love to see a benchmark of what's faster one day
> > though: two structures with dereferencing and SRCU or one structure
> > with mutex/spinlock.

I think until the day has come that we have a SRCU+two and
mutex/spinlock+one implementation for one framework it's moot to discuss
which one is the faster, so I suggest we stop here. (Note, you can
already do mutex/spinlock+two already now. That's what I do for the
non-pure PWM drivers in the next iteration. Preview at
https://lore.kernel.org/linux-pwm/20231124215208.616551-4-u.kleine-koenig@pengutronix.de/T/#u)
For me it's not so clear that SRCU is the winner. Also the winner might
vary depending on questions like:

 - How many PWM (or GPIO) lines does the chip in question expose?
 - Does the implementation of the callbacks need serialisation (because
   the bits for different lines are in common registers)?
 - Usage pattern (influencing the contention of the locks)

(But I adhere to my suggestion to stop now :-)

> Actually there is one thing that - while not technically wrong - makes
> the split solution better. In case of your abstracted lock, you find
> yourself in a very all-or-nothing locking situation, where all of the
> structure is locked or none is. With SRCU protecting just the pointer
> to implementation, we can easily factor that part out and leave
> whatever fine-grained locking is required to the subsystem.

The fine-grainedness of the locking scheme isn't fixed with my approach.

In fact you could just not use the offer to handle framework struct and
driver private data in a single memory chunk (and/or stop using the
knowledge that it is (or can be) a single chunk) and then the two
approaches are not fundamentally different and you can use the same
locking mechanisms.

The biggest difference between our approaches is that I handle
allocation of the framework struct and its registration in two steps in
the drivers while you do that in a single one.

My approach has the advantage that it allows to handle private data in
the same allocation and that the driver can fill in the framework struct
without the need for copying or pointer dereferencing if the framework
needs the driver provided information. Yours has the advantage that
drivers see less of the framework and so are better separated from the
core.

How you weight the different advantages is very subjective.

So if we rule out the subjective metrics we're left with: Both
approaches solve the technical challenge at hand (i.e. ensure unloading
a driver doesn't make the kernel crash if a character device is still
open) and my approach already exists for pwm.

> Additionally: the pointer to implementation has many readers but only
> one writer. I believe this to be the same for your "operational"
> field. I don't know the PWM code very well but I can only guess that
> the situation is similar, where subsystem data structures are read
> more often than they are modified and multiple readers could access
> the structure at the same time lowering latencies.

The lock serves to serialize access to .operational and ensures that the
driver doesn't go away until all callbacks have completed. Is this
serialized in your approach, too?
(If you don't, I wonder if you should. And if you do, I think this
better matches the use case spinlocks and mutexes are optimized for
compared to SRCU.)

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