[PATCH 0/4] pwm: Some refactoring of pwmchip_add()

[PATCH 0/4] pwm: Some refactoring of pwmchip_add()

[Uwe Kleine-König]

Hello,

this is inspired by a patch suggested by Andy Shevchenko[1] and so it
conflicts with his patch.

Best regards
Uwe

[1] https://lore.kernel.org/linux-pwm/20221114170006.61751-1-andriy.shevchenko@linux.intel.com

Uwe Kleine-König (4):
  pwm: Document variables protected by pwm_lock
  pwm: Reduce time the pwm_lock mutex is held in pwmchip_add()
  pwm: Mark free pwm IDs as used in alloc_pwms()
  pwm: Don't initialize list head before calling list_add()

 drivers/pwm/core.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)


base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780
-- 
2.38.1

[PATCH 1/4] pwm: Document variables protected by pwm_lock

[Uwe Kleine-König]

To simplify validation of the used locking, document for the global pwm
mutex what it actually protects against concurrent access. Also note for
two functions modifying these that pwm_lock is held by the caller.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index d333e7422f4a..ebe06efe9de5 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -27,7 +27,10 @@
 
 static DEFINE_MUTEX(pwm_lookup_lock);
 static LIST_HEAD(pwm_lookup_list);
+
+/* protects access to pwm_chips, allocated_pwms, and pwm_tree */
 static DEFINE_MUTEX(pwm_lock);
+
 static LIST_HEAD(pwm_chips);
 static DECLARE_BITMAP(allocated_pwms, MAX_PWMS);
 static RADIX_TREE(pwm_tree, GFP_KERNEL);
@@ -37,6 +40,7 @@ static struct pwm_device *pwm_to_device(unsigned int pwm)
 	return radix_tree_lookup(&pwm_tree, pwm);
 }
 
+/* Called with pwm_lock held */
 static int alloc_pwms(unsigned int count)
 {
 	unsigned int start;
@@ -50,6 +54,7 @@ static int alloc_pwms(unsigned int count)
 	return start;
 }
 
+/* Called with pwm_lock held */
 static void free_pwms(struct pwm_chip *chip)
 {
 	unsigned int i;
-- 
2.38.1

Re: [PATCH 1/4] pwm: Document variables protected by pwm_lock

[Andy Shevchenko]

On Tue, Nov 15, 2022 at 10:15:12PM +0100, Uwe Kleine-König wrote:
> To simplify validation of the used locking, document for the global pwm
> mutex what it actually protects against concurrent access. Also note for
> two functions modifying these that pwm_lock is held by the caller.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index d333e7422f4a..ebe06efe9de5 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -27,7 +27,10 @@
>  
>  static DEFINE_MUTEX(pwm_lookup_lock);
>  static LIST_HEAD(pwm_lookup_list);
> +
> +/* protects access to pwm_chips, allocated_pwms, and pwm_tree */
>  static DEFINE_MUTEX(pwm_lock);
> +
>  static LIST_HEAD(pwm_chips);
>  static DECLARE_BITMAP(allocated_pwms, MAX_PWMS);
>  static RADIX_TREE(pwm_tree, GFP_KERNEL);
> @@ -37,6 +40,7 @@ static struct pwm_device *pwm_to_device(unsigned int pwm)
>  	return radix_tree_lookup(&pwm_tree, pwm);
>  }
>  
> +/* Called with pwm_lock held */
>  static int alloc_pwms(unsigned int count)
>  {
>  	unsigned int start;
> @@ -50,6 +54,7 @@ static int alloc_pwms(unsigned int count)
>  	return start;
>  }
>  
> +/* Called with pwm_lock held */
>  static void free_pwms(struct pwm_chip *chip)
>  {
>  	unsigned int i;
> -- 
> 2.38.1
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/4] pwm: Reduce time the pwm_lock mutex is held in pwmchip_add()

[Uwe Kleine-König]

This simplifies error handling as the need for goto error handling goes
away and at the end of the function the code can be simplified as this
code isn't used in the error case any more.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ebe06efe9de5..2338119a09d8 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -272,20 +272,21 @@ int pwmchip_add(struct pwm_chip *chip)
 	if (!pwm_ops_check(chip))
 		return -EINVAL;
 
+	chip->pwms = kcalloc(chip->npwm, sizeof(*pwm), GFP_KERNEL);
+	if (!chip->pwms)
+		return -ENOMEM;
+
 	mutex_lock(&pwm_lock);
 
 	ret = alloc_pwms(chip->npwm);
-	if (ret < 0)
-		goto out;
+	if (ret < 0) {
+		mutex_unlock(&pwm_lock);
+		kfree(chip->pwms);
+		return ret;
+	}
 
 	chip->base = ret;
 
