Re: [PATCH v3] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[Enric Balletbo i Serra]

Hi Ezequiel and Akhil,

On 22/06/18 09:03, Akhil P Oommen wrote:
> 
> On 6/22/2018 6:41 AM, Ezequiel Garcia wrote:
>> Hey Enric,
>>
>> On Fri, 2018-06-22 at 00:04 +0200, Enric Balletbo i Serra wrote:
>>> When the devfreq driver and the governor driver are built as modules,
>>> the call to devfreq_add_device() or governor_store() fails because
>>> the
>>> governor driver is not loaded at the time the devfreq driver loads.
>>> The
>>> devfreq driver has a build dependency on the governor but also should
>>> have a runtime dependency. We need to make sure that the governor
>>> driver
>>> is loaded before the devfreq driver.
>>>
>>> This patch fixes this bug by adding a try_then_request_governor()
>>> function. First tries to find the governor, and then, if it is not
>>> found,
>>> it requests the module and tries again.
>>>
>>> Fixes: 1b5c1be2c88e (PM / devfreq: map devfreq drivers to governor
>>> using name)
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>>> ---
>>>
>>> Changes in v3:
>>> - Remove unneded change in dev_err message.
>>> - Fix err returned value in case to not find the governor.
>>>
>>> Changes in v2:
>>> - Add a new function to request the module and call that function
>>> from
>>> ÂÂ devfreq_add_device and governor_store.
>>>
>>> Â drivers/devfreq/devfreq.c | 65 ++++++++++++++++++++++++++++++++-----
>>> -- 
>> [snip snip]
>>> -ÂÂÂ governor = find_devfreq_governor(devfreq->governor_name);
>>> +ÂÂÂ governor = try_then_request_governor(devfreq-
>>>> governor_name);
>>> ÂÂÂÂÂ if (IS_ERR(governor)) {
>>> ÂÂÂÂÂÂÂÂÂ dev_err(dev, "%s: Unable to find governor for the
>>> device\n",
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ __func__);
>>> ÂÂÂÂÂÂÂÂÂ err = PTR_ERR(governor);
>>> -ÂÂÂÂÂÂÂ goto err_init;
>>> +ÂÂÂÂÂÂÂ goto err_unregister;
>>> ÂÂÂÂÂ }
>>> Â +ÂÂÂ mutex_lock(&devfreq_list_lock);
>>> +
>> I know it's not something we are introducing in this patch,
>> but still... calling a hook with a mutex held looks
>> fishy to me.
>>
>> This lock should only protect the list, unless I am missing
>> something.
>>

I think so too.

>>> ÂÂÂÂÂ devfreq->governor = governor;
>>> ÂÂÂÂÂ err = devfreq->governor->event_handler(devfreq,
>>> DEVFREQ_GOV_START,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ NULL);
>>> @@ -663,14 +703,16 @@ struct devfreq *devfreq_add_device(struct
>>> device *dev,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ __func__);
>>> ÂÂÂÂÂÂÂÂÂ goto err_init;
>>> ÂÂÂÂÂ }
>>> +
>>> +ÂÂÂ list_add(&devfreq->node, &devfreq_list);
>>> +
>>> ÂÂÂÂÂ mutex_unlock(&devfreq_list_lock);
>>> Â ÂÂÂÂÂ return devfreq;
>>> Â Â err_init:
>>> -ÂÂÂ list_del(&devfreq->node);
>>> ÂÂÂÂÂ mutex_unlock(&devfreq_list_lock);
>>> -
>>> +err_unregister:
>>> ÂÂÂÂÂ device_unregister(&devfreq->dev);
>>> Â err_dev:
>>> ÂÂÂÂÂ if (devfreq)
>>> @@ -988,12 +1030,13 @@ static ssize_t governor_store(struct device
>>> *dev, struct device_attribute *attr,
>>> ÂÂÂÂÂ if (ret != 1)
>>> ÂÂÂÂÂÂÂÂÂ return -EINVAL;
>>> Â -ÂÂÂ mutex_lock(&devfreq_list_lock);
>>> -ÂÂÂ governor = find_devfreq_governor(str_governor);
>>> +ÂÂÂ governor = try_then_request_governor(str_governor);
>>> ÂÂÂÂÂ if (IS_ERR(governor)) {
>>> -ÂÂÂÂÂÂÂ ret = PTR_ERR(governor);
>>> -ÂÂÂÂÂÂÂ goto out;
>>> +ÂÂÂÂÂÂÂ return PTR_ERR(governor);
>>> ÂÂÂÂÂ }
>>> +
>>> +ÂÂÂ mutex_lock(&devfreq_list_lock);
>>> +
>>> ÂÂÂÂÂ if (df->governor == governor) {
>>> ÂÂÂÂÂÂÂÂÂ ret = 0;
>>> ÂÂÂÂÂÂÂÂÂ goto out;
>>> --Â
>>> 2.17.1
>>>
>>>
>>
>> Regards,
>> Eze
> 
> Adding to Ezequiel's point, shouldn't we take more granular lock (devfreq->lock)
> first and then call devfreq_list_lock at the time of adding to the list?
> 

Yes, I think so. I think, though, that this should be a separate patch, not sure
if a pre or post patch to this one, but for sure it's another topic. Current
patch tries to solve different problem an only tries to follow the current
locking/unlocking. Anyway this is a maintainer decision I guess.

Thanks,
 Enric

> -Akhil.
>