[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 01/18] pwm: Provide devm_pwmchip_alloc() function

[Uwe Kleine-König]

This function allocates a struct pwm_chip and driver data. Compared to
the status quo the split into pwm_chip and driver data is new, otherwise
it doesn't change anything relevant (yet).

The intention is that after all drivers are switched to use this
allocation function, its possible to add a struct device to struct
pwm_chip to properly track the latter's lifetime without touching all
drivers again. Proper lifetime tracking is a necessary precondition to
introduce character device support for PWMs (that implements atomic
setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm
userspace support).

The new function pwmchip_priv() (obviously?) only works for chips
allocated with devm_pwmchip_alloc().

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 .../driver-api/driver-model/devres.rst        |  1 +
 Documentation/driver-api/pwm.rst              | 10 ++++----
 drivers/pwm/core.c                            | 23 +++++++++++++++++++
 include/linux/pwm.h                           |  4 ++++
 4 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index 8be086b3f829..73a9ee074737 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -414,6 +414,7 @@ POWER
   devm_reboot_mode_unregister()
 
 PWM
+  devm_pwmchip_alloc()
   devm_pwmchip_add()
   devm_pwm_get()
   devm_fwnode_pwm_get()
diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
index 3fdc95f7a1d1..a3824bd58e4c 100644
--- a/Documentation/driver-api/pwm.rst
+++ b/Documentation/driver-api/pwm.rst
@@ -134,11 +134,11 @@ to implement the pwm_*() functions itself. This means that it's impossible
 to have multiple PWM drivers in the system. For this reason it's mandatory
 for new drivers to use the generic PWM framework.
 
-A new PWM controller/chip can be added using pwmchip_add() and removed
-again with pwmchip_remove(). pwmchip_add() takes a filled in struct
-pwm_chip as argument which provides a description of the PWM chip, the
-number of PWM devices provided by the chip and the chip-specific
-implementation of the supported PWM operations to the framework.
+A new PWM controller/chip can be allocated using devm_pwmchip_alloc, then added
+using pwmchip_add() and removed again with pwmchip_remove(). pwmchip_add()
+takes a filled in struct pwm_chip as argument which provides a description of
+the PWM chip, the number of PWM devices provided by the chip and the
+chip-specific implementation of the supported PWM operations to the framework.
 
 When implementing polarity support in a PWM driver, make sure to respect the
 signal conventions in the PWM framework. By definition, normal polarity
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 81d176142403..3f4c2d940d64 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -304,6 +304,29 @@ void pwmchip_remove(struct pwm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(pwmchip_remove);
 
+void *pwmchip_priv(struct pwm_chip *chip)
+{
+	return (char *)chip + ALIGN(sizeof(*chip), 32);
+}
+EXPORT_SYMBOL_GPL(pwmchip_priv);
+
+struct pwm_chip *devm_pwmchip_alloc(struct device *parent, size_t sizeof_priv)
+{
+	struct pwm_chip *chip;
+	size_t alloc_size;
+
+	alloc_size = ALIGN(sizeof(*chip), 32) + sizeof_priv;
+
+	chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL);
+	if (!chip)
+		return NULL;
+
+	chip->dev = parent;
+
+	return chip;
+}
+EXPORT_SYMBOL_GPL(devm_pwmchip_alloc);
+
 static void devm_pwmchip_remove(void *data)
 {
 	struct pwm_chip *chip = data;
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 10bea923a1d5..648009c8499b 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -5,6 +5,7 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/compiler_attributes.h>
 
 struct pwm_chip;
 
@@ -385,6 +386,9 @@ int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
 int pwmchip_add(struct pwm_chip *chip);
 void pwmchip_remove(struct pwm_chip *chip);
 
+void *pwmchip_priv(struct pwm_chip *chip) __attribute_const__;
+
+struct pwm_chip *devm_pwmchip_alloc(struct device *parent, size_t sizeof_priv);
 int devm_pwmchip_add(struct device *dev, struct pwm_chip *chip);
 
 struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
-- 
2.39.2

[PATCH 02/18] pwm: ab8500: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-ab8500 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/pwm/pwm-ab8500.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-ab8500.c b/drivers/pwm/pwm-ab8500.c
index 583a7d69c741..7b4b4a5d1bfe 100644
--- a/drivers/pwm/pwm-ab8500.c
+++ b/drivers/pwm/pwm-ab8500.c
@@ -24,13 +24,12 @@
 #define AB8500_PWM_CLKRATE 9600000
 
 struct ab8500_pwm_chip {
-	struct pwm_chip chip;
 	unsigned int hwid;
 };
 
 static struct ab8500_pwm_chip *ab8500_pwm_from_chip(struct pwm_chip *chip)
 {
-	return container_of(chip, struct ab8500_pwm_chip, chip);
+	return pwmchip_priv(chip);
 }
 
 static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -186,6 +185,7 @@ static const struct pwm_ops ab8500_pwm_ops = {
 
 static int ab8500_pwm_probe(struct platform_device *pdev)
 {
+	struct pwm_chip *chip;
 	struct ab8500_pwm_chip *ab8500;
 	int err;
 
@@ -196,16 +196,17 @@ static int ab8500_pwm_probe(struct platform_device *pdev)
 	 * Nothing to be done in probe, this is required to get the
 	 * device which is required for ab8500 read and write
 	 */
-	ab8500 = devm_kzalloc(&pdev->dev, sizeof(*ab8500), GFP_KERNEL);
-	if (ab8500 == NULL)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*ab8500));
+	if (chip == NULL)
 		return -ENOMEM;
 
-	ab8500->chip.dev = &pdev->dev;
-	ab8500->chip.ops = &ab8500_pwm_ops;
-	ab8500->chip.npwm = 1;
+	ab8500 = pwmchip_priv(chip);
+
+	chip->ops = &ab8500_pwm_ops;
+	chip->npwm = 1;
 	ab8500->hwid = pdev->id - 1;
 
-	err = devm_pwmchip_add(&pdev->dev, &ab8500->chip);
+	err = devm_pwmchip_add(&pdev->dev, chip);
 	if (err < 0)
 		return dev_err_probe(&pdev->dev, err, "Failed to add pwm chip\n");
 
-- 
2.39.2

[PATCH 03/18] pwm: apple: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-apple 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/pwm/pwm-apple.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-apple.c b/drivers/pwm/pwm-apple.c
index a38a62edd713..cc045a719cbb 100644
--- a/drivers/pwm/pwm-apple.c
+++ b/drivers/pwm/pwm-apple.c
@@ -31,14 +31,13 @@
 #define APPLE_PWM_CTRL_OUTPUT_ENABLE BIT(14)
 
 struct apple_pwm {
-	struct pwm_chip chip;
 	void __iomem *base;
 	u64 clkrate;
 };
 
 static inline struct apple_pwm *to_apple_pwm(struct pwm_chip *chip)
 {
-	return container_of(chip, struct apple_pwm, chip);
+	return pwmchip_priv(chip);
 }
 
 static int apple_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -103,14 +102,17 @@ static const struct pwm_ops apple_pwm_ops = {
 
 static int apple_pwm_probe(struct platform_device *pdev)
 {
+	struct pwm_chip *chip;
 	struct apple_pwm *fpwm;
 	struct clk *clk;
 	int ret;
 
-	fpwm = devm_kzalloc(&pdev->dev, sizeof(*fpwm), GFP_KERNEL);
-	if (!fpwm)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*fpwm));
+	if (!chip)
 		return -ENOMEM;
 
+	fpwm = to_apple_pwm(chip);
+
 	fpwm->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(fpwm->base))
 		return PTR_ERR(fpwm->base);
@@ -129,11 +131,10 @@ static int apple_pwm_probe(struct platform_device *pdev)
 	if (fpwm->clkrate > NSEC_PER_SEC)
 		return dev_err_probe(&pdev->dev, -EINVAL, "pwm clock out of range");
 
-	fpwm->chip.dev = &pdev->dev;
-	fpwm->chip.npwm = 1;
-	fpwm->chip.ops = &apple_pwm_ops;
+	chip->npwm = 1;
+	chip->ops = &apple_pwm_ops;
 
-	ret = devm_pwmchip_add(&pdev->dev, &fpwm->chip);
+	ret = devm_pwmchip_add(&pdev->dev, chip);
 	if (ret < 0)
 		return dev_err_probe(&pdev->dev, ret, "unable to add pwm chip");
 
-- 
2.39.2

[PATCH 04/18] pwm: berlin: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-berlin 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/pwm/pwm-berlin.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
index 8d685d097c21..5b605f554ac5 100644
--- a/drivers/pwm/pwm-berlin.c
+++ b/drivers/pwm/pwm-berlin.c
@@ -48,7 +48,6 @@ struct berlin_pwm_channel {
 };
 
 struct berlin_pwm_chip {
-	struct pwm_chip chip;
 	struct clk *clk;
 	void __iomem *base;
 	struct berlin_pwm_channel channel[BERLIN_PWM_NUMPWMS];
@@ -56,7 +55,7 @@ struct berlin_pwm_chip {
 
 static inline struct berlin_pwm_chip *to_berlin_pwm_chip(struct pwm_chip *chip)
 {
-	return container_of(chip, struct berlin_pwm_chip, chip);
+	return pwmchip_priv(chip);
 }
 
 static inline u32 berlin_pwm_readl(struct berlin_pwm_chip *bpc,
@@ -198,12 +197,14 @@ MODULE_DEVICE_TABLE(of, berlin_pwm_match);
 
 static int berlin_pwm_probe(struct platform_device *pdev)
 {
+	struct pwm_chip *chip;
 	struct berlin_pwm_chip *bpc;
 	int ret;
 
-	bpc = devm_kzalloc(&pdev->dev, sizeof(*bpc), GFP_KERNEL);
-	if (!bpc)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*bpc));
+	if (!chip)
 		return -ENOMEM;
+	bpc = to_berlin_pwm_chip(chip);
 
 	bpc->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(bpc->base))
@@ -213,15 +214,14 @@ static int berlin_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(bpc->clk))
 		return PTR_ERR(bpc->clk);
 
-	bpc->chip.dev = &pdev->dev;
-	bpc->chip.ops = &berlin_pwm_ops;
-	bpc->chip.npwm = BERLIN_PWM_NUMPWMS;
+	chip->ops = &berlin_pwm_ops;
+	chip->npwm = BERLIN_PWM_NUMPWMS;
 
-	ret = devm_pwmchip_add(&pdev->dev, &bpc->chip);
+	ret = devm_pwmchip_add(&pdev->dev, chip);
 	if (ret < 0)
 		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
 
-	platform_set_drvdata(pdev, bpc);
+	platform_set_drvdata(pdev, chip);
 
 	return 0;
 }
