| Message ID | 20221115211515.3750209-3-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: Some refactoring of pwmchip_add() | expand |
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);
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
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>
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);
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(-)