-	chip->pwms = kcalloc(chip->npwm, sizeof(*pwm), GFP_KERNEL);
-	if (!chip->pwms) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
 	for (i = 0; i < chip->npwm; i++) {
 		pwm = &chip->pwms[i];
 
@@ -301,18 +302,14 @@ int pwmchip_add(struct pwm_chip *chip)
 	INIT_LIST_HEAD(&chip->list);
 	list_add(&chip->list, &pwm_chips);
 
-	ret = 0;
+	mutex_unlock(&pwm_lock);
 
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_add(chip);
 
-out:
-	mutex_unlock(&pwm_lock);
-
-	if (!ret)
-		pwmchip_sysfs_export(chip);
+	pwmchip_sysfs_export(chip);
 
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pwmchip_add);
 
-- 
2.38.1

Re: [PATCH 2/4] pwm: Reduce time the pwm_lock mutex is held in pwmchip_add()

[Andy Shevchenko]

On Tue, Nov 15, 2022 at 10:15:13PM +0100, Uwe Kleine-König wrote:
> This simplifies error handling as the need for goto error handling goes
> away and at the end of the function the code can be simplified as this
> code isn't used in the error case any more.

...

> +	mutex_unlock(&pwm_lock);
>  
>  	if (IS_ENABLED(CONFIG_OF))
>  		of_pwmchip_add(chip);

Why calling this without a lock is not a problem? Commit message doesn't share
a bit about this change.

> -out:
> -	mutex_unlock(&pwm_lock);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] pwm: Reduce time the pwm_lock mutex is held in pwmchip_add()

[Uwe Kleine-König]

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

Hello Andy,

On Wed, Nov 16, 2022 at 10:11:32AM +0200, Andy Shevchenko wrote:
> On Tue, Nov 15, 2022 at 10:15:13PM +0100, Uwe Kleine-König wrote:
> > This simplifies error handling as the need for goto error handling goes
> > away and at the end of the function the code can be simplified as this
> > code isn't used in the error case any more.
> 
> ...
> 
> > +	mutex_unlock(&pwm_lock);
> >  
> >  	if (IS_ENABLED(CONFIG_OF))
> >  		of_pwmchip_add(chip);
> 
> Why calling this without a lock is not a problem? Commit message doesn't share
> a bit about this change.

Maybe add another paragraph at the end reading:

Now memory allocation and the call to of_pwmchip_add() are done without
holding the lock. Both don't access the data structures protected by
&pwm_lock.

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 2/4] pwm: Reduce time the pwm_lock mutex is held in pwmchip_add()

[Andy Shevchenko]

On Thu, Nov 17, 2022 at 03:00:24PM +0100, Uwe Kleine-König wrote:
> On Wed, Nov 16, 2022 at 10:11:32AM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 15, 2022 at 10:15:13PM +0100, Uwe Kleine-König wrote:
> > > This simplifies error handling as the need for goto error handling goes
> > > away and at the end of the function the code can be simplified as this
> > > code isn't used in the error case any more.

...

> > > +	mutex_unlock(&pwm_lock);
> > >  
> > >  	if (IS_ENABLED(CONFIG_OF))
> > >  		of_pwmchip_add(chip);
> > 
> > Why calling this without a lock is not a problem? Commit message doesn't share
> > a bit about this change.
> 
> Maybe add another paragraph at the end reading:
> 
> Now memory allocation and the call to of_pwmchip_add() are done without
> holding the lock. Both don't access the data structures protected by
> &pwm_lock.

Good to me, with that added
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/4] pwm: Mark free pwm IDs as used in alloc_pwms()

[Uwe Kleine-König]

alloc_pwms() only identified a free range of IDs and this range was marked
as used only later by pwmchip_add(). Instead let alloc_pwms() already do
the marking (which makes the function actually allocating the range and so
justifies the function name). This way access to the allocated_pwms
bitfield is limited to two functions only.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 2338119a09d8..b43b24bd3c9f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -51,6 +51,8 @@ static int alloc_pwms(unsigned int count)
 	if (start + count > MAX_PWMS)
 		return -ENOSPC;
 
+	bitmap_set(allocated_pwms, start, count);
+
 	return start;
 }
 
@@ -297,8 +299,6 @@ int pwmchip_add(struct pwm_chip *chip)
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
 
-	bitmap_set(allocated_pwms, chip->base, chip->npwm);
-
 	INIT_LIST_HEAD(&chip->list);
 	list_add(&chip->list, &pwm_chips);
 
-- 
2.38.1

Re: [PATCH 3/4] pwm: Mark free pwm IDs as used in alloc_pwms()

[Andy Shevchenko]

On Tue, Nov 15, 2022 at 10:15:14PM +0100, Uwe Kleine-König wrote:
> alloc_pwms() only identified a free range of IDs and this range was marked
> as used only later by pwmchip_add(). Instead let alloc_pwms() already do
> the marking (which makes the function actually allocating the range and so
> justifies the function name). This way access to the allocated_pwms
> bitfield is limited to two functions only.