@@ -229,10 +229,11 @@ static int berlin_pwm_probe(struct platform_device *pdev)
 #ifdef CONFIG_PM_SLEEP
 static int berlin_pwm_suspend(struct device *dev)
 {
-	struct berlin_pwm_chip *bpc = dev_get_drvdata(dev);
+	struct pwm_chip *chip = dev_get_drvdata(dev);
+	struct berlin_pwm_chip *bpc = to_berlin_pwm_chip(chip);
 	unsigned int i;
 
-	for (i = 0; i < bpc->chip.npwm; i++) {
+	for (i = 0; i < chip->npwm; i++) {
 		struct berlin_pwm_channel *channel = &bpc->channel[i];
 
 		channel->enable = berlin_pwm_readl(bpc, i, BERLIN_PWM_ENABLE);
@@ -248,7 +249,8 @@ static int berlin_pwm_suspend(struct device *dev)
 
 static int berlin_pwm_resume(struct device *dev)
 {
-	struct berlin_pwm_chip *bpc = dev_get_drvdata(dev);
+	struct pwm_chip *chip = dev_get_drvdata(dev);
+	struct berlin_pwm_chip *bpc = to_berlin_pwm_chip(chip);
 	unsigned int i;
 	int ret;
 
@@ -256,7 +258,7 @@ static int berlin_pwm_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	for (i = 0; i < bpc->chip.npwm; i++) {
+	for (i = 0; i < chip->npwm; i++) {
 		struct berlin_pwm_channel *channel = &bpc->channel[i];
 
 		berlin_pwm_writel(bpc, i, channel->ctrl, BERLIN_PWM_CONTROL);
-- 
2.39.2

[PATCH 05/18] pwm: clk: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-clk 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/pwm/pwm-clk.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm-clk.c b/drivers/pwm/pwm-clk.c
index 0ee4d2aee4df..5affa343a059 100644
--- a/drivers/pwm/pwm-clk.c
+++ b/drivers/pwm/pwm-clk.c
@@ -28,12 +28,14 @@
 #include <linux/pwm.h>
 
 struct pwm_clk_chip {
-	struct pwm_chip chip;
 	struct clk *clk;
 	bool clk_enabled;
 };
 
-#define to_pwm_clk_chip(_chip) container_of(_chip, struct pwm_clk_chip, chip)
+static inline struct pwm_clk_chip *to_pwm_clk_chip(struct pwm_chip *chip)
+{
+	return pwmchip_priv(chip);
+}
 
 static int pwm_clk_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			 const struct pwm_state *state)
@@ -82,35 +84,37 @@ static const struct pwm_ops pwm_clk_ops = {
 
 static int pwm_clk_probe(struct platform_device *pdev)
 {
+	struct pwm_chip *chip;
 	struct pwm_clk_chip *pcchip;
 	int ret;
 
-	pcchip = devm_kzalloc(&pdev->dev, sizeof(*pcchip), GFP_KERNEL);
-	if (!pcchip)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*pcchip));
+	if (!chip)
 		return -ENOMEM;
+	pcchip = to_pwm_clk_chip(chip);
 
 	pcchip->clk = devm_clk_get_prepared(&pdev->dev, NULL);
 	if (IS_ERR(pcchip->clk))
 		return dev_err_probe(&pdev->dev, PTR_ERR(pcchip->clk),
 				     "Failed to get clock\n");
 
-	pcchip->chip.dev = &pdev->dev;
-	pcchip->chip.ops = &pwm_clk_ops;
-	pcchip->chip.npwm = 1;
+	chip->ops = &pwm_clk_ops;
+	chip->npwm = 1;
 
-	ret = pwmchip_add(&pcchip->chip);
+	ret = pwmchip_add(chip);
 	if (ret < 0)
 		return dev_err_probe(&pdev->dev, ret, "Failed to add pwm chip\n");
 
-	platform_set_drvdata(pdev, pcchip);
+	platform_set_drvdata(pdev, chip);
 	return 0;
 }
 
 static void pwm_clk_remove(struct platform_device *pdev)
 {
-	struct pwm_clk_chip *pcchip = platform_get_drvdata(pdev);
+	struct pwm_chip *chip = platform_get_drvdata(pdev);
+	struct pwm_clk_chip *pcchip = to_pwm_clk_chip(chip);
 
-	pwmchip_remove(&pcchip->chip);
+	pwmchip_remove(chip);
 
 	if (pcchip->clk_enabled)
 		clk_disable(pcchip->clk);
-- 
2.39.2

[PATCH 06/18] pwm: fsl-ftm: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-fsl-ftm 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/pwm/pwm-fsl-ftm.c | 47 +++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
index 5caadbd6194e..9e01c439dd6a 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -41,7 +41,6 @@ struct fsl_pwm_periodcfg {
 };
 
 struct fsl_pwm_chip {
-	struct pwm_chip chip;
 	struct mutex lock;
 	struct regmap *regmap;
 
@@ -56,7 +55,7 @@ struct fsl_pwm_chip {
 
 static inline struct fsl_pwm_chip *to_fsl_chip(struct pwm_chip *chip)
 {
-	return container_of(chip, struct fsl_pwm_chip, chip);
+	return pwmchip_priv(chip);
 }
 
 static void ftm_clear_write_protection(struct fsl_pwm_chip *fpc)
@@ -222,10 +221,11 @@ static bool fsl_pwm_is_other_pwm_enabled(struct fsl_pwm_chip *fpc,
 		return false;
 }
 
-static int fsl_pwm_apply_config(struct fsl_pwm_chip *fpc,
+static int fsl_pwm_apply_config(struct pwm_chip *chip,
 				struct pwm_device *pwm,
 				const struct pwm_state *newstate)
 {
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
 	unsigned int duty;
 	u32 reg_polarity;
 
@@ -233,7 +233,7 @@ static int fsl_pwm_apply_config(struct fsl_pwm_chip *fpc,
 	bool do_write_period = false;
 
 	if (!fsl_pwm_calculate_period(fpc, newstate->period, &periodcfg)) {
-		dev_err(fpc->chip.dev, "failed to calculate new period\n");
+		dev_err(chip->dev, "failed to calculate new period\n");
 		return -EINVAL;
 	}
 
@@ -247,7 +247,7 @@ static int fsl_pwm_apply_config(struct fsl_pwm_chip *fpc,
 	 */
 	else if (!fsl_pwm_periodcfg_are_equal(&fpc->period, &periodcfg)) {
 		if (fsl_pwm_is_other_pwm_enabled(fpc, pwm)) {
-			dev_err(fpc->chip.dev,
+			dev_err(chip->dev,
 				"Cannot change period for PWM %u, disable other PWMs first\n",
 				pwm->hwpwm);
 			return -EBUSY;
@@ -323,7 +323,7 @@ static int fsl_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		goto end_mutex;
 	}
 
-	ret = fsl_pwm_apply_config(fpc, pwm, newstate);
+	ret = fsl_pwm_apply_config(chip, pwm, newstate);
 	if (ret)
 		goto end_mutex;
 
@@ -394,18 +394,19 @@ static const struct regmap_config fsl_pwm_regmap_config = {
 
 static int fsl_pwm_probe(struct platform_device *pdev)
 {
+	struct pwm_chip *chip;
 	struct fsl_pwm_chip *fpc;
 	void __iomem *base;
 	int ret;
 
-	fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
-	if (!fpc)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*fpc));
+	if (!chip)
 		return -ENOMEM;
+	fpc = to_fsl_chip(chip);
 
 	mutex_init(&fpc->lock);
 
 	fpc->soc = of_device_get_match_data(&pdev->dev);
-	fpc->chip.dev = &pdev->dev;
 
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
@@ -424,16 +425,16 @@ static int fsl_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(fpc->clk[FSL_PWM_CLK_SYS]);
 	}
 
-	fpc->clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
+	fpc->clk[FSL_PWM_CLK_FIX] = devm_clk_get(&pdev->dev, "ftm_fix");
 	if (IS_ERR(fpc->clk[FSL_PWM_CLK_FIX]))
 		return PTR_ERR(fpc->clk[FSL_PWM_CLK_FIX]);
 
-	fpc->clk[FSL_PWM_CLK_EXT] = devm_clk_get(fpc->chip.dev, "ftm_ext");
+	fpc->clk[FSL_PWM_CLK_EXT] = devm_clk_get(&pdev->dev, "ftm_ext");
 	if (IS_ERR(fpc->clk[FSL_PWM_CLK_EXT]))
 		return PTR_ERR(fpc->clk[FSL_PWM_CLK_EXT]);
 
 	fpc->clk[FSL_PWM_CLK_CNTEN] =
-				devm_clk_get(fpc->chip.dev, "ftm_cnt_clk_en");
+				devm_clk_get(&pdev->dev, "ftm_cnt_clk_en");
 	if (IS_ERR(fpc->clk[FSL_PWM_CLK_CNTEN]))
 		return PTR_ERR(fpc->clk[FSL_PWM_CLK_CNTEN]);
 
@@ -446,16 +447,16 @@ static int fsl_pwm_probe(struct platform_device *pdev)
 		fpc->ipg_clk = fpc->clk[FSL_PWM_CLK_SYS];
 
 
-	fpc->chip.ops = &fsl_pwm_ops;
-	fpc->chip.npwm = 8;
+	chip->ops = &fsl_pwm_ops;
+	chip->npwm = 8;
 
-	ret = devm_pwmchip_add(&pdev->dev, &fpc->chip);
+	ret = devm_pwmchip_add(&pdev->dev, chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
 		return ret;
 	}
 
-	platform_set_drvdata(pdev, fpc);
+	platform_set_drvdata(pdev, chip);
 
 	return fsl_pwm_init(fpc);
 }
@@ -463,14 +464,15 @@ static int fsl_pwm_probe(struct platform_device *pdev)
 #ifdef CONFIG_PM_SLEEP
 static int fsl_pwm_suspend(struct device *dev)
 {
-	struct fsl_pwm_chip *fpc = dev_get_drvdata(dev);
+	struct pwm_chip *chip = dev_get_drvdata(dev);
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
 	int i;
 
 	regcache_cache_only(fpc->regmap, true);
 	regcache_mark_dirty(fpc->regmap);
 
-	for (i = 0; i < fpc->chip.npwm; i++) {
-		struct pwm_device *pwm = &fpc->chip.pwms[i];
+	for (i = 0; i < chip->npwm; i++) {
+		struct pwm_device *pwm = &chip->pwms[i];
 
 		if (!test_bit(PWMF_REQUESTED, &pwm->flags))
 			continue;
@@ -489,11 +491,12 @@ static int fsl_pwm_suspend(struct device *dev)
 
 static int fsl_pwm_resume(struct device *dev)
 {
-	struct fsl_pwm_chip *fpc = dev_get_drvdata(dev);
+	struct pwm_chip *chip = dev_get_drvdata(dev);
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
 	int i;
 
-	for (i = 0; i < fpc->chip.npwm; i++) {
-		struct pwm_device *pwm = &fpc->chip.pwms[i];
+	for (i = 0; i < chip->npwm; i++) {
+		struct pwm_device *pwm = &chip->pwms[i];
 
 		if (!test_bit(PWMF_REQUESTED, &pwm->flags))
 			continue;
-- 
2.39.2

[PATCH 07/18] pwm: hibvt: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-hibvt 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/pwm/pwm-hibvt.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-hibvt.c b/drivers/pwm/pwm-hibvt.c
index b95df1a96127..70e2439302b6 100644
--- a/drivers/pwm/pwm-hibvt.c
+++ b/drivers/pwm/pwm-hibvt.c
@@ -33,7 +33,6 @@
 #define PWM_DUTY_MASK       GENMASK(31, 0)
 
 struct hibvt_pwm_chip {
-	struct pwm_chip	chip;
 	struct clk *clk;
 	void __iomem *base;
 	struct reset_control *rstc;
@@ -65,7 +64,7 @@ static const struct hibvt_pwm_soc hi3559v100_soc_info = {
 
 static inline struct hibvt_pwm_chip *to_hibvt_pwm_chip(struct pwm_chip *chip)
 {
-	return container_of(chip, struct hibvt_pwm_chip, chip);
+	return pwmchip_priv(chip);
 }
 
 static void hibvt_pwm_set_bits(void __iomem *base, u32 offset,
@@ -192,12 +191,14 @@ static int hibvt_pwm_probe(struct platform_device *pdev)
 {
 	const struct hibvt_pwm_soc *soc =
 				of_device_get_match_data(&pdev->dev);
+	struct pwm_chip	*chip;
 	struct hibvt_pwm_chip *pwm_chip;
 	int ret, i;
 
-	pwm_chip = devm_kzalloc(&pdev->dev, sizeof(*pwm_chip), GFP_KERNEL);
-	if (pwm_chip == NULL)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*pwm_chip));
+	if (chip == NULL)
 		return -ENOMEM;
+	pwm_chip = to_hibvt_pwm_chip(chip);
 
 	pwm_chip->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pwm_chip->clk)) {
@@ -206,9 +207,8 @@ static int hibvt_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(pwm_chip->clk);
 	}
 
-	pwm_chip->chip.ops = &hibvt_pwm_ops;
-	pwm_chip->chip.dev = &pdev->dev;
-	pwm_chip->chip.npwm = soc->num_pwms;
+	chip->ops = &hibvt_pwm_ops;
+	chip->npwm = soc->num_pwms;
 	pwm_chip->soc = soc;
 
 	pwm_chip->base = devm_platform_ioremap_resource(pdev, 0);
@@ -229,29 +229,31 @@ static int hibvt_pwm_probe(struct platform_device *pdev)
 	msleep(30);
 	reset_control_deassert(pwm_chip->rstc);
 
-	ret = pwmchip_add(&pwm_chip->chip);
+	ret = pwmchip_add(chip);
 	if (ret < 0) {
 		clk_disable_unprepare(pwm_chip->clk);
 		return ret;
 	}
 
-	for (i = 0; i < pwm_chip->chip.npwm; i++) {
+	for (i = 0; i < chip->npwm; i++) {
 		hibvt_pwm_set_bits(pwm_chip->base, PWM_CTRL_ADDR(i),
 				PWM_KEEP_MASK, (0x1 << PWM_KEEP_SHIFT));
 	}
 
-	platform_set_drvdata(pdev, pwm_chip);
+	platform_set_drvdata(pdev, chip);
 
 	return 0;
 }
 
 static void hibvt_pwm_remove(struct platform_device *pdev)
 {
+	struct pwm_chip *chip;
 	struct hibvt_pwm_chip *pwm_chip;
 
-	pwm_chip = platform_get_drvdata(pdev);
+	chip = platform_get_drvdata(pdev);
+	pwm_chip = to_hibvt_pwm_chip(chip);
 
-	pwmchip_remove(&pwm_chip->chip);
+	pwmchip_remove(chip);
 
 	reset_control_assert(pwm_chip->rstc);
 	msleep(30);
-- 
2.39.2

[PATCH 08/18] pwm: imx1: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-imx1 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/pwm/pwm-imx1.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-imx1.c b/drivers/pwm/pwm-imx1.c
index 1f2eb1c8ff6c..e4ecbd6a2e63 100644
--- a/drivers/pwm/pwm-imx1.c
+++ b/drivers/pwm/pwm-imx1.c
@@ -29,10 +29,9 @@ struct pwm_imx1_chip {
 	struct clk *clk_ipg;
 	struct clk *clk_per;
 	void __iomem *mmio_base;
-	struct pwm_chip chip;
 };
 
-#define to_pwm_imx1_chip(chip)	container_of(chip, struct pwm_imx1_chip, chip)
+#define to_pwm_imx1_chip(chip)	pwmchip_priv(chip)
 
 static int pwm_imx1_clk_prepare_enable(struct pwm_chip *chip)
 {
@@ -158,11 +157,13 @@ MODULE_DEVICE_TABLE(of, pwm_imx1_dt_ids);
 
 static int pwm_imx1_probe(struct platform_device *pdev)
 {
+	struct pwm_chip *chip;
 	struct pwm_imx1_chip *imx;
 
-	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
-	if (!imx)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*imx));
+	if (!chip)
 		return -ENOMEM;
+	imx = to_pwm_imx1_chip(chip);
 
 	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
 	if (IS_ERR(imx->clk_ipg))
@@ -174,15 +175,14 @@ static int pwm_imx1_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),
 				     "failed to get peripheral clock\n");
 
-	imx->chip.ops = &pwm_imx1_ops;
-	imx->chip.dev = &pdev->dev;
-	imx->chip.npwm = 1;
+	chip->ops = &pwm_imx1_ops;
+	chip->npwm = 1;
 
 	imx->mmio_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(imx->mmio_base))
 		return PTR_ERR(imx->mmio_base);
 
-	return devm_pwmchip_add(&pdev->dev, &imx->chip);
+	return devm_pwmchip_add(&pdev->dev, chip);
 }
 
 static struct platform_driver pwm_imx1_driver = {
-- 
2.39.2

[PATCH 09/18] pwm: imx27: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-imx27 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/pwm/pwm-imx27.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 29a3089c534c..1f64c4b5b793 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -83,7 +83,6 @@ struct pwm_imx27_chip {
 	struct clk	*clk_ipg;
 	struct clk	*clk_per;
 	void __iomem	*mmio_base;
-	struct pwm_chip	chip;
 
 	/*
 	 * The driver cannot read the current duty cycle from the hardware if
@@ -93,7 +92,10 @@ struct pwm_imx27_chip {
 	unsigned int duty_cycle;
 };
 
-#define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
+static inline struct pwm_imx27_chip *to_pwm_imx27_chip(struct pwm_chip *chip)
+{
+	return pwmchip_priv(chip);
+}
 
 static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
 {
@@ -307,13 +309,15 @@ MODULE_DEVICE_TABLE(of, pwm_imx27_dt_ids);
 
 static int pwm_imx27_probe(struct platform_device *pdev)
 {
+	struct pwm_chip *chip;
 	struct pwm_imx27_chip *imx;
 	int ret;
 	u32 pwmcr;
 
-	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
-	if (imx == NULL)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*imx));
+	if (!chip)
 		return -ENOMEM;
+	imx = pwmchip_priv(chip);
 
 	imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
 	if (IS_ERR(imx->clk_ipg))
@@ -325,9 +329,8 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(imx->clk_per),
 				     "failed to get peripheral clock\n");
 
-	imx->chip.ops = &pwm_imx27_ops;
-	imx->chip.dev = &pdev->dev;
-	imx->chip.npwm = 1;
+	chip->ops = &pwm_imx27_ops;
+	chip->npwm = 1;
 
 	imx->mmio_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(imx->mmio_base))
@@ -342,7 +345,7 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 	if (!(pwmcr & MX3_PWMCR_EN))
 		pwm_imx27_clk_disable_unprepare(imx);
 
-	return devm_pwmchip_add(&pdev->dev, &imx->chip);
+	return devm_pwmchip_add(&pdev->dev, chip);
 }
 
 static struct platform_driver imx_pwm_driver = {
-- 
2.39.2

[PATCH 10/18] pwm: jz4740: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-jz4740 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/pwm/pwm-jz4740.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 8b01819df67d..de3b891f1ceb 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -25,23 +25,22 @@ struct soc_info {
 };
 
 struct jz4740_pwm_chip {
-	struct pwm_chip chip;
 	struct regmap *map;
 	struct clk *clk[];
 };
 
 static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
 {
-	return container_of(chip, struct jz4740_pwm_chip, chip);
+	return pwmchip_priv(chip);
 }
 
-static bool jz4740_pwm_can_use_chn(struct jz4740_pwm_chip *jz,
+static bool jz4740_pwm_can_use_chn(struct pwm_chip *chip,
 				   unsigned int channel)
 {
 	/* Enable all TCU channels for PWM use by default except channels 0/1 */
-	u32 pwm_channels_mask = GENMASK(jz->chip.npwm - 1, 2);
+	u32 pwm_channels_mask = GENMASK(chip->npwm - 1, 2);
 
-	device_property_read_u32(jz->chip.dev->parent,
+	device_property_read_u32(chip->dev->parent,
 				 "ingenic,pwm-channels-mask",
 				 &pwm_channels_mask);
 
@@ -55,7 +54,7 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	char name[16];
 	int err;
 
-	if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
+	if (!jz4740_pwm_can_use_chn(chip, pwm->hwpwm))
 		return -EBUSY;
 
 	snprintf(name, sizeof(name), "timer%u", pwm->hwpwm);
@@ -224,6 +223,7 @@ static const struct pwm_ops jz4740_pwm_ops = {
 static int jz4740_pwm_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct pwm_chip *chip;
 	struct jz4740_pwm_chip *jz4740;
 	const struct soc_info *info;
 
@@ -231,10 +231,10 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
 	if (!info)
 		return -EINVAL;
 
-	jz4740 = devm_kzalloc(dev, struct_size(jz4740, clk, info->num_pwms),
-			      GFP_KERNEL);
-	if (!jz4740)
+	chip = devm_pwmchip_alloc(dev, struct_size(jz4740, clk, info->num_pwms));
+	if (!chip)
 		return -ENOMEM;
+	jz4740 = to_jz4740(chip);
 
 	jz4740->map = device_node_to_regmap(dev->parent->of_node);
 	if (IS_ERR(jz4740->map)) {
@@ -242,11 +242,10 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(jz4740->map);
 	}
 
-	jz4740->chip.dev = dev;
-	jz4740->chip.ops = &jz4740_pwm_ops;
-	jz4740->chip.npwm = info->num_pwms;
+	chip->ops = &jz4740_pwm_ops;
+	chip->npwm = info->num_pwms;
 
-	return devm_pwmchip_add(dev, &jz4740->chip);
+	return devm_pwmchip_add(dev, chip);
 }
 
 static const struct soc_info jz4740_soc_info = {
-- 
2.39.2

[PATCH 11/18] pwm: keembay: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-keembay 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/pwm/pwm-keembay.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-keembay.c b/drivers/pwm/pwm-keembay.c
index ac02d8bb4a0b..ceef94080823 100644
--- a/drivers/pwm/pwm-keembay.c
+++ b/drivers/pwm/pwm-keembay.c
@@ -36,7 +36,6 @@
 #define KMB_PWM_HIGHLOW_OFFSET(ch)	(0x20 + 4 * (ch))
 
 struct keembay_pwm {
-	struct pwm_chip chip;
 	struct device *dev;
 	struct clk *clk;
 	void __iomem *base;
@@ -44,7 +43,7 @@ struct keembay_pwm {
 
 static inline struct keembay_pwm *to_keembay_pwm_dev(struct pwm_chip *chip)
 {
-	return container_of(chip, struct keembay_pwm, chip);
+	return pwmchip_priv(chip);
 }
 
 static void keembay_clk_unprepare(void *data)
@@ -186,12 +185,14 @@ static const struct pwm_ops keembay_pwm_ops = {
 static int keembay_pwm_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct pwm_chip *chip;
 	struct keembay_pwm *priv;
 	int ret;
 
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
+	chip = devm_pwmchip_alloc(dev, sizeof(*priv));
+	if (!chip)
 		return -ENOMEM;
+	priv = to_keembay_pwm_dev(chip);
 
 	priv->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(priv->clk))
@@ -205,11 +206,10 @@ static int keembay_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	priv->chip.dev = dev;
-	priv->chip.ops = &keembay_pwm_ops;
-	priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
+	chip->ops = &keembay_pwm_ops;
+	chip->npwm = KMB_TOTAL_PWM_CHANNELS;
 
-	ret = devm_pwmchip_add(dev, &priv->chip);
+	ret = devm_pwmchip_add(dev, chip);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to add PWM chip\n");
 
-- 
2.39.2

[PATCH 12/18] pwm: lpc18xx-sct: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-lpc18xx-sct 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/pwm/pwm-lpc18xx-sct.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
index 0bf0893a5d52..13d4eb9d533f 100644
--- a/drivers/pwm/pwm-lpc18xx-sct.c
+++ b/drivers/pwm/pwm-lpc18xx-sct.c
@@ -92,7 +92,6 @@ struct lpc18xx_pwm_data {
 
 struct lpc18xx_pwm_chip {
 	struct device *dev;
-	struct pwm_chip chip;
 	void __iomem *base;
 	struct clk *pwm_clk;
 	unsigned long clk_rate;
@@ -109,7 +108,7 @@ struct lpc18xx_pwm_chip {
 static inline struct lpc18xx_pwm_chip *
 to_lpc18xx_pwm_chip(struct pwm_chip *chip)
 {
-	return container_of(chip, struct lpc18xx_pwm_chip, chip);
+	return pwmchip_priv(chip);
 }
 
 static inline void lpc18xx_pwm_writel(struct lpc18xx_pwm_chip *lpc18xx_pwm,
@@ -351,14 +350,15 @@ MODULE_DEVICE_TABLE(of, lpc18xx_pwm_of_match);
 
 static int lpc18xx_pwm_probe(struct platform_device *pdev)
 {
+	struct pwm_chip *chip;
 	struct lpc18xx_pwm_chip *lpc18xx_pwm;
 	int ret;
 	u64 val;
 
-	lpc18xx_pwm = devm_kzalloc(&pdev->dev, sizeof(*lpc18xx_pwm),
-				   GFP_KERNEL);
-	if (!lpc18xx_pwm)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*lpc18xx_pwm));
+	if (!chip)
 		return -ENOMEM;
+	lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
 
 	lpc18xx_pwm->dev = &pdev->dev;
 
@@ -391,9 +391,8 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
 	lpc18xx_pwm->min_period_ns = DIV_ROUND_UP(NSEC_PER_SEC,
 						  lpc18xx_pwm->clk_rate);
 
-	lpc18xx_pwm->chip.dev = &pdev->dev;
-	lpc18xx_pwm->chip.ops = &lpc18xx_pwm_ops;
-	lpc18xx_pwm->chip.npwm = LPC18XX_NUM_PWMS;
+	chip->ops = &lpc18xx_pwm_ops;
+	chip->npwm = LPC18XX_NUM_PWMS;
 
 	/* SCT counter must be in unify (32 bit) mode */
 	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CONFIG,
@@ -425,21 +424,22 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
 	val |= LPC18XX_PWM_PRE(0);
 	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL, val);
 
-	ret = pwmchip_add(&lpc18xx_pwm->chip);
+	ret = pwmchip_add(chip);
 	if (ret < 0)
 		return dev_err_probe(&pdev->dev, ret, "pwmchip_add failed\n");
 
-	platform_set_drvdata(pdev, lpc18xx_pwm);
+	platform_set_drvdata(pdev, chip);
 
 	return 0;
 }
 
 static void lpc18xx_pwm_remove(struct platform_device *pdev)
 {
-	struct lpc18xx_pwm_chip *lpc18xx_pwm = platform_get_drvdata(pdev);
+	struct pwm_chip *chip = platform_get_drvdata(pdev);
+	struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
 	u32 val;
 
-	pwmchip_remove(&lpc18xx_pwm->chip);
+	pwmchip_remove(chip);
 
 	val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
 	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL,
-- 
2.39.2

[PATCH 13/18] pwm: lpc32xx: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-lpc32xx 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/pwm/pwm-lpc32xx.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
index 806f0bb3ad6d..ea891a8be12f 100644
--- a/drivers/pwm/pwm-lpc32xx.c
+++ b/drivers/pwm/pwm-lpc32xx.c
@@ -15,7 +15,6 @@
 #include <linux/slab.h>
 
 struct lpc32xx_pwm_chip {
-	struct pwm_chip chip;
 	struct clk *clk;
 	void __iomem *base;
 };
@@ -23,8 +22,8 @@ struct lpc32xx_pwm_chip {
 #define PWM_ENABLE	BIT(31)
 #define PWM_PIN_LEVEL	BIT(30)
 
-#define to_lpc32xx_pwm_chip(_chip) \
-	container_of(_chip, struct lpc32xx_pwm_chip, chip)
+#define to_lpc32xx_pwm_chip(chip) \
+	pwmchip_priv(chip)
 
 static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			      int duty_ns, int period_ns)
@@ -120,13 +119,15 @@ static const struct pwm_ops lpc32xx_pwm_ops = {
 
 static int lpc32xx_pwm_probe(struct platform_device *pdev)
 {
+	struct pwm_chip *chip;
 	struct lpc32xx_pwm_chip *lpc32xx;
 	int ret;
 	u32 val;
 
-	lpc32xx = devm_kzalloc(&pdev->dev, sizeof(*lpc32xx), GFP_KERNEL);
-	if (!lpc32xx)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*lpc32xx));
+	if (!chip)
 		return -ENOMEM;
+	lpc32xx = to_lpc32xx_pwm_chip(chip);
 
 	lpc32xx->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(lpc32xx->base))
@@ -136,16 +137,15 @@ static int lpc32xx_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(lpc32xx->clk))
 		return PTR_ERR(lpc32xx->clk);
 
-	lpc32xx->chip.dev = &pdev->dev;
-	lpc32xx->chip.ops = &lpc32xx_pwm_ops;
-	lpc32xx->chip.npwm = 1;
+	chip->ops = &lpc32xx_pwm_ops;
+	chip->npwm = 1;
 
 	/* If PWM is disabled, configure the output to the default value */
 	val = readl(lpc32xx->base);
 	val &= ~PWM_PIN_LEVEL;
 	writel(val, lpc32xx->base);
 
-	ret = devm_pwmchip_add(&pdev->dev, &lpc32xx->chip);
+	ret = devm_pwmchip_add(&pdev->dev, chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to add PWM chip, error %d\n", ret);
 		return ret;
-- 
2.39.2

[PATCH 14/18] pwm: mxs: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-mxs 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/pwm/pwm-mxs.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
index 766dbc58dad8..62f8ab499d44 100644
--- a/drivers/pwm/pwm-mxs.c
+++ b/drivers/pwm/pwm-mxs.c
@@ -37,12 +37,14 @@ static const u8 cdiv_shift[PERIOD_CDIV_MAX] = {
 };
 
 struct mxs_pwm_chip {
-	struct pwm_chip chip;
 	struct clk *clk;
 	void __iomem *base;
 };
 
-#define to_mxs_pwm_chip(_chip) container_of(_chip, struct mxs_pwm_chip, chip)
+static inline struct mxs_pwm_chip *to_mxs_pwm_chip(struct pwm_chip *chip)
+{
+	return pwmchip_priv(chip);
+}
 
 static int mxs_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			 const struct pwm_state *state)
@@ -121,12 +123,14 @@ static const struct pwm_ops mxs_pwm_ops = {
 static int mxs_pwm_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct pwm_chip *chip;
 	struct mxs_pwm_chip *mxs;
 	int ret;
 
-	mxs = devm_kzalloc(&pdev->dev, sizeof(*mxs), GFP_KERNEL);
-	if (!mxs)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*mxs));
+	if (!chip)
 		return -ENOMEM;
+	mxs = to_mxs_pwm_chip(chip);
 
 	mxs->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(mxs->base))
@@ -136,10 +140,9 @@ static int mxs_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(mxs->clk))
 		return PTR_ERR(mxs->clk);
 
-	mxs->chip.dev = &pdev->dev;
-	mxs->chip.ops = &mxs_pwm_ops;
+	chip->ops = &mxs_pwm_ops;
 
-	ret = of_property_read_u32(np, "fsl,pwm-number", &mxs->chip.npwm);
+	ret = of_property_read_u32(np, "fsl,pwm-number", &chip->npwm);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
 		return ret;
@@ -150,7 +153,7 @@ static int mxs_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret, "failed to reset PWM\n");
 
-	ret = devm_pwmchip_add(&pdev->dev, &mxs->chip);
+	ret = devm_pwmchip_add(&pdev->dev, chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret);
 		return ret;
-- 
2.39.2

[PATCH 15/18] pwm: ntxec: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-ntxec 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/pwm/pwm-ntxec.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-ntxec.c b/drivers/pwm/pwm-ntxec.c
index 7514ea384ec5..6f266c24d012 100644
--- a/drivers/pwm/pwm-ntxec.c
+++ b/drivers/pwm/pwm-ntxec.c
@@ -25,12 +25,11 @@
 
 struct ntxec_pwm {
 	struct ntxec *ec;
-	struct pwm_chip chip;
 };
 
 static struct ntxec_pwm *ntxec_pwm_from_chip(struct pwm_chip *chip)
 {
-	return container_of(chip, struct ntxec_pwm, chip);
+	return pwmchip_priv(chip);
 }
 
 #define NTXEC_REG_AUTO_OFF_HI	0xa1
@@ -57,7 +56,7 @@ static struct ntxec_pwm *ntxec_pwm_from_chip(struct pwm_chip *chip)
 static int ntxec_pwm_set_raw_period_and_duty_cycle(struct pwm_chip *chip,
 						   int period, int duty)
 {
-	struct ntxec_pwm *priv = ntxec_pwm_from_chip(chip);
+	struct ntxec *ec = ntxec_pwm_from_chip(chip)->ec;
 
 	/*
 	 * Changes to the period and duty cycle take effect as soon as the
@@ -77,13 +76,13 @@ static int ntxec_pwm_set_raw_period_and_duty_cycle(struct pwm_chip *chip,
 		{ NTXEC_REG_DUTY_LOW, ntxec_reg8(duty) },
 	};
 
-	return regmap_multi_reg_write(priv->ec->regmap, regs, ARRAY_SIZE(regs));
+	return regmap_multi_reg_write(ec->regmap, regs, ARRAY_SIZE(regs));
 }
 
 static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
 			   const struct pwm_state *state)
 {
-	struct ntxec_pwm *priv = ntxec_pwm_from_chip(chip);
+	struct ntxec *ec = ntxec_pwm_from_chip(chip)->ec;
 	unsigned int period, duty;
 	int res;
 
@@ -110,18 +109,18 @@ static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
 		if (res)
 			return res;
 
-		res = regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
+		res = regmap_write(ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
 		if (res)
 			return res;
 
 		/* Disable the auto-off timer */
-		res = regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
+		res = regmap_write(ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
 		if (res)
 			return res;
 
-		return regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
+		return regmap_write(ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
 	} else {
-		return regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
+		return regmap_write(ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
 	}
 }
 
@@ -136,20 +135,18 @@ static const struct pwm_ops ntxec_pwm_ops = {
 
 static int ntxec_pwm_probe(struct platform_device *pdev)
 {
-	struct ntxec *ec = dev_get_drvdata(pdev->dev.parent);
 	struct ntxec_pwm *priv;
 	struct pwm_chip *chip;
 
 	device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);
 
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*priv));
+	if (!chip)
 		return -ENOMEM;
+	priv = ntxec_pwm_from_chip(chip);
 
-	priv->ec = ec;
+	priv->ec = dev_get_drvdata(pdev->dev.parent);
 
-	chip = &priv->chip;
-	chip->dev = &pdev->dev;
 	chip->ops = &ntxec_pwm_ops;
 	chip->npwm = 1;
 
-- 
2.39.2

[PATCH 16/18] pwm: pxa: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-pxa 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/pwm/pwm-pxa.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index 57e0f7b58b3a..d37f81e6eedc 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -48,7 +48,6 @@ MODULE_DEVICE_TABLE(platform, pwm_id_table);
 #define PWMDCR_FD	(1 << 10)
 
 struct pxa_pwm_chip {
-	struct pwm_chip	chip;
 	struct device	*dev;
 
 	struct clk	*clk;
@@ -57,7 +56,7 @@ struct pxa_pwm_chip {
 
 static inline struct pxa_pwm_chip *to_pxa_pwm_chip(struct pwm_chip *chip)
 {
-	return container_of(chip, struct pxa_pwm_chip, chip);
+	return pwmchip_priv(chip);
 }
 
 /*
@@ -159,6 +158,7 @@ MODULE_DEVICE_TABLE(of, pwm_of_match);
 static int pwm_probe(struct platform_device *pdev)
 {
 	const struct platform_device_id *id = platform_get_device_id(pdev);
+	struct pwm_chip *chip;
 	struct pxa_pwm_chip *pc;
 	int ret = 0;
 
@@ -168,28 +168,28 @@ static int pwm_probe(struct platform_device *pdev)
 	if (id == NULL)
 		return -EINVAL;
 
-	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
-	if (pc == NULL)
+	chip = devm_pwmchip_alloc(&pdev->dev, sizeof(*pc));
+	if (chip == NULL)
 		return -ENOMEM;
+	pc = to_pxa_pwm_chip(chip);
 
 	pc->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pc->clk))
 		return PTR_ERR(pc->clk);
 
-	pc->chip.dev = &pdev->dev;
-	pc->chip.ops = &pxa_pwm_ops;
-	pc->chip.npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
+	chip->ops = &pxa_pwm_ops;
+	chip->npwm = (id->driver_data & HAS_SECONDARY_PWM) ? 2 : 1;
 
 	if (IS_ENABLED(CONFIG_OF)) {
-		pc->chip.of_xlate = of_pwm_single_xlate;
-		pc->chip.of_pwm_n_cells = 1;
+		chip->of_xlate = of_pwm_single_xlate;
+		chip->of_pwm_n_cells = 1;
 	}
 
 	pc->mmio_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pc->mmio_base))
 		return PTR_ERR(pc->mmio_base);
 
-	ret = devm_pwmchip_add(&pdev->dev, &pc->chip);
+	ret = devm_pwmchip_add(&pdev->dev, chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
 		return ret;
-- 
2.39.2

[PATCH 17/18] pwm: stm32: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm-stm32 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/pwm/pwm-stm32.c | 42 ++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 3d6be7749e23..ade89b169bd1 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -27,7 +27,6 @@ struct stm32_breakinput {
 };
 
 struct stm32_pwm {
-	struct pwm_chip chip;
 	struct mutex lock; /* protect pwm config/enable */
 	struct clk *clk;
 	struct regmap *regmap;
@@ -40,7 +39,7 @@ struct stm32_pwm {
 
 static inline struct stm32_pwm *to_stm32_pwm_dev(struct pwm_chip *chip)
 {
-	return container_of(chip, struct stm32_pwm, chip);
+	return pwmchip_priv(chip);
 }
 
 static u32 active_channels(struct stm32_pwm *dev)
@@ -109,7 +108,7 @@ static int stm32_pwm_raw_capture(struct stm32_pwm *priv, struct pwm_device *pwm,
 				 unsigned long tmo_ms, u32 *raw_prd,
 				 u32 *raw_dty)
 {
-	struct device *parent = priv->chip.dev->parent;
+	struct device *parent = pwm->chip->dev->parent;
 	enum stm32_timers_dmas dma_id;
 	u32 ccen, ccr;
 	int ret;
@@ -185,7 +184,7 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	ret = clk_enable(priv->clk);
 	if (ret) {
-		dev_err(priv->chip.dev, "failed to enable counter clock\n");
+		dev_err(chip->dev, "failed to enable counter clock\n");
 		goto unlock;
 	}
 
@@ -451,6 +450,7 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
 	int ret;
 
+	dev_info(chip->dev, "%s:%d, priv=%px\n", __func__, __LINE__, priv);
 	enabled = pwm->state.enabled;
 
 	if (enabled && !state->enabled) {
@@ -613,11 +613,14 @@ static int stm32_pwm_probe(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 	struct stm32_timers *ddata = dev_get_drvdata(pdev->dev.parent);
 	struct stm32_pwm *priv;
+	struct pwm_chip *chip;
 	int ret;
 
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
+	chip = devm_pwmchip_alloc(dev, sizeof(*priv));
+	if (!chip)
 		return -ENOMEM;
+	priv = to_stm32_pwm_dev(chip);
+	dev_info(chip->dev, "%s:%d, priv=%px\n", __func__, __LINE__, priv);
 
 	mutex_init(&priv->lock);
 	priv->regmap = ddata->regmap;
@@ -633,33 +636,40 @@ static int stm32_pwm_probe(struct platform_device *pdev)
 
 	stm32_pwm_detect_complementary(priv);
 
-	priv->chip.dev = dev;
-	priv->chip.ops = &stm32pwm_ops;
-	priv->chip.npwm = stm32_pwm_detect_channels(priv);
+	chip->ops = &stm32pwm_ops;
+	chip->npwm = stm32_pwm_detect_channels(priv);
 
-	ret = devm_pwmchip_add(dev, &priv->chip);
+	ret = devm_pwmchip_add(dev, chip);
 	if (ret < 0)
 		return ret;
 
-	platform_set_drvdata(pdev, priv);
+	platform_set_drvdata(pdev, chip);
 
 	return 0;
 }
 
+static void stm32_pwm_remove(struct platform_device *pdev)
+{
+	struct pwm_chip *chip = platform_get_drvdata(pdev);
+	dev_info(&pdev->dev, "%s:%d: chip=%px\n", __func__, __LINE__, chip);
+	chip->ops = 0;
+}
+
 static int __maybe_unused stm32_pwm_suspend(struct device *dev)
 {
-	struct stm32_pwm *priv = dev_get_drvdata(dev);
+	struct pwm_chip *chip = dev_get_drvdata(dev);
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
 	unsigned int i;
 	u32 ccer, mask;
 
 	/* Look for active channels */
 	ccer = active_channels(priv);
 
-	for (i = 0; i < priv->chip.npwm; i++) {
+	for (i = 0; i < chip->npwm; i++) {
 		mask = TIM_CCER_CC1E << (i * 4);
 		if (ccer & mask) {
 			dev_err(dev, "PWM %u still in use by consumer %s\n",
-				i, priv->chip.pwms[i].label);
+				i, chip->pwms[i].label);
 			return -EBUSY;
 		}
 	}
@@ -669,7 +679,8 @@ static int __maybe_unused stm32_pwm_suspend(struct device *dev)
 
 static int __maybe_unused stm32_pwm_resume(struct device *dev)
 {
-	struct stm32_pwm *priv = dev_get_drvdata(dev);
+	struct pwm_chip *chip = dev_get_drvdata(dev);
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
 	int ret;
 
 	ret = pinctrl_pm_select_default_state(dev);
@@ -690,6 +701,7 @@ MODULE_DEVICE_TABLE(of, stm32_pwm_of_match);
 
 static struct platform_driver stm32_pwm_driver = {
 	.probe	= stm32_pwm_probe,
+	.remove_new = stm32_pwm_remove,
 	.driver	= {
 		.name = "stm32-pwm",
 		.of_match_table = stm32_pwm_of_match,
-- 
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 01/18] pwm: Provide devm_pwmchip_alloc() function

[Andy Shevchenko]

On Tue, Jul 18, 2023 at 08:18:32PM +0200, Uwe Kleine-König wrote:
> This function allocates a struct pwm_chip and driver data. Compared to
> the status quo the split into pwm_chip and driver data is new, otherwise
> it doesn't change anything relevant (yet).
> 
> The intention is that after all drivers are switched to use this
> allocation function, its possible to add a struct device to struct
> pwm_chip to properly track the latter's lifetime without touching all
> drivers again. Proper lifetime tracking is a necessary precondition to
> introduce character device support for PWMs (that implements atomic
> setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm
> userspace support).
> 
> The new function pwmchip_priv() (obviously?) only works for chips
> allocated with devm_pwmchip_alloc().

...

> +void *pwmchip_priv(struct pwm_chip *chip)
> +{
> +	return (char *)chip + ALIGN(sizeof(*chip), 32);

Why 32? I haven't found any explanation on the choice. I can understand arch
minimum align, but hard coded value is a bit hard to get.

> +}

...

> +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, size_t sizeof_priv)
> +{
> +	struct pwm_chip *chip;
> +	size_t alloc_size;

> +	alloc_size = ALIGN(sizeof(*chip), 32) + sizeof_priv;

Ditto.

Shouldn't it use a macro from overflow.h?

> +	chip = devm_kzalloc(parent, alloc_size, GFP_KERNEL);
> +	if (!chip)
> +		return NULL;
> +
> +	chip->dev = parent;
> +
> +	return chip;
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Thierry Reding]

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

On Tue, Jul 18, 2023 at 08:18:32PM +0200, Uwe Kleine-König wrote:
> This function allocates a struct pwm_chip and driver data. Compared to
> the status quo the split into pwm_chip and driver data is new, otherwise
> it doesn't change anything relevant (yet).
> 
> The intention is that after all drivers are switched to use this
> allocation function, its possible to add a struct device to struct
> pwm_chip to properly track the latter's lifetime without touching all
> drivers again. Proper lifetime tracking is a necessary precondition to
> introduce character device support for PWMs (that implements atomic
> setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm
> userspace support).
> 
> The new function pwmchip_priv() (obviously?) only works for chips
> allocated with devm_pwmchip_alloc().

If this is supposed to be similar to the GPIO chardev, why doesn't GPIO
require this way of allocating a struct gpio_chip? I'm not a fan of
doing all this upfront work without seeing where this is ultimately
headed. Please hold off on reworking everything until you have a
complete proposal that can be reviewed in full.

Thierry

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

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

[Uwe Kleine-König]

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

Hello Thierry,

On Wed, Jul 19, 2023 at 09:59:29AM +0200, Thierry Reding wrote:
> On Tue, Jul 18, 2023 at 08:18:32PM +0200, Uwe Kleine-König wrote:
> > This function allocates a struct pwm_chip and driver data. Compared to
> > the status quo the split into pwm_chip and driver data is new, otherwise
> > it doesn't change anything relevant (yet).
> > 
> > The intention is that after all drivers are switched to use this
> > allocation function, its possible to add a struct device to struct
> > pwm_chip to properly track the latter's lifetime without touching all
> > drivers again. Proper lifetime tracking is a necessary precondition to
> > introduce character device support for PWMs (that implements atomic
> > setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm
> > userspace support).
> > 
> > The new function pwmchip_priv() (obviously?) only works for chips
> > allocated with devm_pwmchip_alloc().
> 
> If this is supposed to be similar to the GPIO chardev, why doesn't GPIO
> require this way of allocating a struct gpio_chip?

For each gpio_chip there is an external struct gpio_device that is
allocated in gpiochip_add(). This works but has some downsides. E.g. 
gpiochip_remove() has to grab a mutex that is also held while userspace
polls a gpioctrl device and so gpiochip_remove blocks. While this works
(as in: it doesn't crash) it's in the category "not as good as
possible".

I'm convinced that getting this right before adding the complexity of
chardev support is a good idea.

> I'm not a fan of doing all this upfront work without seeing where this
> is ultimately headed. Please hold off on reworking everything until
> you have a complete proposal that can be reviewed in full.

I see some benefit of the conversion started in this patch set stand
alone, too. But I agree it's for now a bit theoretic.

With the goal to pick the agreed good approach for pwm this is a bigger
pile of work. Completing it in private and only present the complete
proposal has the downside that I get feedback for the house's seating
only when the roof is assembled. While me spending much time isn't your
problem, I have a problem with it. Splitting the task in chunks that go
in one after another is surely the quicker and more effective approach.

So it would be great if you could understand the issue tackled here even
though there are subsystems that do it differently (and less optimal)
and the usecase isn't complete yet. 

Thanks
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 01/18] pwm: Provide devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

Hello Andy,

On Tue, Jul 18, 2023 at 11:05:29PM +0300, Andy Shevchenko wrote:
> On Tue, Jul 18, 2023 at 08:18:32PM +0200, Uwe Kleine-König wrote:
> > This function allocates a struct pwm_chip and driver data. Compared to
> > the status quo the split into pwm_chip and driver data is new, otherwise
> > it doesn't change anything relevant (yet).
> > 
> > The intention is that after all drivers are switched to use this
> > allocation function, its possible to add a struct device to struct
> > pwm_chip to properly track the latter's lifetime without touching all
> > drivers again. Proper lifetime tracking is a necessary precondition to
> > introduce character device support for PWMs (that implements atomic
> > setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm
> > userspace support).
> > 
> > The new function pwmchip_priv() (obviously?) only works for chips
> > allocated with devm_pwmchip_alloc().
> 
> ...
> 
> > +void *pwmchip_priv(struct pwm_chip *chip)
> > +{
> > +	return (char *)chip + ALIGN(sizeof(*chip), 32);
> 
> Why 32? I haven't found any explanation on the choice. I can understand arch
> minimum align, but hard coded value is a bit hard to get.

I copied that part from netdev_priv (without introducing the equivalent
of NETDEV_ALIGN). The 32 there isn't motivated in a comment, and i
didn't think much about it. Alternatives that I'm aware of are:

	dma_get_cache_alignment() (spi)
	Use a struct (counter's counter_device_allochelper uses ____cacheline_aligned)

(but I wonder about dma_get_cache_alignment() which might return 1 on
some archs and then doesn't even ensure natural aligning for shorts.)

> > +}
> 
> ...
> 
> > +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, size_t sizeof_priv)
> > +{
> > +	struct pwm_chip *chip;
> > +	size_t alloc_size;
> 
> > +	alloc_size = ALIGN(sizeof(*chip), 32) + sizeof_priv;
> 
> Ditto.

Of course this needs to match the above. spi uses dev_get_drvdata on the
controller's dev. Also a nice idea.

> Shouldn't it use a macro from overflow.h?

Yupp, that would make sense. Something like:

	if (unlikely(check_add_overflow(ALIGN(sizeof(*chip), 32), sizeof_priv, &alloc_size)))
		return NULL;

(with 32 replaced by something with a better name.)

Thanks for your feedback,
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 01/18] pwm: Provide devm_pwmchip_alloc() function

[Uwe Kleine-König]

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

On Wed, Jul 19, 2023 at 09:59:29AM +0200, Thierry Reding wrote:
> On Tue, Jul 18, 2023 at 08:18:32PM +0200, Uwe Kleine-König wrote:
> > This function allocates a struct pwm_chip and driver data. Compared to
> > the status quo the split into pwm_chip and driver data is new, otherwise
> > it doesn't change anything relevant (yet).
> > 
> > The intention is that after all drivers are switched to use this
> > allocation function, its possible to add a struct device to struct
> > pwm_chip to properly track the latter's lifetime without touching all
> > drivers again. Proper lifetime tracking is a necessary precondition to
> > introduce character device support for PWMs (that implements atomic
> > setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm
> > userspace support).
> > 
> > The new function pwmchip_priv() (obviously?) only works for chips
> > allocated with devm_pwmchip_alloc().
> 
> If this is supposed to be similar to the GPIO chardev, why doesn't GPIO
> require this way of allocating a struct gpio_chip? I'm not a fan of
> doing all this upfront work without seeing where this is ultimately
> headed. Please hold off on reworking everything until you have a
> complete proposal that can be reviewed in full.

I'm working on that and already have a patch stack with more than 100
patches, many of them are cleanups that I found while working on each
PWM driver (most of these are already posted to the linux-pwm list). The
biggest part is to convert each of the 50+ pwm drivers to
pwmchip_alloc().

Today I managed to trigger the problem I intend to address with this
series. My machine to test this on is an stm32mp157. To be able to
trigger the problem reliably I applied the following patches on top of
v6.5-rc1:

 - pwm: stm32: Don't modify HW state in .remove() callback
   This is a cleanup that I already sent out.
   https://lore.kernel.org/r/20230713155142.2454010-2-u.kleine-koenig@pengutronix.de
   The purpose for reproducing the problem is to not trigger further
   calls to the apply callback.

 - The following patch:

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 687967d3265f..c7fc02b0fa3c 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -451,6 +451,10 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
 	int ret;
 
+	dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
+	msleep(5000);
+	dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
+
 	enabled = pwm->state.enabled;
 
 	if (enabled && !state->enabled) {
@@ -650,7 +654,11 @@ static void stm32_pwm_remove(struct platform_device *pdev)
 {
 	struct stm32_pwm *priv = platform_get_drvdata(pdev);
 
+	dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
 	pwmchip_remove(&priv->chip);
+	dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
+
+	priv->regmap = NULL;
 }
 
 static int __maybe_unused stm32_pwm_suspend(struct device *dev)

The first hunk is only there to widen the race window. The second is to
give some diagnostics and make stm32_pwm_apply() crash if it continues
to run after the msleep. (Without it it didn't crash reproducibly, don't
understand why. *shrug*)

The device tree contains a pwm-fan device making use of one of the PWMs.

Now I do the following:

	echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind

Unbinding the fan device has two effects:

 - The device link between fan and pwm looses its property to unbind fan
   when pwm gets unbound.
   (Its .status changes from DL_STATE_ACTIVE to DL_STATE_AVAILABLE)
 - It calls pwm_fan_cleanup() which triggers a call to
   pwm_apply_state().

So when the pwm device gets unbound the first thread is sleeping in
stm32_pwm_apply(). The driver calls pwmchip_remove() and sets
priv->regmap to NULL. Then a few seconds later the first thread wakes up
in stm32_pwm_apply() with the chip freed and priv->regmap = NULL. Bang!

This looks as follows:

root@crown:~# echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
[  187.182113] stm32-pwm 40007000.timer:pwm: stm32_pwm_apply:454
[  188.164769] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:657
[  188.184555] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:659
root@crown:~# [  192.236423] platform 40007000.timer:pwm: stm32_pwm_apply:456
[  192.240727] 8<--- cut here ---
[  192.243759] Unable to handle kernel NULL pointer dereference at virtual address 0000001c when read
...

Even without the crash you can see that stm32_pwm_apply() is still
running after pwmchip_remove() completed.

I'm unsure if the device link could be improved here to ensure that the
fan is completely unbound even if it started unbinding already before
the pwm device gets unbound. (And if it could, would this fit the device
links purpose and so be a sensible improvement?)

If not, the only possible other fix is to make sure that the pwm
callbacks are serialized with pwmchip_remove() and stop calling the
driver callbacks when pwmchip_remove() was called. For that the
pwm_chip struct must stay around until all consumers are really gone. So
the pwm_chip must not be allocated using devm_kzalloc for the pwm's
parent device.

Am I missing something? Is this good enough to convince you that we need
the concept of devm_pwmchip_alloc() from this thread?

My preferred way to continue fixing that would be to get all the
preparing cleanups into next soon to keep my patch stack smaller.
Another pre-condition to serializing the pwm callbacks is (I think) that
all low-level drivers must be fixed to not call pwm-API functions[1].

Then I'll prepare switching all drivers based on this series plus some
more patches to introduce a struct device in struct gpio_chip to track
the lifetime and eventually fix the issue demonstrated above.

While addressing this issue isn't a hard requirement for my final plan
to introduce /dev/pwmchip character devices, fixing it now before the
pwm core is complicated with the character device code should be easier
at least. (Also these character devices introduce another "user" of
pwm_chips and so another reason that these shouldn't be allocated using
devm_kzalloc.)

Best regards
Uwe

[1] first approximation:

	$ git grep -Ew 'pwm_(apply|get_state|is_enabled|set_period|get_period|[sg]et_duty_cycle|get_polarity|enable|disable)' v6.5-rc1 -- drivers/pwm/pwm-*.c | wc -l
	43

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

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

[Uwe Kleine-König]

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

Hello Saravana,

you were pointed out to me as the expert for device links. I found a
problem with these.

On Tue, Jul 25, 2023 at 11:10:04PM +0200, Uwe Kleine-König wrote:
> Today I managed to trigger the problem I intend to address with this
> series. My machine to test this on is an stm32mp157. To be able to
> trigger the problem reliably I applied the following patches on top of
> v6.5-rc1:
> 
>  - pwm: stm32: Don't modify HW state in .remove() callback
>    This is a cleanup that I already sent out.
>    https://lore.kernel.org/r/20230713155142.2454010-2-u.kleine-koenig@pengutronix.de
>    The purpose for reproducing the problem is to not trigger further
>    calls to the apply callback.
> 
>  - The following patch:
> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 687967d3265f..c7fc02b0fa3c 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -451,6 +451,10 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
>  	int ret;
>  
> +	dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> +	msleep(5000);
> +	dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> +
>  	enabled = pwm->state.enabled;
>  
>  	if (enabled && !state->enabled) {
> @@ -650,7 +654,11 @@ static void stm32_pwm_remove(struct platform_device *pdev)
>  {
>  	struct stm32_pwm *priv = platform_get_drvdata(pdev);
>  
> +	dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
>  	pwmchip_remove(&priv->chip);
> +	dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> +
> +	priv->regmap = NULL;
>  }
>  
>  static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> 
> The first hunk is only there to widen the race window. The second is to
> give some diagnostics and make stm32_pwm_apply() crash if it continues
> to run after the msleep. (Without it it didn't crash reproducibly, don't
> understand why. *shrug*)
> 
> The device tree contains a pwm-fan device making use of one of the PWMs.
> 
> Now I do the following:
> 
> 	echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> 
> Unbinding the fan device has two effects:
> 
>  - The device link between fan and pwm looses its property to unbind fan
>    when pwm gets unbound.
>    (Its .status changes from DL_STATE_ACTIVE to DL_STATE_AVAILABLE)
>  - It calls pwm_fan_cleanup() which triggers a call to
>    pwm_apply_state().
> 
> So when the pwm device gets unbound the first thread is sleeping in
> stm32_pwm_apply(). The driver calls pwmchip_remove() and sets
> priv->regmap to NULL. Then a few seconds later the first thread wakes up
> in stm32_pwm_apply() with the chip freed and priv->regmap = NULL. Bang!
> 
> This looks as follows:
> 
> root@crown:~# echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> [  187.182113] stm32-pwm 40007000.timer:pwm: stm32_pwm_apply:454
> [  188.164769] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:657
> [  188.184555] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:659
> root@crown:~# [  192.236423] platform 40007000.timer:pwm: stm32_pwm_apply:456
> [  192.240727] 8<--- cut here ---
> [  192.243759] Unable to handle kernel NULL pointer dereference at virtual address 0000001c when read
> ...
> 
> Even without the crash you can see that stm32_pwm_apply() is still
> running after pwmchip_remove() completed.
> 
> I'm unsure if the device link could be improved here to ensure that the
> fan is completely unbound even if it started unbinding already before
> the pwm device gets unbound. (And if it could, would this fit the device
> links purpose and so be a sensible improvement?)

While I think that there is something to be done in the pwm core that
this doesn't explode (i.e. do proper lifetime tracking such that a
pwm_chip doesn't disappear while still being used---and I'm working on
that) I expected that the device links between pwm consumer and provider
would prevent the above described oops, too. But somehow the fan already
going away (but still using the PWM) when the PWM is unbound, results in
the PWM disappearing before the fan is completely gone.

Is this expected, or a problem that can (and should?) be fixed?

If you need more context or a tester, don't hesitate to ask.

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 01/18] pwm: Provide devm_pwmchip_alloc() function

[Saravana Kannan]

On Tue, Oct 10, 2023 at 1:05 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Saravana,
>
> you were pointed out to me as the expert for device links. I found a
> problem with these.
>
> On Tue, Jul 25, 2023 at 11:10:04PM +0200, Uwe Kleine-König wrote:
> > Today I managed to trigger the problem I intend to address with this
> > series. My machine to test this on is an stm32mp157. To be able to
> > trigger the problem reliably I applied the following patches on top of
> > v6.5-rc1:
> >
> >  - pwm: stm32: Don't modify HW state in .remove() callback
> >    This is a cleanup that I already sent out.
> >    https://lore.kernel.org/r/20230713155142.2454010-2-u.kleine-koenig@pengutronix.de
> >    The purpose for reproducing the problem is to not trigger further
> >    calls to the apply callback.
> >
> >  - The following patch:
> >
> > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > index 687967d3265f..c7fc02b0fa3c 100644
> > --- a/drivers/pwm/pwm-stm32.c
> > +++ b/drivers/pwm/pwm-stm32.c
> > @@ -451,6 +451,10 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >       struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> >       int ret;
> >
> > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > +     msleep(5000);
> > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > +
> >       enabled = pwm->state.enabled;
> >
> >       if (enabled && !state->enabled) {
> > @@ -650,7 +654,11 @@ static void stm32_pwm_remove(struct platform_device *pdev)
> >  {
> >       struct stm32_pwm *priv = platform_get_drvdata(pdev);
> >
> > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> >       pwmchip_remove(&priv->chip);
> > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > +
> > +     priv->regmap = NULL;
> >  }
> >
> >  static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> >
> > The first hunk is only there to widen the race window. The second is to
> > give some diagnostics and make stm32_pwm_apply() crash if it continues
> > to run after the msleep. (Without it it didn't crash reproducibly, don't
> > understand why. *shrug*)
> >
> > The device tree contains a pwm-fan device making use of one of the PWMs.
> >
> > Now I do the following:
> >
> >       echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> >
> > Unbinding the fan device has two effects:
> >
> >  - The device link between fan and pwm looses its property to unbind fan
> >    when pwm gets unbound.
> >    (Its .status changes from DL_STATE_ACTIVE to DL_STATE_AVAILABLE)
> >  - It calls pwm_fan_cleanup() which triggers a call to
> >    pwm_apply_state().
> >
> > So when the pwm device gets unbound the first thread is sleeping in
> > stm32_pwm_apply(). The driver calls pwmchip_remove() and sets
> > priv->regmap to NULL. Then a few seconds later the first thread wakes up
> > in stm32_pwm_apply() with the chip freed and priv->regmap = NULL. Bang!
> >
> > This looks as follows:
> >
> > root@crown:~# echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > [  187.182113] stm32-pwm 40007000.timer:pwm: stm32_pwm_apply:454
> > [  188.164769] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:657
> > [  188.184555] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:659
> > root@crown:~# [  192.236423] platform 40007000.timer:pwm: stm32_pwm_apply:456
> > [  192.240727] 8<--- cut here ---
> > [  192.243759] Unable to handle kernel NULL pointer dereference at virtual address 0000001c when read
> > ...
> >
> > Even without the crash you can see that stm32_pwm_apply() is still
> > running after pwmchip_remove() completed.
> >
> > I'm unsure if the device link could be improved here to ensure that the
> > fan is completely unbound even if it started unbinding already before
> > the pwm device gets unbound. (And if it could, would this fit the device
> > links purpose and so be a sensible improvement?)
>
> While I think that there is something to be done in the pwm core that
> this doesn't explode (i.e. do proper lifetime tracking such that a
> pwm_chip doesn't disappear while still being used---and I'm working on
> that) I expected that the device links between pwm consumer and provider
> would prevent the above described oops, too. But somehow the fan already
> going away (but still using the PWM) when the PWM is unbound, results in
> the PWM disappearing before the fan is completely gone.
>
> Is this expected, or a problem that can (and should?) be fixed?

I didn't read your full series, but I read this email. With what's in
this email, the problem seems to be in the driver or the pwm
framework. The pwm driver/framework can't tell the driver core that
you successfully unbound (returning from .remove()) before you have
finish all your ongoing transactions with the device. If your
"apply()" is still running, you need to make sure it's complete before
.remove() does any resource releasing/clean up.

Also, how is the consumer driver's .remove() succeeding if it has an
ongoing pwm call()? This all sounds like insufficient locking and
critical region protection in both the consumer and supplier.

Device links can't do anything here because you are giving it wrong
info -- that the unbind was successful before it actually is.

Device links will and can make sure that the consumer is unbound
successfully before the unbind is called on the supplier. And it looks
like that's still true here.

-Saravana

>
> If you need more context or a tester, don't hesitate to ask.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[Uwe Kleine-König]

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

On Fri, Oct 13, 2023 at 02:42:20PM -0700, Saravana Kannan wrote:
> On Tue, Oct 10, 2023 at 1:05 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello Saravana,
> >
> > you were pointed out to me as the expert for device links. I found a
> > problem with these.
> >
> > On Tue, Jul 25, 2023 at 11:10:04PM +0200, Uwe Kleine-König wrote:
> > > Today I managed to trigger the problem I intend to address with this
> > > series. My machine to test this on is an stm32mp157. To be able to
> > > trigger the problem reliably I applied the following patches on top of
> > > v6.5-rc1:
> > >
> > >  - pwm: stm32: Don't modify HW state in .remove() callback
> > >    This is a cleanup that I already sent out.
> > >    https://lore.kernel.org/r/20230713155142.2454010-2-u.kleine-koenig@pengutronix.de
> > >    The purpose for reproducing the problem is to not trigger further
> > >    calls to the apply callback.
> > >
> > >  - The following patch:
> > >
> > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > > index 687967d3265f..c7fc02b0fa3c 100644
> > > --- a/drivers/pwm/pwm-stm32.c
> > > +++ b/drivers/pwm/pwm-stm32.c
> > > @@ -451,6 +451,10 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >       struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > >       int ret;
> > >
> > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > +     msleep(5000);
> > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > +
> > >       enabled = pwm->state.enabled;
> > >
> > >       if (enabled && !state->enabled) {
> > > @@ -650,7 +654,11 @@ static void stm32_pwm_remove(struct platform_device *pdev)
> > >  {
> > >       struct stm32_pwm *priv = platform_get_drvdata(pdev);
> > >
> > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > >       pwmchip_remove(&priv->chip);
> > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > +
> > > +     priv->regmap = NULL;
> > >  }
> > >
> > >  static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> > >
> > > The first hunk is only there to widen the race window. The second is to
> > > give some diagnostics and make stm32_pwm_apply() crash if it continues
> > > to run after the msleep. (Without it it didn't crash reproducibly, don't
> > > understand why. *shrug*)
> > >
> > > The device tree contains a pwm-fan device making use of one of the PWMs.
> > >
> > > Now I do the following:
> > >
> > >       echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > >
> > > Unbinding the fan device has two effects:
> > >
> > >  - The device link between fan and pwm looses its property to unbind fan
> > >    when pwm gets unbound.
> > >    (Its .status changes from DL_STATE_ACTIVE to DL_STATE_AVAILABLE)
> > >  - It calls pwm_fan_cleanup() which triggers a call to
> > >    pwm_apply_state().
> > >
> > > So when the pwm device gets unbound the first thread is sleeping in
> > > stm32_pwm_apply(). The driver calls pwmchip_remove() and sets
> > > priv->regmap to NULL. Then a few seconds later the first thread wakes up
> > > in stm32_pwm_apply() with the chip freed and priv->regmap = NULL. Bang!
> > >
> > > This looks as follows:
> > >
> > > root@crown:~# echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > [  187.182113] stm32-pwm 40007000.timer:pwm: stm32_pwm_apply:454
> > > [  188.164769] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:657
> > > [  188.184555] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:659
> > > root@crown:~# [  192.236423] platform 40007000.timer:pwm: stm32_pwm_apply:456
> > > [  192.240727] 8<--- cut here ---
> > > [  192.243759] Unable to handle kernel NULL pointer dereference at virtual address 0000001c when read
> > > ...
> > >
> > > Even without the crash you can see that stm32_pwm_apply() is still
> > > running after pwmchip_remove() completed.
> > >
> > > I'm unsure if the device link could be improved here to ensure that the
> > > fan is completely unbound even if it started unbinding already before
> > > the pwm device gets unbound. (And if it could, would this fit the device
> > > links purpose and so be a sensible improvement?)
> >
> > While I think that there is something to be done in the pwm core that
> > this doesn't explode (i.e. do proper lifetime tracking such that a
> > pwm_chip doesn't disappear while still being used---and I'm working on
> > that) I expected that the device links between pwm consumer and provider
> > would prevent the above described oops, too. But somehow the fan already
> > going away (but still using the PWM) when the PWM is unbound, results in
> > the PWM disappearing before the fan is completely gone.
> >
> > Is this expected, or a problem that can (and should?) be fixed?
> 
> I didn't read your full series, but I read this email. With what's in
> this email, the problem seems to be in the driver or the pwm
> framework. The pwm driver/framework can't tell the driver core that
> you successfully unbound (returning from .remove()) before you have
> finish all your ongoing transactions with the device. If your
> "apply()" is still running, you need to make sure it's complete before
> .remove() does any resource releasing/clean up.
> 
> Also, how is the consumer driver's .remove() succeeding if it has an
> ongoing pwm call()?

The thing that works fine and as expected is:

 - trigger unbind of PWM device via sysfs

Because there is a device link PWM provider -> pwm consumer (fan), the
fan is removed and once its gone (and not earlier), the PWM gets unbound.

The failing sequence is:

 - trigger unbind of fan device in userspace thread A via sysfs. The
   fan's remove callback blocks for 5s in pwm_apply_state() and so
   .remove() doesn't complete yet.

 - a second later: trigger unbind of PWM device via sysfs in thread B.
   As before I'd expect that the device link results in waiting for the
   fan to be removed completely, but the PWM is removed immediately.

 - pwm_apply_state's sleep completes (in thread B) and operates on freed
   resources => bang!

> This all sounds like insufficient locking and
> critical region protection in both the consumer and supplier.

My (and I think also Thierry's) expectation was, that the device link
provides the needed synchronisation. But it doesn't as it doesn't block
the PWM provider going away until the fan is completely gone.

> Device links can't do anything here because you are giving it wrong
> info -- that the unbind was successful before it actually is.

The fan's unbind is ongoing, but not complete yet and I'd expect that
the device link blocks unbinding the PWM until the fan is completely
gone. So I think there is no wrong information.

> Device links will and can make sure that the consumer is unbound
> successfully before the unbind is called on the supplier. And it looks
> like that's still true here.

I hope you understood the situation better now and see the problem we
have.

The problem is fixable in the pwm framework (and I'm working on that),
but I think there is also something to improve around devicelink
handling.

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 01/18] pwm: Provide devm_pwmchip_alloc() function

[Saravana Kannan]

On Sat, Oct 14, 2023 at 9:17 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Fri, Oct 13, 2023 at 02:42:20PM -0700, Saravana Kannan wrote:
> > On Tue, Oct 10, 2023 at 1:05 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > Hello Saravana,
> > >
> > > you were pointed out to me as the expert for device links. I found a
> > > problem with these.
> > >
> > > On Tue, Jul 25, 2023 at 11:10:04PM +0200, Uwe Kleine-König wrote:
> > > > Today I managed to trigger the problem I intend to address with this
> > > > series. My machine to test this on is an stm32mp157. To be able to
> > > > trigger the problem reliably I applied the following patches on top of
> > > > v6.5-rc1:
> > > >
> > > >  - pwm: stm32: Don't modify HW state in .remove() callback
> > > >    This is a cleanup that I already sent out.
> > > >    https://lore.kernel.org/r/20230713155142.2454010-2-u.kleine-koenig@pengutronix.de
> > > >    The purpose for reproducing the problem is to not trigger further
> > > >    calls to the apply callback.
> > > >
> > > >  - The following patch:
> > > >
> > > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > > > index 687967d3265f..c7fc02b0fa3c 100644
> > > > --- a/drivers/pwm/pwm-stm32.c
> > > > +++ b/drivers/pwm/pwm-stm32.c
> > > > @@ -451,6 +451,10 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >       struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > > >       int ret;
> > > >
> > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > +     msleep(5000);
> > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > +
> > > >       enabled = pwm->state.enabled;
> > > >
> > > >       if (enabled && !state->enabled) {
> > > > @@ -650,7 +654,11 @@ static void stm32_pwm_remove(struct platform_device *pdev)
> > > >  {
> > > >       struct stm32_pwm *priv = platform_get_drvdata(pdev);
> > > >
> > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > >       pwmchip_remove(&priv->chip);
> > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > +
> > > > +     priv->regmap = NULL;
> > > >  }
> > > >
> > > >  static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> > > >
> > > > The first hunk is only there to widen the race window. The second is to
> > > > give some diagnostics and make stm32_pwm_apply() crash if it continues
> > > > to run after the msleep. (Without it it didn't crash reproducibly, don't
> > > > understand why. *shrug*)
> > > >
> > > > The device tree contains a pwm-fan device making use of one of the PWMs.
> > > >
> > > > Now I do the following:
> > > >
> > > >       echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > >
> > > > Unbinding the fan device has two effects:
> > > >
> > > >  - The device link between fan and pwm looses its property to unbind fan
> > > >    when pwm gets unbound.
> > > >    (Its .status changes from DL_STATE_ACTIVE to DL_STATE_AVAILABLE)
> > > >  - It calls pwm_fan_cleanup() which triggers a call to
> > > >    pwm_apply_state().
> > > >
> > > > So when the pwm device gets unbound the first thread is sleeping in
> > > > stm32_pwm_apply(). The driver calls pwmchip_remove() and sets
> > > > priv->regmap to NULL. Then a few seconds later the first thread wakes up
> > > > in stm32_pwm_apply() with the chip freed and priv->regmap = NULL. Bang!
> > > >
> > > > This looks as follows:
> > > >
> > > > root@crown:~# echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > [  187.182113] stm32-pwm 40007000.timer:pwm: stm32_pwm_apply:454
> > > > [  188.164769] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:657
> > > > [  188.184555] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:659
> > > > root@crown:~# [  192.236423] platform 40007000.timer:pwm: stm32_pwm_apply:456
> > > > [  192.240727] 8<--- cut here ---
> > > > [  192.243759] Unable to handle kernel NULL pointer dereference at virtual address 0000001c when read
> > > > ...
> > > >
> > > > Even without the crash you can see that stm32_pwm_apply() is still
> > > > running after pwmchip_remove() completed.
> > > >
> > > > I'm unsure if the device link could be improved here to ensure that the
> > > > fan is completely unbound even if it started unbinding already before
> > > > the pwm device gets unbound. (And if it could, would this fit the device
> > > > links purpose and so be a sensible improvement?)
> > >
> > > While I think that there is something to be done in the pwm core that
> > > this doesn't explode (i.e. do proper lifetime tracking such that a
> > > pwm_chip doesn't disappear while still being used---and I'm working on
> > > that) I expected that the device links between pwm consumer and provider
> > > would prevent the above described oops, too. But somehow the fan already
> > > going away (but still using the PWM) when the PWM is unbound, results in
> > > the PWM disappearing before the fan is completely gone.
> > >
> > > Is this expected, or a problem that can (and should?) be fixed?
> >
> > I didn't read your full series, but I read this email. With what's in
> > this email, the problem seems to be in the driver or the pwm
> > framework. The pwm driver/framework can't tell the driver core that
> > you successfully unbound (returning from .remove()) before you have
> > finish all your ongoing transactions with the device. If your
> > "apply()" is still running, you need to make sure it's complete before
> > .remove() does any resource releasing/clean up.
> >
> > Also, how is the consumer driver's .remove() succeeding if it has an
> > ongoing pwm call()?
>
> The thing that works fine and as expected is:
>
>  - trigger unbind of PWM device via sysfs
>
> Because there is a device link PWM provider -> pwm consumer (fan), the
> fan is removed and once its gone (and not earlier), the PWM gets unbound.
>
> The failing sequence is:
>
>  - trigger unbind of fan device in userspace thread A via sysfs. The
>    fan's remove callback blocks for 5s in pwm_apply_state() and so
>    .remove() doesn't complete yet.
>
>  - a second later: trigger unbind of PWM device via sysfs in thread B.
>    As before I'd expect that the device link results in waiting for the
>    fan to be removed completely, but the PWM is removed immediately.
>
>  - pwm_apply_state's sleep completes (in thread B) and operates on freed
>    resources => bang!
>
> > This all sounds like insufficient locking and
> > critical region protection in both the consumer and supplier.
>
> My (and I think also Thierry's) expectation was, that the device link
> provides the needed synchronisation. But it doesn't as it doesn't block
> the PWM provider going away until the fan is completely gone.
>
> > Device links can't do anything here because you are giving it wrong
> > info -- that the unbind was successful before it actually is.
>
> The fan's unbind is ongoing, but not complete yet and I'd expect that
> the device link blocks unbinding the PWM until the fan is completely
> gone. So I think there is no wrong information.
>
> > Device links will and can make sure that the consumer is unbound
> > successfully before the unbind is called on the supplier. And it looks
> > like that's still true here.
>
> I hope you understood the situation better now and see the problem we
> have.
>
> The problem is fixable in the pwm framework (and I'm working on that),
> but I think there is also something to improve around devicelink
> handling.

Thanks for a better explanation of the issue. I agree, this seems like
something device links should be able to take care of.

I'll take a look into this.

-Saravana

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

[Saravana Kannan]

On Tue, Oct 17, 2023 at 4:35 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Sat, Oct 14, 2023 at 9:17 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Fri, Oct 13, 2023 at 02:42:20PM -0700, Saravana Kannan wrote:
> > > On Tue, Oct 10, 2023 at 1:05 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > Hello Saravana,
> > > >
> > > > you were pointed out to me as the expert for device links. I found a
> > > > problem with these.
> > > >
> > > > On Tue, Jul 25, 2023 at 11:10:04PM +0200, Uwe Kleine-König wrote:
> > > > > Today I managed to trigger the problem I intend to address with this
> > > > > series. My machine to test this on is an stm32mp157. To be able to
> > > > > trigger the problem reliably I applied the following patches on top of
> > > > > v6.5-rc1:
> > > > >
> > > > >  - pwm: stm32: Don't modify HW state in .remove() callback
> > > > >    This is a cleanup that I already sent out.
> > > > >    https://lore.kernel.org/r/20230713155142.2454010-2-u.kleine-koenig@pengutronix.de
> > > > >    The purpose for reproducing the problem is to not trigger further
> > > > >    calls to the apply callback.
> > > > >
> > > > >  - The following patch:
> > > > >
> > > > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > > > > index 687967d3265f..c7fc02b0fa3c 100644
> > > > > --- a/drivers/pwm/pwm-stm32.c
> > > > > +++ b/drivers/pwm/pwm-stm32.c
> > > > > @@ -451,6 +451,10 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > >       struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > > > >       int ret;
> > > > >
> > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > +     msleep(5000);
> > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > +
> > > > >       enabled = pwm->state.enabled;
> > > > >
> > > > >       if (enabled && !state->enabled) {
> > > > > @@ -650,7 +654,11 @@ static void stm32_pwm_remove(struct platform_device *pdev)
> > > > >  {
> > > > >       struct stm32_pwm *priv = platform_get_drvdata(pdev);
> > > > >
> > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > >       pwmchip_remove(&priv->chip);
> > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > > +
> > > > > +     priv->regmap = NULL;
> > > > >  }
> > > > >
> > > > >  static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> > > > >
> > > > > The first hunk is only there to widen the race window. The second is to
> > > > > give some diagnostics and make stm32_pwm_apply() crash if it continues
> > > > > to run after the msleep. (Without it it didn't crash reproducibly, don't
> > > > > understand why. *shrug*)
> > > > >
> > > > > The device tree contains a pwm-fan device making use of one of the PWMs.
> > > > >
> > > > > Now I do the following:
> > > > >
> > > > >       echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > >
> > > > > Unbinding the fan device has two effects:
> > > > >
> > > > >  - The device link between fan and pwm looses its property to unbind fan
> > > > >    when pwm gets unbound.
> > > > >    (Its .status changes from DL_STATE_ACTIVE to DL_STATE_AVAILABLE)
> > > > >  - It calls pwm_fan_cleanup() which triggers a call to
> > > > >    pwm_apply_state().
> > > > >
> > > > > So when the pwm device gets unbound the first thread is sleeping in
> > > > > stm32_pwm_apply(). The driver calls pwmchip_remove() and sets
> > > > > priv->regmap to NULL. Then a few seconds later the first thread wakes up
> > > > > in stm32_pwm_apply() with the chip freed and priv->regmap = NULL. Bang!
> > > > >
> > > > > This looks as follows:
> > > > >
> > > > > root@crown:~# echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > > [  187.182113] stm32-pwm 40007000.timer:pwm: stm32_pwm_apply:454
> > > > > [  188.164769] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:657
> > > > > [  188.184555] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:659
> > > > > root@crown:~# [  192.236423] platform 40007000.timer:pwm: stm32_pwm_apply:456
> > > > > [  192.240727] 8<--- cut here ---
> > > > > [  192.243759] Unable to handle kernel NULL pointer dereference at virtual address 0000001c when read
> > > > > ...
> > > > >
> > > > > Even without the crash you can see that stm32_pwm_apply() is still
> > > > > running after pwmchip_remove() completed.
> > > > >
> > > > > I'm unsure if the device link could be improved here to ensure that the
> > > > > fan is completely unbound even if it started unbinding already before
> > > > > the pwm device gets unbound. (And if it could, would this fit the device
> > > > > links purpose and so be a sensible improvement?)
> > > >
> > > > While I think that there is something to be done in the pwm core that
> > > > this doesn't explode (i.e. do proper lifetime tracking such that a
> > > > pwm_chip doesn't disappear while still being used---and I'm working on
> > > > that) I expected that the device links between pwm consumer and provider
> > > > would prevent the above described oops, too. But somehow the fan already
> > > > going away (but still using the PWM) when the PWM is unbound, results in
> > > > the PWM disappearing before the fan is completely gone.
> > > >
> > > > Is this expected, or a problem that can (and should?) be fixed?
> > >
> > > I didn't read your full series, but I read this email. With what's in
> > > this email, the problem seems to be in the driver or the pwm
> > > framework. The pwm driver/framework can't tell the driver core that
> > > you successfully unbound (returning from .remove()) before you have
> > > finish all your ongoing transactions with the device. If your
> > > "apply()" is still running, you need to make sure it's complete before
> > > .remove() does any resource releasing/clean up.
> > >
> > > Also, how is the consumer driver's .remove() succeeding if it has an
> > > ongoing pwm call()?
> >
> > The thing that works fine and as expected is:
> >
> >  - trigger unbind of PWM device via sysfs
> >
> > Because there is a device link PWM provider -> pwm consumer (fan), the
> > fan is removed and once its gone (and not earlier), the PWM gets unbound.
> >
> > The failing sequence is:
> >
> >  - trigger unbind of fan device in userspace thread A via sysfs. The
> >    fan's remove callback blocks for 5s in pwm_apply_state() and so
> >    .remove() doesn't complete yet.
> >
> >  - a second later: trigger unbind of PWM device via sysfs in thread B.
> >    As before I'd expect that the device link results in waiting for the
> >    fan to be removed completely, but the PWM is removed immediately.
> >
> >  - pwm_apply_state's sleep completes (in thread B) and operates on freed
> >    resources => bang!
> >
> > > This all sounds like insufficient locking and
> > > critical region protection in both the consumer and supplier.
> >
> > My (and I think also Thierry's) expectation was, that the device link
> > provides the needed synchronisation. But it doesn't as it doesn't block
> > the PWM provider going away until the fan is completely gone.
> >
> > > Device links can't do anything here because you are giving it wrong
> > > info -- that the unbind was successful before it actually is.
> >
> > The fan's unbind is ongoing, but not complete yet and I'd expect that
> > the device link blocks unbinding the PWM until the fan is completely
> > gone. So I think there is no wrong information.
> >
> > > Device links will and can make sure that the consumer is unbound
> > > successfully before the unbind is called on the supplier. And it looks
> > > like that's still true here.
> >
> > I hope you understood the situation better now and see the problem we
> > have.
> >
> > The problem is fixable in the pwm framework (and I'm working on that),
> > but I think there is also something to improve around devicelink
> > handling.
>
> Thanks for a better explanation of the issue. I agree, this seems like
> something device links should be able to take care of.
>
> I'll take a look into this.

Took me a while to debug this because I couldn't find the .remove()
function and I was very confused about what's going on.
I'm guessing you started hitting this issue only after moving to the
devm_ variant of the pwm APIs.

I think I have a fix.
https://lore.kernel.org/lkml/20231018013851.3303928-1-saravanak@google.com/T/#u

I didn't even compile test it, but it's a trivial change. Can you
please test it out and give your "tested-by" if it fixes your issue?

Thanks,
Saravana

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

[Uwe Kleine-König]

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

On Tue, Oct 17, 2023 at 06:42:40PM -0700, Saravana Kannan wrote:
> On Tue, Oct 17, 2023 at 4:35 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Sat, Oct 14, 2023 at 9:17 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > On Fri, Oct 13, 2023 at 02:42:20PM -0700, Saravana Kannan wrote:
> > > > On Tue, Oct 10, 2023 at 1:05 AM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > >
> > > > > Hello Saravana,
> > > > >
> > > > > you were pointed out to me as the expert for device links. I found a
> > > > > problem with these.
> > > > >
> > > > > On Tue, Jul 25, 2023 at 11:10:04PM +0200, Uwe Kleine-König wrote:
> > > > > > Today I managed to trigger the problem I intend to address with this
> > > > > > series. My machine to test this on is an stm32mp157. To be able to
> > > > > > trigger the problem reliably I applied the following patches on top of
> > > > > > v6.5-rc1:
> > > > > >
> > > > > >  - pwm: stm32: Don't modify HW state in .remove() callback
> > > > > >    This is a cleanup that I already sent out.
> > > > > >    https://lore.kernel.org/r/20230713155142.2454010-2-u.kleine-koenig@pengutronix.de
> > > > > >    The purpose for reproducing the problem is to not trigger further
> > > > > >    calls to the apply callback.
> > > > > >
> > > > > >  - The following patch:
> > > > > >
> > > > > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > > > > > index 687967d3265f..c7fc02b0fa3c 100644
> > > > > > --- a/drivers/pwm/pwm-stm32.c
> > > > > > +++ b/drivers/pwm/pwm-stm32.c
> > > > > > @@ -451,6 +451,10 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > >       struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > > > > >       int ret;
> > > > > >
> > > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > +     msleep(5000);
> > > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > +
> > > > > >       enabled = pwm->state.enabled;
> > > > > >
> > > > > >       if (enabled && !state->enabled) {
> > > > > > @@ -650,7 +654,11 @@ static void stm32_pwm_remove(struct platform_device *pdev)
> > > > > >  {
> > > > > >       struct stm32_pwm *priv = platform_get_drvdata(pdev);
> > > > > >
> > > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > > >       pwmchip_remove(&priv->chip);
> > > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > +
> > > > > > +     priv->regmap = NULL;
> > > > > >  }
> > > > > >
> > > > > >  static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> > > > > >
> > > > > > The first hunk is only there to widen the race window. The second is to
> > > > > > give some diagnostics and make stm32_pwm_apply() crash if it continues
> > > > > > to run after the msleep. (Without it it didn't crash reproducibly, don't
> > > > > > understand why. *shrug*)
> > > > > >
> > > > > > The device tree contains a pwm-fan device making use of one of the PWMs.
> > > > > >
> > > > > > Now I do the following:
> > > > > >
> > > > > >       echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > > >
> > > > > > Unbinding the fan device has two effects:
> > > > > >
> > > > > >  - The device link between fan and pwm looses its property to unbind fan
> > > > > >    when pwm gets unbound.
> > > > > >    (Its .status changes from DL_STATE_ACTIVE to DL_STATE_AVAILABLE)
> > > > > >  - It calls pwm_fan_cleanup() which triggers a call to
> > > > > >    pwm_apply_state().
> > > > > >
> > > > > > So when the pwm device gets unbound the first thread is sleeping in
> > > > > > stm32_pwm_apply(). The driver calls pwmchip_remove() and sets
> > > > > > priv->regmap to NULL. Then a few seconds later the first thread wakes up
> > > > > > in stm32_pwm_apply() with the chip freed and priv->regmap = NULL. Bang!
> > > > > >
> > > > > > This looks as follows:
> > > > > >
> > > > > > root@crown:~# echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > > > [  187.182113] stm32-pwm 40007000.timer:pwm: stm32_pwm_apply:454
> > > > > > [  188.164769] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:657
> > > > > > [  188.184555] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:659
> > > > > > root@crown:~# [  192.236423] platform 40007000.timer:pwm: stm32_pwm_apply:456
> > > > > > [  192.240727] 8<--- cut here ---
> > > > > > [  192.243759] Unable to handle kernel NULL pointer dereference at virtual address 0000001c when read
> > > > > > ...
> > > > > >
> > > > > > Even without the crash you can see that stm32_pwm_apply() is still
> > > > > > running after pwmchip_remove() completed.
> > > > > >
> > > > > > I'm unsure if the device link could be improved here to ensure that the
> > > > > > fan is completely unbound even if it started unbinding already before
> > > > > > the pwm device gets unbound. (And if it could, would this fit the device
> > > > > > links purpose and so be a sensible improvement?)
> > > > >
> > > > > While I think that there is something to be done in the pwm core that
> > > > > this doesn't explode (i.e. do proper lifetime tracking such that a
> > > > > pwm_chip doesn't disappear while still being used---and I'm working on
> > > > > that) I expected that the device links between pwm consumer and provider
> > > > > would prevent the above described oops, too. But somehow the fan already
> > > > > going away (but still using the PWM) when the PWM is unbound, results in
> > > > > the PWM disappearing before the fan is completely gone.
> > > > >
> > > > > Is this expected, or a problem that can (and should?) be fixed?
> > > >
> > > > I didn't read your full series, but I read this email. With what's in
> > > > this email, the problem seems to be in the driver or the pwm
> > > > framework. The pwm driver/framework can't tell the driver core that
> > > > you successfully unbound (returning from .remove()) before you have
> > > > finish all your ongoing transactions with the device. If your
> > > > "apply()" is still running, you need to make sure it's complete before
> > > > .remove() does any resource releasing/clean up.
> > > >
> > > > Also, how is the consumer driver's .remove() succeeding if it has an
> > > > ongoing pwm call()?
> > >
> > > The thing that works fine and as expected is:
> > >
> > >  - trigger unbind of PWM device via sysfs
> > >
> > > Because there is a device link PWM provider -> pwm consumer (fan), the
> > > fan is removed and once its gone (and not earlier), the PWM gets unbound.
> > >
> > > The failing sequence is:
> > >
> > >  - trigger unbind of fan device in userspace thread A via sysfs. The
> > >    fan's remove callback blocks for 5s in pwm_apply_state() and so
> > >    .remove() doesn't complete yet.
> > >
> > >  - a second later: trigger unbind of PWM device via sysfs in thread B.
> > >    As before I'd expect that the device link results in waiting for the
> > >    fan to be removed completely, but the PWM is removed immediately.
> > >
> > >  - pwm_apply_state's sleep completes (in thread B) and operates on freed
> > >    resources => bang!
> > >
> > > > This all sounds like insufficient locking and
> > > > critical region protection in both the consumer and supplier.
> > >
> > > My (and I think also Thierry's) expectation was, that the device link
> > > provides the needed synchronisation. But it doesn't as it doesn't block
> > > the PWM provider going away until the fan is completely gone.
> > >
> > > > Device links can't do anything here because you are giving it wrong
> > > > info -- that the unbind was successful before it actually is.
> > >
> > > The fan's unbind is ongoing, but not complete yet and I'd expect that
> > > the device link blocks unbinding the PWM until the fan is completely
> > > gone. So I think there is no wrong information.
> > >
> > > > Device links will and can make sure that the consumer is unbound
> > > > successfully before the unbind is called on the supplier. And it looks
> > > > like that's still true here.
> > >
> > > I hope you understood the situation better now and see the problem we
> > > have.
> > >
> > > The problem is fixable in the pwm framework (and I'm working on that),
> > > but I think there is also something to improve around devicelink
> > > handling.
> >
> > Thanks for a better explanation of the issue. I agree, this seems like
> > something device links should be able to take care of.
> >
> > I'll take a look into this.
> 
> Took me a while to debug this because I couldn't find the .remove()
> function and I was very confused about what's going on.
> I'm guessing you started hitting this issue only after moving to the
> devm_ variant of the pwm APIs.

Ah I see. That problem wouldn't happen if the fan called a pwm API
function in its remove callback but that happens in a devm cleanup call
(registered by devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx) in
pwm_fan_probe()). I first thought you talked about
8c89fd866ad221af037ef0ec3d60b83d0b859c65.

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 01/18] pwm: Provide devm_pwmchip_alloc() function

[Saravana Kannan]

On Wed, Oct 18, 2023 at 4:17 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Tue, Oct 17, 2023 at 06:42:40PM -0700, Saravana Kannan wrote:
> > On Tue, Oct 17, 2023 at 4:35 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Sat, Oct 14, 2023 at 9:17 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > On Fri, Oct 13, 2023 at 02:42:20PM -0700, Saravana Kannan wrote:
> > > > > On Tue, Oct 10, 2023 at 1:05 AM Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > >
> > > > > > Hello Saravana,
> > > > > >
> > > > > > you were pointed out to me as the expert for device links. I found a
> > > > > > problem with these.
> > > > > >
> > > > > > On Tue, Jul 25, 2023 at 11:10:04PM +0200, Uwe Kleine-König wrote:
> > > > > > > Today I managed to trigger the problem I intend to address with this
> > > > > > > series. My machine to test this on is an stm32mp157. To be able to
> > > > > > > trigger the problem reliably I applied the following patches on top of
> > > > > > > v6.5-rc1:
> > > > > > >
> > > > > > >  - pwm: stm32: Don't modify HW state in .remove() callback
> > > > > > >    This is a cleanup that I already sent out.
> > > > > > >    https://lore.kernel.org/r/20230713155142.2454010-2-u.kleine-koenig@pengutronix.de
> > > > > > >    The purpose for reproducing the problem is to not trigger further
> > > > > > >    calls to the apply callback.
> > > > > > >
> > > > > > >  - The following patch:
> > > > > > >
> > > > > > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > > > > > > index 687967d3265f..c7fc02b0fa3c 100644
> > > > > > > --- a/drivers/pwm/pwm-stm32.c
> > > > > > > +++ b/drivers/pwm/pwm-stm32.c
> > > > > > > @@ -451,6 +451,10 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > > >       struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > > > > > >       int ret;
> > > > > > >
> > > > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > +     msleep(5000);
> > > > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > +
> > > > > > >       enabled = pwm->state.enabled;
> > > > > > >
> > > > > > >       if (enabled && !state->enabled) {
> > > > > > > @@ -650,7 +654,11 @@ static void stm32_pwm_remove(struct platform_device *pdev)
> > > > > > >  {
> > > > > > >       struct stm32_pwm *priv = platform_get_drvdata(pdev);
> > > > > > >
> > > > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > >       pwmchip_remove(&priv->chip);
> > > > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > +
> > > > > > > +     priv->regmap = NULL;
> > > > > > >  }
> > > > > > >
> > > > > > >  static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> > > > > > >
> > > > > > > The first hunk is only there to widen the race window. The second is to
> > > > > > > give some diagnostics and make stm32_pwm_apply() crash if it continues
> > > > > > > to run after the msleep. (Without it it didn't crash reproducibly, don't
> > > > > > > understand why. *shrug*)
> > > > > > >
> > > > > > > The device tree contains a pwm-fan device making use of one of the PWMs.
> > > > > > >
> > > > > > > Now I do the following:
> > > > > > >
> > > > > > >       echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > > > >
> > > > > > > Unbinding the fan device has two effects:
> > > > > > >
> > > > > > >  - The device link between fan and pwm looses its property to unbind fan
> > > > > > >    when pwm gets unbound.
> > > > > > >    (Its .status changes from DL_STATE_ACTIVE to DL_STATE_AVAILABLE)
> > > > > > >  - It calls pwm_fan_cleanup() which triggers a call to
> > > > > > >    pwm_apply_state().
> > > > > > >
> > > > > > > So when the pwm device gets unbound the first thread is sleeping in
> > > > > > > stm32_pwm_apply(). The driver calls pwmchip_remove() and sets
> > > > > > > priv->regmap to NULL. Then a few seconds later the first thread wakes up
> > > > > > > in stm32_pwm_apply() with the chip freed and priv->regmap = NULL. Bang!
> > > > > > >
> > > > > > > This looks as follows:
> > > > > > >
> > > > > > > root@crown:~# echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > > > > [  187.182113] stm32-pwm 40007000.timer:pwm: stm32_pwm_apply:454
> > > > > > > [  188.164769] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:657
> > > > > > > [  188.184555] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:659
> > > > > > > root@crown:~# [  192.236423] platform 40007000.timer:pwm: stm32_pwm_apply:456
> > > > > > > [  192.240727] 8<--- cut here ---
> > > > > > > [  192.243759] Unable to handle kernel NULL pointer dereference at virtual address 0000001c when read
> > > > > > > ...
> > > > > > >
> > > > > > > Even without the crash you can see that stm32_pwm_apply() is still
> > > > > > > running after pwmchip_remove() completed.
> > > > > > >
> > > > > > > I'm unsure if the device link could be improved here to ensure that the
> > > > > > > fan is completely unbound even if it started unbinding already before
> > > > > > > the pwm device gets unbound. (And if it could, would this fit the device
> > > > > > > links purpose and so be a sensible improvement?)
> > > > > >
> > > > > > While I think that there is something to be done in the pwm core that
> > > > > > this doesn't explode (i.e. do proper lifetime tracking such that a
> > > > > > pwm_chip doesn't disappear while still being used---and I'm working on
> > > > > > that) I expected that the device links between pwm consumer and provider
> > > > > > would prevent the above described oops, too. But somehow the fan already
> > > > > > going away (but still using the PWM) when the PWM is unbound, results in
> > > > > > the PWM disappearing before the fan is completely gone.
> > > > > >
> > > > > > Is this expected, or a problem that can (and should?) be fixed?
> > > > >
> > > > > I didn't read your full series, but I read this email. With what's in
> > > > > this email, the problem seems to be in the driver or the pwm
> > > > > framework. The pwm driver/framework can't tell the driver core that
> > > > > you successfully unbound (returning from .remove()) before you have
> > > > > finish all your ongoing transactions with the device. If your
> > > > > "apply()" is still running, you need to make sure it's complete before
> > > > > .remove() does any resource releasing/clean up.
> > > > >
> > > > > Also, how is the consumer driver's .remove() succeeding if it has an
> > > > > ongoing pwm call()?
> > > >
> > > > The thing that works fine and as expected is:
> > > >
> > > >  - trigger unbind of PWM device via sysfs
> > > >
> > > > Because there is a device link PWM provider -> pwm consumer (fan), the
> > > > fan is removed and once its gone (and not earlier), the PWM gets unbound.
> > > >
> > > > The failing sequence is:
> > > >
> > > >  - trigger unbind of fan device in userspace thread A via sysfs. The
> > > >    fan's remove callback blocks for 5s in pwm_apply_state() and so
> > > >    .remove() doesn't complete yet.
> > > >
> > > >  - a second later: trigger unbind of PWM device via sysfs in thread B.
> > > >    As before I'd expect that the device link results in waiting for the
> > > >    fan to be removed completely, but the PWM is removed immediately.
> > > >
> > > >  - pwm_apply_state's sleep completes (in thread B) and operates on freed
> > > >    resources => bang!
> > > >
> > > > > This all sounds like insufficient locking and
> > > > > critical region protection in both the consumer and supplier.
> > > >
> > > > My (and I think also Thierry's) expectation was, that the device link
> > > > provides the needed synchronisation. But it doesn't as it doesn't block
> > > > the PWM provider going away until the fan is completely gone.
> > > >
> > > > > Device links can't do anything here because you are giving it wrong
> > > > > info -- that the unbind was successful before it actually is.
> > > >
> > > > The fan's unbind is ongoing, but not complete yet and I'd expect that
> > > > the device link blocks unbinding the PWM until the fan is completely
> > > > gone. So I think there is no wrong information.
> > > >
> > > > > Device links will and can make sure that the consumer is unbound
> > > > > successfully before the unbind is called on the supplier. And it looks
> > > > > like that's still true here.
> > > >
> > > > I hope you understood the situation better now and see the problem we
> > > > have.
> > > >
> > > > The problem is fixable in the pwm framework (and I'm working on that),
> > > > but I think there is also something to improve around devicelink
> > > > handling.
> > >
> > > Thanks for a better explanation of the issue. I agree, this seems like
> > > something device links should be able to take care of.
> > >
> > > I'll take a look into this.
> >
> > Took me a while to debug this because I couldn't find the .remove()
> > function and I was very confused about what's going on.
> > I'm guessing you started hitting this issue only after moving to the
> > devm_ variant of the pwm APIs.
>
> Ah I see. That problem wouldn't happen if the fan called a pwm API
> function in its remove callback but that happens in a devm cleanup call
> (registered by devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx) in
> pwm_fan_probe()). I first thought you talked about
> 8c89fd866ad221af037ef0ec3d60b83d0b859c65.

Am I not talking about that commit?

Btw, I'm still a bit confused by this thread. In your earlier emails
to me, you said:

>  - trigger unbind of fan device in userspace thread A via sysfs. The
>  fan's remove callback blocks for 5s in pwm_apply_state() and so
>  .remove() doesn't complete yet.

But the latest tree (Tot) didn't have any .remove() function at all.
So I just decided to see if there's any issue in Tot and just fix
that. I'm glad my fix helps fixed the issue with the used of devm_*()
APIs.

However, are you really seeing the issue (supplier freed before
consumer) if you do the clean up in the .remove() function? If so,
there might still be another issue that needs to be fixed.

Thanks,
Saravana

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

[Uwe Kleine-König]

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

On Wed, Oct 18, 2023 at 02:01:30PM -0700, Saravana Kannan wrote:
> On Wed, Oct 18, 2023 at 4:17 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Tue, Oct 17, 2023 at 06:42:40PM -0700, Saravana Kannan wrote:
> > > On Tue, Oct 17, 2023 at 4:35 PM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Sat, Oct 14, 2023 at 9:17 AM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > >
> > > > > On Fri, Oct 13, 2023 at 02:42:20PM -0700, Saravana Kannan wrote:
> > > > > > On Tue, Oct 10, 2023 at 1:05 AM Uwe Kleine-König
> > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > >
> > > > > > > Hello Saravana,
> > > > > > >
> > > > > > > you were pointed out to me as the expert for device links. I found a
> > > > > > > problem with these.
> > > > > > >
> > > > > > > On Tue, Jul 25, 2023 at 11:10:04PM +0200, Uwe Kleine-König wrote:
> > > > > > > > Today I managed to trigger the problem I intend to address with this
> > > > > > > > series. My machine to test this on is an stm32mp157. To be able to
> > > > > > > > trigger the problem reliably I applied the following patches on top of
> > > > > > > > v6.5-rc1:
> > > > > > > >
> > > > > > > >  - pwm: stm32: Don't modify HW state in .remove() callback
> > > > > > > >    This is a cleanup that I already sent out.
> > > > > > > >    https://lore.kernel.org/r/20230713155142.2454010-2-u.kleine-koenig@pengutronix.de
> > > > > > > >    The purpose for reproducing the problem is to not trigger further
> > > > > > > >    calls to the apply callback.
> > > > > > > >
> > > > > > > >  - The following patch:
> > > > > > > >
> > > > > > > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > > > > > > > index 687967d3265f..c7fc02b0fa3c 100644
> > > > > > > > --- a/drivers/pwm/pwm-stm32.c
> > > > > > > > +++ b/drivers/pwm/pwm-stm32.c
> > > > > > > > @@ -451,6 +451,10 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > > > >       struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > > > > > > >       int ret;
> > > > > > > >
> > > > > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > > +     msleep(5000);
> > > > > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > > +
> > > > > > > >       enabled = pwm->state.enabled;
> > > > > > > >
> > > > > > > >       if (enabled && !state->enabled) {
> > > > > > > > @@ -650,7 +654,11 @@ static void stm32_pwm_remove(struct platform_device *pdev)
> > > > > > > >  {
> > > > > > > >       struct stm32_pwm *priv = platform_get_drvdata(pdev);
> > > > > > > >
> > > > > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > >       pwmchip_remove(&priv->chip);
> > > > > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > > +
> > > > > > > > +     priv->regmap = NULL;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> > > > > > > >
> > > > > > > > The first hunk is only there to widen the race window. The second is to
> > > > > > > > give some diagnostics and make stm32_pwm_apply() crash if it continues
> > > > > > > > to run after the msleep. (Without it it didn't crash reproducibly, don't
> > > > > > > > understand why. *shrug*)
> > > > > > > >
> > > > > > > > The device tree contains a pwm-fan device making use of one of the PWMs.
> > > > > > > >
> > > > > > > > Now I do the following:
> > > > > > > >
> > > > > > > >       echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > > > > >
> > > > > > > > Unbinding the fan device has two effects:
> > > > > > > >
> > > > > > > >  - The device link between fan and pwm looses its property to unbind fan
> > > > > > > >    when pwm gets unbound.
> > > > > > > >    (Its .status changes from DL_STATE_ACTIVE to DL_STATE_AVAILABLE)
> > > > > > > >  - It calls pwm_fan_cleanup() which triggers a call to
> > > > > > > >    pwm_apply_state().
> > > > > > > >
> > > > > > > > So when the pwm device gets unbound the first thread is sleeping in
> > > > > > > > stm32_pwm_apply(). The driver calls pwmchip_remove() and sets
> > > > > > > > priv->regmap to NULL. Then a few seconds later the first thread wakes up
> > > > > > > > in stm32_pwm_apply() with the chip freed and priv->regmap = NULL. Bang!
> > > > > > > >
> > > > > > > > This looks as follows:
> > > > > > > >
> > > > > > > > root@crown:~# echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > > > > > [  187.182113] stm32-pwm 40007000.timer:pwm: stm32_pwm_apply:454
> > > > > > > > [  188.164769] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:657
> > > > > > > > [  188.184555] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:659
> > > > > > > > root@crown:~# [  192.236423] platform 40007000.timer:pwm: stm32_pwm_apply:456
> > > > > > > > [  192.240727] 8<--- cut here ---
> > > > > > > > [  192.243759] Unable to handle kernel NULL pointer dereference at virtual address 0000001c when read
> > > > > > > > ...
> > > > > > > >
> > > > > > > > Even without the crash you can see that stm32_pwm_apply() is still
> > > > > > > > running after pwmchip_remove() completed.
> > > > > > > >
> > > > > > > > I'm unsure if the device link could be improved here to ensure that the
> > > > > > > > fan is completely unbound even if it started unbinding already before
> > > > > > > > the pwm device gets unbound. (And if it could, would this fit the device
> > > > > > > > links purpose and so be a sensible improvement?)
> > > > > > >
> > > > > > > While I think that there is something to be done in the pwm core that
> > > > > > > this doesn't explode (i.e. do proper lifetime tracking such that a
> > > > > > > pwm_chip doesn't disappear while still being used---and I'm working on
> > > > > > > that) I expected that the device links between pwm consumer and provider
> > > > > > > would prevent the above described oops, too. But somehow the fan already
> > > > > > > going away (but still using the PWM) when the PWM is unbound, results in
> > > > > > > the PWM disappearing before the fan is completely gone.
> > > > > > >
> > > > > > > Is this expected, or a problem that can (and should?) be fixed?
> > > > > >
> > > > > > I didn't read your full series, but I read this email. With what's in
> > > > > > this email, the problem seems to be in the driver or the pwm
> > > > > > framework. The pwm driver/framework can't tell the driver core that
> > > > > > you successfully unbound (returning from .remove()) before you have
> > > > > > finish all your ongoing transactions with the device. If your
> > > > > > "apply()" is still running, you need to make sure it's complete before
> > > > > > .remove() does any resource releasing/clean up.
> > > > > >
> > > > > > Also, how is the consumer driver's .remove() succeeding if it has an
> > > > > > ongoing pwm call()?
> > > > >
> > > > > The thing that works fine and as expected is:
> > > > >
> > > > >  - trigger unbind of PWM device via sysfs
> > > > >
> > > > > Because there is a device link PWM provider -> pwm consumer (fan), the
> > > > > fan is removed and once its gone (and not earlier), the PWM gets unbound.
> > > > >
> > > > > The failing sequence is:
> > > > >
> > > > >  - trigger unbind of fan device in userspace thread A via sysfs. The
> > > > >    fan's remove callback blocks for 5s in pwm_apply_state() and so
> > > > >    .remove() doesn't complete yet.
> > > > >
> > > > >  - a second later: trigger unbind of PWM device via sysfs in thread B.
> > > > >    As before I'd expect that the device link results in waiting for the
> > > > >    fan to be removed completely, but the PWM is removed immediately.
> > > > >
> > > > >  - pwm_apply_state's sleep completes (in thread B) and operates on freed
> > > > >    resources => bang!
> > > > >
> > > > > > This all sounds like insufficient locking and
> > > > > > critical region protection in both the consumer and supplier.
> > > > >
> > > > > My (and I think also Thierry's) expectation was, that the device link
> > > > > provides the needed synchronisation. But it doesn't as it doesn't block
> > > > > the PWM provider going away until the fan is completely gone.
> > > > >
> > > > > > Device links can't do anything here because you are giving it wrong
> > > > > > info -- that the unbind was successful before it actually is.
> > > > >
> > > > > The fan's unbind is ongoing, but not complete yet and I'd expect that
> > > > > the device link blocks unbinding the PWM until the fan is completely
> > > > > gone. So I think there is no wrong information.
> > > > >
> > > > > > Device links will and can make sure that the consumer is unbound
> > > > > > successfully before the unbind is called on the supplier. And it looks
> > > > > > like that's still true here.
> > > > >
> > > > > I hope you understood the situation better now and see the problem we
> > > > > have.
> > > > >
> > > > > The problem is fixable in the pwm framework (and I'm working on that),
> > > > > but I think there is also something to improve around devicelink
> > > > > handling.
> > > >
> > > > Thanks for a better explanation of the issue. I agree, this seems like
> > > > something device links should be able to take care of.
> > > >
> > > > I'll take a look into this.
> > >
> > > Took me a while to debug this because I couldn't find the .remove()
> > > function and I was very confused about what's going on.
> > > I'm guessing you started hitting this issue only after moving to the
> > > devm_ variant of the pwm APIs.
> >
> > Ah I see. That problem wouldn't happen if the fan called a pwm API
> > function in its remove callback but that happens in a devm cleanup call
> > (registered by devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx) in
> > pwm_fan_probe()). I first thought you talked about
> > 8c89fd866ad221af037ef0ec3d60b83d0b859c65.
> 
> Am I not talking about that commit?

The relevant thing is that the fan (i.e. the consumer) uses
pwm_apply_state() in a devm cleanup. If the pwm provider uses
devm_pwmchip_add or plain pwmchip_add + pwmchip_remove in .remove()
doesn't matter.

> Btw, I'm still a bit confused by this thread. In your earlier emails
> to me, you said:
> 
> >  - trigger unbind of fan device in userspace thread A via sysfs. The
> >  fan's remove callback blocks for 5s in pwm_apply_state() and so
> >  .remove() doesn't complete yet.
> 
> But the latest tree (Tot) didn't have any .remove() function at all.
> So I just decided to see if there's any issue in Tot and just fix
> that. I'm glad my fix helps fixed the issue with the used of devm_*()
> APIs.

I was sloppy here and called it "remove callback" when it was really a
devm cleanup call. Sorry if that confused you. I didn't expect this to
make a difference (and I'm not even sure I was aware this is a devm
cleanup and not directly .remove() when I wrote that).

> However, are you really seeing the issue (supplier freed before
> consumer) if you do the clean up in the .remove() function? If so,
> there might still be another issue that needs to be fixed.

I didn't test that and now having understood the issue you fixed and
seeing the effect, I confidently claim there is nothing to fix for
drivers that use pwm_apply_state in .remove().

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 01/18] pwm: Provide devm_pwmchip_alloc() function

[Saravana Kannan]

On Wed, Oct 18, 2023 at 2:21 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Wed, Oct 18, 2023 at 02:01:30PM -0700, Saravana Kannan wrote:
> > On Wed, Oct 18, 2023 at 4:17 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > On Tue, Oct 17, 2023 at 06:42:40PM -0700, Saravana Kannan wrote:
> > > > On Tue, Oct 17, 2023 at 4:35 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > On Sat, Oct 14, 2023 at 9:17 AM Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > >
> > > > > > On Fri, Oct 13, 2023 at 02:42:20PM -0700, Saravana Kannan wrote:
> > > > > > > On Tue, Oct 10, 2023 at 1:05 AM Uwe Kleine-König
> > > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > >
> > > > > > > > Hello Saravana,
> > > > > > > >
> > > > > > > > you were pointed out to me as the expert for device links. I found a
> > > > > > > > problem with these.
> > > > > > > >
> > > > > > > > On Tue, Jul 25, 2023 at 11:10:04PM +0200, Uwe Kleine-König wrote:
> > > > > > > > > Today I managed to trigger the problem I intend to address with this
> > > > > > > > > series. My machine to test this on is an stm32mp157. To be able to
> > > > > > > > > trigger the problem reliably I applied the following patches on top of
> > > > > > > > > v6.5-rc1:
> > > > > > > > >
> > > > > > > > >  - pwm: stm32: Don't modify HW state in .remove() callback
> > > > > > > > >    This is a cleanup that I already sent out.
> > > > > > > > >    https://lore.kernel.org/r/20230713155142.2454010-2-u.kleine-koenig@pengutronix.de
> > > > > > > > >    The purpose for reproducing the problem is to not trigger further
> > > > > > > > >    calls to the apply callback.
> > > > > > > > >
> > > > > > > > >  - The following patch:
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > > > > > > > > index 687967d3265f..c7fc02b0fa3c 100644
> > > > > > > > > --- a/drivers/pwm/pwm-stm32.c
> > > > > > > > > +++ b/drivers/pwm/pwm-stm32.c
> > > > > > > > > @@ -451,6 +451,10 @@ static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > > > > >       struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > > > > > > > >       int ret;
> > > > > > > > >
> > > > > > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > > > +     msleep(5000);
> > > > > > > > > +     dev_info(chip->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > > > +
> > > > > > > > >       enabled = pwm->state.enabled;
> > > > > > > > >
> > > > > > > > >       if (enabled && !state->enabled) {
> > > > > > > > > @@ -650,7 +654,11 @@ static void stm32_pwm_remove(struct platform_device *pdev)
> > > > > > > > >  {
> > > > > > > > >       struct stm32_pwm *priv = platform_get_drvdata(pdev);
> > > > > > > > >
> > > > > > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > > >       pwmchip_remove(&priv->chip);
> > > > > > > > > +     dev_info(&pdev->dev, "%s:%d\n", __func__, __LINE__);
> > > > > > > > > +
> > > > > > > > > +     priv->regmap = NULL;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  static int __maybe_unused stm32_pwm_suspend(struct device *dev)
> > > > > > > > >
> > > > > > > > > The first hunk is only there to widen the race window. The second is to
> > > > > > > > > give some diagnostics and make stm32_pwm_apply() crash if it continues
> > > > > > > > > to run after the msleep. (Without it it didn't crash reproducibly, don't
> > > > > > > > > understand why. *shrug*)
> > > > > > > > >
> > > > > > > > > The device tree contains a pwm-fan device making use of one of the PWMs.
> > > > > > > > >
> > > > > > > > > Now I do the following:
> > > > > > > > >
> > > > > > > > >       echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > > > > > >
> > > > > > > > > Unbinding the fan device has two effects:
> > > > > > > > >
> > > > > > > > >  - The device link between fan and pwm looses its property to unbind fan
> > > > > > > > >    when pwm gets unbound.
> > > > > > > > >    (Its .status changes from DL_STATE_ACTIVE to DL_STATE_AVAILABLE)
> > > > > > > > >  - It calls pwm_fan_cleanup() which triggers a call to
> > > > > > > > >    pwm_apply_state().
> > > > > > > > >
> > > > > > > > > So when the pwm device gets unbound the first thread is sleeping in
> > > > > > > > > stm32_pwm_apply(). The driver calls pwmchip_remove() and sets
> > > > > > > > > priv->regmap to NULL. Then a few seconds later the first thread wakes up
> > > > > > > > > in stm32_pwm_apply() with the chip freed and priv->regmap = NULL. Bang!
> > > > > > > > >
> > > > > > > > > This looks as follows:
> > > > > > > > >
> > > > > > > > > root@crown:~# echo fan > /sys/bus/platform/drivers/pwm-fan/unbind & sleep 1; echo 40007000.timer:pwm > /sys/bus/platform/drivers/stm32-pwm/unbind
> > > > > > > > > [  187.182113] stm32-pwm 40007000.timer:pwm: stm32_pwm_apply:454
> > > > > > > > > [  188.164769] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:657
> > > > > > > > > [  188.184555] stm32-pwm 40007000.timer:pwm: stm32_pwm_remove:659
> > > > > > > > > root@crown:~# [  192.236423] platform 40007000.timer:pwm: stm32_pwm_apply:456
> > > > > > > > > [  192.240727] 8<--- cut here ---
> > > > > > > > > [  192.243759] Unable to handle kernel NULL pointer dereference at virtual address 0000001c when read
> > > > > > > > > ...
> > > > > > > > >
> > > > > > > > > Even without the crash you can see that stm32_pwm_apply() is still
> > > > > > > > > running after pwmchip_remove() completed.
> > > > > > > > >
> > > > > > > > > I'm unsure if the device link could be improved here to ensure that the
> > > > > > > > > fan is completely unbound even if it started unbinding already before
> > > > > > > > > the pwm device gets unbound. (And if it could, would this fit the device
> > > > > > > > > links purpose and so be a sensible improvement?)
> > > > > > > >
> > > > > > > > While I think that there is something to be done in the pwm core that
> > > > > > > > this doesn't explode (i.e. do proper lifetime tracking such that a
> > > > > > > > pwm_chip doesn't disappear while still being used---and I'm working on
> > > > > > > > that) I expected that the device links between pwm consumer and provider
> > > > > > > > would prevent the above described oops, too. But somehow the fan already
> > > > > > > > going away (but still using the PWM) when the PWM is unbound, results in
> > > > > > > > the PWM disappearing before the fan is completely gone.
> > > > > > > >
> > > > > > > > Is this expected, or a problem that can (and should?) be fixed?
> > > > > > >
> > > > > > > I didn't read your full series, but I read this email. With what's in
> > > > > > > this email, the problem seems to be in the driver or the pwm
> > > > > > > framework. The pwm driver/framework can't tell the driver core that
> > > > > > > you successfully unbound (returning from .remove()) before you have
> > > > > > > finish all your ongoing transactions with the device. If your
> > > > > > > "apply()" is still running, you need to make sure it's complete before
> > > > > > > .remove() does any resource releasing/clean up.
> > > > > > >
> > > > > > > Also, how is the consumer driver's .remove() succeeding if it has an
> > > > > > > ongoing pwm call()?
> > > > > >
> > > > > > The thing that works fine and as expected is:
> > > > > >
> > > > > >  - trigger unbind of PWM device via sysfs
> > > > > >
> > > > > > Because there is a device link PWM provider -> pwm consumer (fan), the
> > > > > > fan is removed and once its gone (and not earlier), the PWM gets unbound.
> > > > > >
> > > > > > The failing sequence is:
> > > > > >
> > > > > >  - trigger unbind of fan device in userspace thread A via sysfs. The
> > > > > >    fan's remove callback blocks for 5s in pwm_apply_state() and so
> > > > > >    .remove() doesn't complete yet.
> > > > > >
> > > > > >  - a second later: trigger unbind of PWM device via sysfs in thread B.
> > > > > >    As before I'd expect that the device link results in waiting for the
> > > > > >    fan to be removed completely, but the PWM is removed immediately.
> > > > > >
> > > > > >  - pwm_apply_state's sleep completes (in thread B) and operates on freed
> > > > > >    resources => bang!
> > > > > >
> > > > > > > This all sounds like insufficient locking and
> > > > > > > critical region protection in both the consumer and supplier.
> > > > > >
> > > > > > My (and I think also Thierry's) expectation was, that the device link
> > > > > > provides the needed synchronisation. But it doesn't as it doesn't block
> > > > > > the PWM provider going away until the fan is completely gone.
> > > > > >
> > > > > > > Device links can't do anything here because you are giving it wrong
> > > > > > > info -- that the unbind was successful before it actually is.
> > > > > >
> > > > > > The fan's unbind is ongoing, but not complete yet and I'd expect that
> > > > > > the device link blocks unbinding the PWM until the fan is completely
> > > > > > gone. So I think there is no wrong information.
> > > > > >
> > > > > > > Device links will and can make sure that the consumer is unbound
> > > > > > > successfully before the unbind is called on the supplier. And it looks
> > > > > > > like that's still true here.
> > > > > >
> > > > > > I hope you understood the situation better now and see the problem we
> > > > > > have.
> > > > > >
> > > > > > The problem is fixable in the pwm framework (and I'm working on that),
> > > > > > but I think there is also something to improve around devicelink
> > > > > > handling.
> > > > >
> > > > > Thanks for a better explanation of the issue. I agree, this seems like
> > > > > something device links should be able to take care of.
> > > > >
> > > > > I'll take a look into this.
> > > >
> > > > Took me a while to debug this because I couldn't find the .remove()
> > > > function and I was very confused about what's going on.
> > > > I'm guessing you started hitting this issue only after moving to the
> > > > devm_ variant of the pwm APIs.
> > >
> > > Ah I see. That problem wouldn't happen if the fan called a pwm API
> > > function in its remove callback but that happens in a devm cleanup call
> > > (registered by devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx) in
> > > pwm_fan_probe()). I first thought you talked about
> > > 8c89fd866ad221af037ef0ec3d60b83d0b859c65.
> >
> > Am I not talking about that commit?
>
> The relevant thing is that the fan (i.e. the consumer) uses
> pwm_apply_state() in a devm cleanup. If the pwm provider uses
> devm_pwmchip_add or plain pwmchip_add + pwmchip_remove in .remove()
> doesn't matter.

Duh! For whatever reason I had forgotten that stm32 was the supplier
and was confusing myself.

>
> > Btw, I'm still a bit confused by this thread. In your earlier emails
> > to me, you said:
> >
> > >  - trigger unbind of fan device in userspace thread A via sysfs. The
> > >  fan's remove callback blocks for 5s in pwm_apply_state() and so
> > >  .remove() doesn't complete yet.
> >
> > But the latest tree (Tot) didn't have any .remove() function at all.
> > So I just decided to see if there's any issue in Tot and just fix
> > that. I'm glad my fix helps fixed the issue with the used of devm_*()
> > APIs.
>
> I was sloppy here and called it "remove callback" when it was really a
> devm cleanup call. Sorry if that confused you. I didn't expect this to
> make a difference (and I'm not even sure I was aware this is a devm
> cleanup and not directly .remove() when I wrote that).
>
> > However, are you really seeing the issue (supplier freed before
> > consumer) if you do the clean up in the .remove() function? If so,
> > there might still be another issue that needs to be fixed.
>
> I didn't test that and now having understood the issue you fixed and
> seeing the effect, I confidently claim there is nothing to fix for
> drivers that use pwm_apply_state in .remove().

Yeah, I'm convinced now too!

-Saravana