| Message ID | 398895f3b4f9ca4b03b47b56dfa25fbd58fd2311.1569319738.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Add dev_pm_qos support | expand |
Hi Leonard, I copied and pasted my comments as following: because you send new v8 version without waiting any reply. In my case, I don't prefer to remove 'devm_' feature. v7's comment. I'm not sure that it is necessary. Because, mainline kernel provides the resource-managed feature with 'devm_*' in order to reduce the memory leak by user. It's like going back to a time when 'devm_*' feature wasn't there. I think just it depends on the coding style of author. I leave the final decision of this patch to Myungjoo. On 19. 9. 24. 오후 7:11, Leonard Crestez wrote: > In general it is a better to initialize an object before making it > accessible externally (through device_register). > > This makes it possible to avoid relying on locking a partially > initialized object. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/devfreq/devfreq.c | 43 +++++++++++++++++++++++---------------- > 1 file changed, 25 insertions(+), 18 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 12c4bcdc1f17..dbc6dc882aba 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -588,10 +588,12 @@ static void devfreq_dev_release(struct device *dev) > mutex_unlock(&devfreq_list_lock); > > if (devfreq->profile->exit) > devfreq->profile->exit(devfreq->dev.parent); > > + kfree(devfreq->time_in_state); > + kfree(devfreq->trans_table); > mutex_destroy(&devfreq->lock); > kfree(devfreq); > } > > /** > @@ -671,44 +673,43 @@ struct devfreq *devfreq_add_device(struct device *dev, > devfreq->max_freq = devfreq->scaling_max_freq; > > 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); > - if (err) { > - mutex_unlock(&devfreq->lock); > - put_device(&devfreq->dev); > - goto err_out; > - } > - > - devfreq->trans_table = devm_kzalloc(&devfreq->dev, > + devfreq->trans_table = kzalloc( > array3_size(sizeof(unsigned int), > devfreq->profile->max_state, > devfreq->profile->max_state), > GFP_KERNEL); > if (!devfreq->trans_table) { > mutex_unlock(&devfreq->lock); > err = -ENOMEM; > - goto err_devfreq; > + goto err_dev; > } > > - devfreq->time_in_state = devm_kcalloc(&devfreq->dev, > - devfreq->profile->max_state, > - sizeof(unsigned long), > - GFP_KERNEL); > + devfreq->time_in_state = kcalloc(devfreq->profile->max_state, > + sizeof(unsigned long), > + GFP_KERNEL); > if (!devfreq->time_in_state) { > mutex_unlock(&devfreq->lock); > err = -ENOMEM; > - goto err_devfreq; > + goto err_dev; > } > > devfreq->last_stat_updated = jiffies; > > srcu_init_notifier_head(&devfreq->transition_notifier_list); > > + dev_set_name(&devfreq->dev, "devfreq%d", > + atomic_inc_return(&devfreq_no)); > + err = device_register(&devfreq->dev); > + if (err) { > + mutex_unlock(&devfreq->lock); > + put_device(&devfreq->dev); > + goto err_out; > + } > + > mutex_unlock(&devfreq->lock); > > mutex_lock(&devfreq_list_lock); > > governor = try_then_request_governor(devfreq->governor_name); > @@ -734,14 +735,20 @@ struct devfreq *devfreq_add_device(struct device *dev, > > return devfreq; > > err_init: > mutex_unlock(&devfreq_list_lock); > -err_devfreq: > devfreq_remove_device(devfreq); > - devfreq = NULL; > + return ERR_PTR(err); > + > err_dev: > + /* > + * Cleanup path for errors that happen before registration. > + * Otherwise we rely on devfreq_dev_release. > + */ > + kfree(devfreq->time_in_state); > + kfree(devfreq->trans_table); > kfree(devfreq); > err_out: > return ERR_PTR(err); > } > EXPORT_SYMBOL(devfreq_add_device); >
>In general it is a better to initialize an object before making it >accessible externally (through device_register). > >This makes it possible to avoid relying on locking a partially >initialized object. > >Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >Reviewed-by: Matthias Kaehlcke <mka@chromium.org> Do you object to the general idea of devm_* for device drivers? or did you find a bug in the memory handling in the code? The result of this commit still relies on locking anyway. Cheers, MyungJoo.
On 25.09.2019 04:59, MyungJoo Ham wrote: >> In general it is a better to initialize an object before making it >> accessible externally (through device_register). >> >> This makes it possible to avoid relying on locking a partially >> initialized object. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > > Do you object to the general idea of devm_* for device drivers? > or did you find a bug in the memory handling in the code? > > The result of this commit still relies on locking anyway. This is a dependency of the following patch which removes the locking. I will add this to the commit message because everybody asks. This patch removes devm because devm itself is only available after device_initialize. Moving the allocation ahead of device_register (which calls device_initialize) requires switching to manual memory management. Since there are only two pointers it seemed reasonable. Alternatively device_register could be split into explicit "device_initialize" and "device_add" steps and devm could be used between those steps. Before: - device_register - devm-based-alloc After: + device_initialize + devm-based-alloc + device_add Does this make sense? -- Regards, Leonard
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 12c4bcdc1f17..dbc6dc882aba 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -588,10 +588,12 @@ static void devfreq_dev_release(struct device *dev) mutex_unlock(&devfreq_list_lock); if (devfreq->profile->exit) devfreq->profile->exit(devfreq->dev.parent); + kfree(devfreq->time_in_state); + kfree(devfreq->trans_table); mutex_destroy(&devfreq->lock); kfree(devfreq); } /** @@ -671,44 +673,43 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->max_freq = devfreq->scaling_max_freq; 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); - if (err) { - mutex_unlock(&devfreq->lock); - put_device(&devfreq->dev); - goto err_out; - } - - devfreq->trans_table = devm_kzalloc(&devfreq->dev, + devfreq->trans_table = kzalloc( array3_size(sizeof(unsigned int), devfreq->profile->max_state, devfreq->profile->max_state), GFP_KERNEL); if (!devfreq->trans_table) { mutex_unlock(&devfreq->lock); err = -ENOMEM; - goto err_devfreq; + goto err_dev; } - devfreq->time_in_state = devm_kcalloc(&devfreq->dev, - devfreq->profile->max_state, - sizeof(unsigned long), - GFP_KERNEL); + devfreq->time_in_state = kcalloc(devfreq->profile->max_state, + sizeof(unsigned long), + GFP_KERNEL); if (!devfreq->time_in_state) { mutex_unlock(&devfreq->lock); err = -ENOMEM; - goto err_devfreq; + goto err_dev; } devfreq->last_stat_updated = jiffies; srcu_init_notifier_head(&devfreq->transition_notifier_list); + dev_set_name(&devfreq->dev, "devfreq%d", + atomic_inc_return(&devfreq_no)); + err = device_register(&devfreq->dev); + if (err) { + mutex_unlock(&devfreq->lock); + put_device(&devfreq->dev); + goto err_out; + } + mutex_unlock(&devfreq->lock); mutex_lock(&devfreq_list_lock); governor = try_then_request_governor(devfreq->governor_name); @@ -734,14 +735,20 @@ struct devfreq *devfreq_add_device(struct device *dev, return devfreq; err_init: mutex_unlock(&devfreq_list_lock); -err_devfreq: devfreq_remove_device(devfreq); - devfreq = NULL; + return ERR_PTR(err); + err_dev: + /* + * Cleanup path for errors that happen before registration. + * Otherwise we rely on devfreq_dev_release. + */ + kfree(devfreq->time_in_state); + kfree(devfreq->trans_table); kfree(devfreq); err_out: return ERR_PTR(err); } EXPORT_SYMBOL(devfreq_add_device);