diff mbox series

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

Message ID 20221115211515.3750209-4-u.kleine-koenig@pengutronix.de
State New
Headers
Series pwm: Some refactoring of pwmchip_add() |

Commit Message

Uwe Kleine-König Nov. 15, 2022, 9:15 p.m. UTC 
  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(-)

Comments

Andy Shevchenko Nov. 16, 2022, 8:17 a.m. UTC | #1 
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).
Uwe Kleine-König Nov. 16, 2022, 1:59 p.m. UTC | #2 
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
Andy Shevchenko Nov. 16, 2022, 2:04 p.m. UTC | #3 
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
>
diff mbox series

Patch

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);