| Message ID | 59bd0d871fad520eb417ca46943fa7f86ef9186a.1568764439.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Add dev_pm_qos support | expand |
Hi Leonard, On Wed, Sep 18, 2019 at 03:18:22AM +0300, Leonard Crestez wrote: > In general it is a better to initialize an object before making it > accessible externally (through device_register). > > This make it possible to avoid relying on locking a partially > initialized object. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index a715f27f35fd..57a217fc92de 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev) > > if (devfreq->profile->exit) > devfreq->profile->exit(devfreq->dev.parent); > > mutex_destroy(&devfreq->lock); > + kfree(devfreq->time_in_state); > + kfree(devfreq->trans_table); > kfree(devfreq); > } > > /** > * devfreq_add_device() - Add devfreq feature to the device > @@ -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; goto err_dev; > + } > + > mutex_unlock(&devfreq->lock); > > mutex_lock(&devfreq_list_lock); > > governor = try_then_request_governor(devfreq->governor_name); > @@ -734,14 +735,15 @@ 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); The two return paths in the unwind part are unorthodox, but I see why they are needed. Maybe add an empty line between the two paths to make it a bit more evident that they are separate. > err_dev: This code path should include mutex_destroy(&devfreq->lock); That was already missing in the original code though. Actually with the later device registration the mutex could be initialized later and doesn't need to be held. This would obsolete the mutex_unlock() calls in the error paths.
On Wed, Sep 18, 2019 at 04:29:04PM -0700, Matthias Kaehlcke wrote: > Hi Leonard, > > On Wed, Sep 18, 2019 at 03:18:22AM +0300, Leonard Crestez wrote: > > In general it is a better to initialize an object before making it > > accessible externally (through device_register). > > > > This make it possible to avoid relying on locking a partially > > initialized object. > > > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > > --- > > drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------ > > 1 file changed, 20 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > index a715f27f35fd..57a217fc92de 100644 > > --- a/drivers/devfreq/devfreq.c > > +++ b/drivers/devfreq/devfreq.c > > @@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev) > > > > if (devfreq->profile->exit) > > devfreq->profile->exit(devfreq->dev.parent); > > > > mutex_destroy(&devfreq->lock); > > + kfree(devfreq->time_in_state); > > + kfree(devfreq->trans_table); > > kfree(devfreq); > > } > > > > /** > > * devfreq_add_device() - Add devfreq feature to the device > > @@ -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; > > goto err_dev; > > > + } > > + > > mutex_unlock(&devfreq->lock); > > > > mutex_lock(&devfreq_list_lock); > > > > governor = try_then_request_governor(devfreq->governor_name); > > @@ -734,14 +735,15 @@ 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); > > The two return paths in the unwind part are unorthodox, but I > see why they are needed. Maybe add an empty line between the two paths > to make it a bit more evident that they are separate. > > > err_dev: > > This code path should include > > mutex_destroy(&devfreq->lock); > > That was already missing in the original code though. > > Actually with the later device registration the mutex could be > initialized later and doesn't need to be held. This would > obsolete the mutex_unlock() calls in the error paths. Just saw that you are already doing this in "[4/8] PM / devfreq: Don't take lock in devfreq_add_device" :)
On 19.09.2019 02:29, Matthias Kaehlcke wrote: > Hi Leonard, > > On Wed, Sep 18, 2019 at 03:18:22AM +0300, Leonard Crestez wrote: >> In general it is a better to initialize an object before making it >> accessible externally (through device_register). >> >> This make it possible to avoid relying on locking a partially >> initialized object. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> --- >> drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------ >> 1 file changed, 20 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index a715f27f35fd..57a217fc92de 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev) >> >> if (devfreq->profile->exit) >> devfreq->profile->exit(devfreq->dev.parent); >> >> mutex_destroy(&devfreq->lock); >> + kfree(devfreq->time_in_state); >> + kfree(devfreq->trans_table); >> kfree(devfreq); >> } >> >> /** >> * devfreq_add_device() - Add devfreq feature to the device >> @@ -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; > > goto err_dev; > >> + } >> + >> mutex_unlock(&devfreq->lock); >> >> mutex_lock(&devfreq_list_lock); >> >> governor = try_then_request_governor(devfreq->governor_name); >> @@ -734,14 +735,15 @@ 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); > > The two return paths in the unwind part are unorthodox, but I > see why they are needed. Maybe add an empty line between the two paths > to make it a bit more evident that they are separate. Old code did "devfreq = NULL" just so that the later kfree did nothing. There were already two unwind paths, it's just that the second one was less obvious. I will add a comment. >> err_dev: > > This code path should include > > mutex_destroy(&devfreq->lock); > > That was already missing in the original code though. Yes, that would be a separate patch. > Actually with the later device registration the mutex could be > initialized later and doesn't need to be held. This would > obsolete the mutex_unlock() calls in the error paths Next patch already removes mutex_lock before device_register (that's the purpose of the cleanup). If you're suggesting to move mutex_init around it's not clear what would be gained? -- Regards, Leonard
On Thu, Sep 19, 2019 at 06:52:07PM +0000, Leonard Crestez wrote: > On 19.09.2019 02:29, Matthias Kaehlcke wrote: > > Hi Leonard, > > > > On Wed, Sep 18, 2019 at 03:18:22AM +0300, Leonard Crestez wrote: > >> In general it is a better to initialize an object before making it > >> accessible externally (through device_register). > >> > >> This make it possible to avoid relying on locking a partially > >> initialized object. > >> > >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > >> --- > >> drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------ > >> 1 file changed, 20 insertions(+), 18 deletions(-) > >> > >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > >> index a715f27f35fd..57a217fc92de 100644 > >> --- a/drivers/devfreq/devfreq.c > >> +++ b/drivers/devfreq/devfreq.c > >> @@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev) > >> > >> if (devfreq->profile->exit) > >> devfreq->profile->exit(devfreq->dev.parent); > >> > >> mutex_destroy(&devfreq->lock); > >> + kfree(devfreq->time_in_state); > >> + kfree(devfreq->trans_table); > >> kfree(devfreq); > >> } > >> > >> /** > >> * devfreq_add_device() - Add devfreq feature to the device > >> @@ -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; > > > > goto err_dev; > > > >> + } > >> + > >> mutex_unlock(&devfreq->lock); > >> > >> mutex_lock(&devfreq_list_lock); > >> > >> governor = try_then_request_governor(devfreq->governor_name); > >> @@ -734,14 +735,15 @@ 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); > > > > The two return paths in the unwind part are unorthodox, but I > > see why they are needed. Maybe add an empty line between the two paths > > to make it a bit more evident that they are separate. > > Old code did "devfreq = NULL" just so that the later kfree did nothing. > There were already two unwind paths, it's just that the second one was > less obvious. I will add a comment. > > >> err_dev: > > > > This code path should include > > > > mutex_destroy(&devfreq->lock); > > > > That was already missing in the original code though. > > Yes, that would be a separate patch. > > > Actually with the later device registration the mutex could be > > initialized later and doesn't need to be held. This would > > obsolete the mutex_unlock() calls in the error paths > Next patch already removes mutex_lock before device_register (that's the > purpose of the cleanup). If you're suggesting to move mutex_init around > it's not clear what would be gained? As per my earlier reply to self: I didn't look at the next patch before writing this, it's all good, nothing to do here :)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index a715f27f35fd..57a217fc92de 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -589,10 +589,12 @@ static void devfreq_dev_release(struct device *dev) if (devfreq->profile->exit) devfreq->profile->exit(devfreq->dev.parent); mutex_destroy(&devfreq->lock); + kfree(devfreq->time_in_state); + kfree(devfreq->trans_table); kfree(devfreq); } /** * devfreq_add_device() - Add devfreq feature to the device @@ -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,15 @@ 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: + 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 make it possible to avoid relying on locking a partially initialized object. Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- drivers/devfreq/devfreq.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-)