| Message ID | 85ccf6afe5db556c610ce2b47ccc38132b6671f6.1573686315.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | PM / devfreq: Don't take lock in devfreq_add_device | expand |
On 11/14/19 8:21 AM, Leonard Crestez wrote: > Splitting device_register into device_initialize and device_add allows > devm-based allocations to be performed before device_add. > > It also simplifies error paths in devfreq_add_device: just call > put_device instead of duplicating parts of devfreq_dev_release. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/devfreq/devfreq.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 27af1b95fd23..b89a82382536 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -689,10 +689,11 @@ struct devfreq *devfreq_add_device(struct device *dev, > mutex_init(&devfreq->lock); > mutex_lock(&devfreq->lock); > devfreq->dev.parent = dev; > devfreq->dev.class = devfreq_class; > devfreq->dev.release = devfreq_dev_release; > + device_initialize(&devfreq->dev); > INIT_LIST_HEAD(&devfreq->node); > devfreq->profile = profile; > strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN); > devfreq->previous_freq = profile->initial_freq; > devfreq->last_status.current_frequency = profile->initial_freq; > @@ -726,15 +727,14 @@ struct devfreq *devfreq_add_device(struct device *dev, > devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); > atomic_set(&devfreq->suspend_count, 0); > > dev_set_name(&devfreq->dev, "devfreq%d", > atomic_inc_return(&devfreq_no)); > - err = device_register(&devfreq->dev); > + err = device_add(&devfreq->dev); > if (err) { > mutex_unlock(&devfreq->lock); > - put_device(&devfreq->dev); > - goto err_out; > + goto err_dev; > } > > devfreq->trans_table = devm_kzalloc(&devfreq->dev, > array3_size(sizeof(unsigned int), > devfreq->profile->max_state, > @@ -789,13 +789,13 @@ struct devfreq *devfreq_add_device(struct device *dev, > > err_init: > mutex_unlock(&devfreq_list_lock); > err_devfreq: > devfreq_remove_device(devfreq); > - devfreq = NULL; > + return ERR_PTR(err); > err_dev: > - kfree(devfreq); > + put_device(&devfreq->dev); > err_out: > return ERR_PTR(err); > } > EXPORT_SYMBOL(devfreq_add_device); > > As I previously commented, I don't prefer to split out of bodyf of device_register(). Instead, your first version is better without devm. Regards, Chanwoo Choi
On 2019-12-02 3:02 AM, Chanwoo Choi wrote: > On 11/14/19 8:21 AM, Leonard Crestez wrote: >> Splitting device_register into device_initialize and device_add allows >> devm-based allocations to be performed before device_add. >> >> It also simplifies error paths in devfreq_add_device: just call >> put_device instead of duplicating parts of devfreq_dev_release. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> --- >> drivers/devfreq/devfreq.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 27af1b95fd23..b89a82382536 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -689,10 +689,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >> mutex_init(&devfreq->lock); >> mutex_lock(&devfreq->lock); >> devfreq->dev.parent = dev; >> devfreq->dev.class = devfreq_class; >> devfreq->dev.release = devfreq_dev_release; >> + device_initialize(&devfreq->dev); >> INIT_LIST_HEAD(&devfreq->node); >> devfreq->profile = profile; >> strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN); >> devfreq->previous_freq = profile->initial_freq; >> devfreq->last_status.current_frequency = profile->initial_freq; >> @@ -726,15 +727,14 @@ struct devfreq *devfreq_add_device(struct device *dev, >> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); >> atomic_set(&devfreq->suspend_count, 0); >> >> dev_set_name(&devfreq->dev, "devfreq%d", >> atomic_inc_return(&devfreq_no)); >> - err = device_register(&devfreq->dev); >> + err = device_add(&devfreq->dev); >> if (err) { >> mutex_unlock(&devfreq->lock); >> - put_device(&devfreq->dev); >> - goto err_out; >> + goto err_dev; >> } >> >> devfreq->trans_table = devm_kzalloc(&devfreq->dev, >> array3_size(sizeof(unsigned int), >> devfreq->profile->max_state, >> @@ -789,13 +789,13 @@ struct devfreq *devfreq_add_device(struct device *dev, >> >> err_init: >> mutex_unlock(&devfreq_list_lock); >> err_devfreq: >> devfreq_remove_device(devfreq); >> - devfreq = NULL; >> + return ERR_PTR(err); >> err_dev: >> - kfree(devfreq); >> + put_device(&devfreq->dev); >> err_out: >> return ERR_PTR(err); >> } >> EXPORT_SYMBOL(devfreq_add_device); >> >> > > As I previously commented, I don't prefer to split out of bodyf of device_register(). > Instead, your first version is better without devm. Very well, feel free to drop 2-5 of this series then. Or perhaps I misunderstood and the locking cleanups would be acceptable in the variant that removes devm from a few allocations? There's quite a bunch of stuff flying around the merge window already so I'll refrain from posting until v5.5-rc1 anyway. I went a little overboard with tricky cleanups and this ended up delaying the functionality I wanted to push. -- Regards, Leonard
On 12/2/19 1:45 PM, Leonard Crestez wrote: > On 2019-12-02 3:02 AM, Chanwoo Choi wrote: >> On 11/14/19 8:21 AM, Leonard Crestez wrote: >>> Splitting device_register into device_initialize and device_add allows >>> devm-based allocations to be performed before device_add. >>> >>> It also simplifies error paths in devfreq_add_device: just call >>> put_device instead of duplicating parts of devfreq_dev_release. >>> >>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>> --- >>> drivers/devfreq/devfreq.c | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index 27af1b95fd23..b89a82382536 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -689,10 +689,11 @@ struct devfreq *devfreq_add_device(struct device *dev, >>> mutex_init(&devfreq->lock); >>> mutex_lock(&devfreq->lock); >>> devfreq->dev.parent = dev; >>> devfreq->dev.class = devfreq_class; >>> devfreq->dev.release = devfreq_dev_release; >>> + device_initialize(&devfreq->dev); >>> INIT_LIST_HEAD(&devfreq->node); >>> devfreq->profile = profile; >>> strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN); >>> devfreq->previous_freq = profile->initial_freq; >>> devfreq->last_status.current_frequency = profile->initial_freq; >>> @@ -726,15 +727,14 @@ struct devfreq *devfreq_add_device(struct device *dev, >>> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); >>> atomic_set(&devfreq->suspend_count, 0); >>> >>> dev_set_name(&devfreq->dev, "devfreq%d", >>> atomic_inc_return(&devfreq_no)); >>> - err = device_register(&devfreq->dev); >>> + err = device_add(&devfreq->dev); >>> if (err) { >>> mutex_unlock(&devfreq->lock); >>> - put_device(&devfreq->dev); >>> - goto err_out; >>> + goto err_dev; >>> } >>> >>> devfreq->trans_table = devm_kzalloc(&devfreq->dev, >>> array3_size(sizeof(unsigned int), >>> devfreq->profile->max_state, >>> @@ -789,13 +789,13 @@ struct devfreq *devfreq_add_device(struct device *dev, >>> >>> err_init: >>> mutex_unlock(&devfreq_list_lock); >>> err_devfreq: >>> devfreq_remove_device(devfreq); >>> - devfreq = NULL; >>> + return ERR_PTR(err); >>> err_dev: >>> - kfree(devfreq); >>> + put_device(&devfreq->dev); >>> err_out: >>> return ERR_PTR(err); >>> } >>> EXPORT_SYMBOL(devfreq_add_device); >>> >>> >> >> As I previously commented, I don't prefer to split out of bodyf of device_register(). >> Instead, your first version is better without devm. > > Very well, feel free to drop 2-5 of this series then. > > Or perhaps I misunderstood and the locking cleanups would be acceptable > in the variant that removes devm from a few allocations? There's quite a > bunch of stuff flying around the merge window already so I'll refrain > from posting until v5.5-rc1 anyway. Don't need to wait the v5.5-rc1. You can send the patches. But, This series have to be merged to v5.6-rc1. > > I went a little overboard with tricky cleanups and this ended up > delaying the functionality I wanted to push. > > -- > Regards, > Leonard > >
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 27af1b95fd23..b89a82382536 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -689,10 +689,11 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_init(&devfreq->lock); mutex_lock(&devfreq->lock); devfreq->dev.parent = dev; devfreq->dev.class = devfreq_class; devfreq->dev.release = devfreq_dev_release; + device_initialize(&devfreq->dev); INIT_LIST_HEAD(&devfreq->node); devfreq->profile = profile; strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN); devfreq->previous_freq = profile->initial_freq; devfreq->last_status.current_frequency = profile->initial_freq; @@ -726,15 +727,14 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); atomic_set(&devfreq->suspend_count, 0); dev_set_name(&devfreq->dev, "devfreq%d", atomic_inc_return(&devfreq_no)); - err = device_register(&devfreq->dev); + err = device_add(&devfreq->dev); if (err) { mutex_unlock(&devfreq->lock); - put_device(&devfreq->dev); - goto err_out; + goto err_dev; } devfreq->trans_table = devm_kzalloc(&devfreq->dev, array3_size(sizeof(unsigned int), devfreq->profile->max_state, @@ -789,13 +789,13 @@ struct devfreq *devfreq_add_device(struct device *dev, err_init: mutex_unlock(&devfreq_list_lock); err_devfreq: devfreq_remove_device(devfreq); - devfreq = NULL; + return ERR_PTR(err); err_dev: - kfree(devfreq); + put_device(&devfreq->dev); err_out: return ERR_PTR(err); } EXPORT_SYMBOL(devfreq_add_device);
Splitting device_register into device_initialize and device_add allows devm-based allocations to be performed before device_add. It also simplifies error paths in devfreq_add_device: just call put_device instead of duplicating parts of devfreq_dev_release. Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- drivers/devfreq/devfreq.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)