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

[Enric Balletbo i Serra <enric.balletbo@collabora.com>]

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@collabora.com>
>>> ---
>>>
>>> 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.
>