This change is a bit fragile in a long term. Currently we know that we have
no points of error after alloc_pwms() in ->probe(), but if somebody misses
this in the future, we became to the case where bitmap might be exhausted
(kinda resource leakage).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] pwm: Mark free pwm IDs as used in alloc_pwms()

[Uwe Kleine-König]

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

On Wed, Nov 16, 2022 at 10:17:08AM +0200, Andy Shevchenko wrote:
> On Tue, Nov 15, 2022 at 10:15:14PM +0100, Uwe Kleine-König wrote:
> > alloc_pwms() only identified a free range of IDs and this range was marked
> > as used only later by pwmchip_add(). Instead let alloc_pwms() already do
> > the marking (which makes the function actually allocating the range and so
> > justifies the function name). This way access to the allocated_pwms
> > bitfield is limited to two functions only.
> 
> This change is a bit fragile in a long term. Currently we know that we have
> no points of error after alloc_pwms() in ->probe(), but if somebody misses
> this in the future, we became to the case where bitmap might be exhausted
> (kinda resource leakage).

That is always the case for a function allocating resources. If you add
an error path after the (previously) last allocation, you have to care
for that.

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 3/4] pwm: Mark free pwm IDs as used in alloc_pwms()

[Andy Shevchenko]

On Tue, Nov 15, 2022 at 10:15:14PM +0100, Uwe Kleine-König wrote:
> alloc_pwms() only identified a free range of IDs and this range was marked
> as used only later by pwmchip_add(). Instead let alloc_pwms() already do
> the marking (which makes the function actually allocating the range and so
> justifies the function name). This way access to the allocated_pwms
> bitfield is limited to two functions only.

Let's assume that developers will be cautious about properly freeing allocated
resources.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 2338119a09d8..b43b24bd3c9f 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -51,6 +51,8 @@ static int alloc_pwms(unsigned int count)
>  	if (start + count > MAX_PWMS)
>  		return -ENOSPC;
>  
> +	bitmap_set(allocated_pwms, start, count);
> +
>  	return start;
>  }
>  
> @@ -297,8 +299,6 @@ int pwmchip_add(struct pwm_chip *chip)
>  		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
>  	}
>  
> -	bitmap_set(allocated_pwms, chip->base, chip->npwm);
> -
>  	INIT_LIST_HEAD(&chip->list);
>  	list_add(&chip->list, &pwm_chips);
>  
> -- 
> 2.38.1
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH 4/4] pwm: Don't initialize list head before calling list_add()

[Uwe Kleine-König]

list_add() just overwrites the members of the element to add (here:
chip->list) without any checks, even in the DEBUG_LIST case. So save the
effort to initialize the list.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

this patch I'm not sure about. A quick grep shows there are (only?) 40
more code locations that call INIT_LIST_HEAD just before list_add().
In my understanding INIT_LIST_HEAD is only to initialize lists, but
chip->list is not a list, but the data needed to track chip as a list
member.

Best regards
Uwe

 drivers/pwm/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index b43b24bd3c9f..61bacd8d9b44 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -299,7 +299,6 @@ int pwmchip_add(struct pwm_chip *chip)
 		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
 	}
 
-	INIT_LIST_HEAD(&chip->list);
 	list_add(&chip->list, &pwm_chips);
 
 	mutex_unlock(&pwm_lock);
-- 
2.38.1

Re: [PATCH 4/4] pwm: Don't initialize list head before calling list_add()

[Andy Shevchenko]

On Tue, Nov 15, 2022 at 10:15:15PM +0100, Uwe Kleine-König wrote:
> list_add() just overwrites the members of the element to add (here:
> chip->list) without any checks, even in the DEBUG_LIST case. So save the
> effort to initialize the list.

This is good patch. I agree with it.

The cause of this code appearing either some older changes, or cargo cult
of the trick similar to when list_del_init() is used against a list node.
(FYI: that trick is useful to simplify the check if the node in question
 belongs to any list, by calling list_empty() against _node_ pointer)

The _add_ case with initialization makes no sense to me,

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> this patch I'm not sure about. A quick grep shows there are (only?) 40
> more code locations that call INIT_LIST_HEAD just before list_add().
> In my understanding INIT_LIST_HEAD is only to initialize lists, but
> chip->list is not a list, but the data needed to track chip as a list
> member.
> 
> Best regards
> Uwe
> 
>  drivers/pwm/core.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index b43b24bd3c9f..61bacd8d9b44 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -299,7 +299,6 @@ int pwmchip_add(struct pwm_chip *chip)
>  		radix_tree_insert(&pwm_tree, pwm->pwm, pwm);
>  	}
>  
> -	INIT_LIST_HEAD(&chip->list);
>  	list_add(&chip->list, &pwm_chips);
>  
>  	mutex_unlock(&pwm_lock);
> -- 
> 2.38.1
> 

-- 
With Best Regards,
Andy Shevchenko