Patchwork [v2,1/2] drivers: devfreq: change devfreq workqueue mechanism

login
register
mail settings
Submitter Lukasz Luba
Date Feb. 11, 2019, 3:30 p.m.
Message ID <1549899005-7760-2-git-send-email-l.luba@partner.samsung.com>
Download mbox | patch
Permalink /patch/723055/
State New
Headers show

Comments

Lukasz Luba - Feb. 11, 2019, 3:30 p.m.
There is no need for creating another workqueue in the system,
the existing one should meet the requirements.
This patch removes devfreq's custom workqueue and uses system one.
It switches from queue_delayed_work() to schedule_delayed_work().
It also does not wake up the system when it enters suspend (this
functionality stays the same).

Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 drivers/devfreq/devfreq.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)
Matthias Kaehlcke - Feb. 11, 2019, 9:42 p.m.
Hi Lukasz,

On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote:
> There is no need for creating another workqueue in the system,
> the existing one should meet the requirements.
> This patch removes devfreq's custom workqueue and uses system one.
> It switches from queue_delayed_work() to schedule_delayed_work().
> It also does not wake up the system when it enters suspend (this
> functionality stays the same).
> 
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 0ae3de7..882e717 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -31,13 +31,6 @@
>  
>  static struct class *devfreq_class;
>  
> -/*
> - * devfreq core provides delayed work based load monitoring helper
> - * functions. Governors can use these or can implement their own
> - * monitoring mechanism.
> - */
> -static struct workqueue_struct *devfreq_wq;
> -
>  /* The list of all device-devfreq governors */
>  static LIST_HEAD(devfreq_governor_list);
>  /* The list of all device-devfreq */
> @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work)
>  	if (err)
>  		dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err);
>  
> -	queue_delayed_work(devfreq_wq, &devfreq->work,
> -				msecs_to_jiffies(devfreq->profile->polling_ms));
> +	schedule_delayed_work(&devfreq->work,
> +			      msecs_to_jiffies(devfreq->profile->polling_ms));
>  	mutex_unlock(&devfreq->lock);
>  }
>  
> @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq)
>  {
>  	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
>  	if (devfreq->profile->polling_ms)
> -		queue_delayed_work(devfreq_wq, &devfreq->work,
> +		schedule_delayed_work(&devfreq->work,
>  			msecs_to_jiffies(devfreq->profile->polling_ms));
>  }
>  EXPORT_SYMBOL(devfreq_monitor_start);
> @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>  
>  	if (!delayed_work_pending(&devfreq->work) &&
>  			devfreq->profile->polling_ms)
> -		queue_delayed_work(devfreq_wq, &devfreq->work,
> +		schedule_delayed_work(&devfreq->work,
>  			msecs_to_jiffies(devfreq->profile->polling_ms));
>  
>  	devfreq->last_stat_updated = jiffies;
> @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
>  
>  	/* if current delay is zero, start polling with new delay */
>  	if (!cur_delay) {
> -		queue_delayed_work(devfreq_wq, &devfreq->work,
> +		schedule_delayed_work(&devfreq->work,
>  			msecs_to_jiffies(devfreq->profile->polling_ms));
>  		goto out;
>  	}
> @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
>  		cancel_delayed_work_sync(&devfreq->work);
>  		mutex_lock(&devfreq->lock);
>  		if (!devfreq->stop_polling)
> -			queue_delayed_work(devfreq_wq, &devfreq->work,
> +			schedule_delayed_work(&devfreq->work,
>  			      msecs_to_jiffies(devfreq->profile->polling_ms));
>  	}
>  out:
> @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void)
>  		return PTR_ERR(devfreq_class);
>  	}
>  
> -	devfreq_wq = create_freezable_workqueue("devfreq_wq");
> -	if (!devfreq_wq) {
> -		class_destroy(devfreq_class);
> -		pr_err("%s: couldn't create workqueue\n", __FILE__);
> -		return -ENOMEM;
> -	}
>  	devfreq_class->dev_groups = devfreq_groups;
>  
>  	return 0;

As commented on v1, the change from a custom to a system workqueue
seems reasonable to me. However this patch also changes from a
freezable workqueue to a non-freezable one. C&P of my comments on v1:

``WQ_FREEZABLE``
    A freezable wq participates in the freeze phase of the system
    suspend operations.  Work items on the wq are drained and no
    new work item starts execution until thawed.

I'm not entirely sure what the impact of this is.

I imagine suspend is potentially quicker because the wq isn't drained,
but could works that execute during the suspend phase be a problem?

Cheers

Matthias
Lukasz Luba - Feb. 12, 2019, 11:20 a.m.
Hi Matthias,

On 2/11/19 10:42 PM, Matthias Kaehlcke wrote:
> Hi Lukasz,
> 
> On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote:
>> There is no need for creating another workqueue in the system,
>> the existing one should meet the requirements.
>> This patch removes devfreq's custom workqueue and uses system one.
>> It switches from queue_delayed_work() to schedule_delayed_work().
>> It also does not wake up the system when it enters suspend (this
>> functionality stays the same).
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   drivers/devfreq/devfreq.c | 25 ++++++-------------------
>>   1 file changed, 6 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 0ae3de7..882e717 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -31,13 +31,6 @@
>>   
>>   static struct class *devfreq_class;
>>   
>> -/*
>> - * devfreq core provides delayed work based load monitoring helper
>> - * functions. Governors can use these or can implement their own
>> - * monitoring mechanism.
>> - */
>> -static struct workqueue_struct *devfreq_wq;
>> -
>>   /* The list of all device-devfreq governors */
>>   static LIST_HEAD(devfreq_governor_list);
>>   /* The list of all device-devfreq */
>> @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work)
>>   	if (err)
>>   		dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err);
>>   
>> -	queue_delayed_work(devfreq_wq, &devfreq->work,
>> -				msecs_to_jiffies(devfreq->profile->polling_ms));
>> +	schedule_delayed_work(&devfreq->work,
>> +			      msecs_to_jiffies(devfreq->profile->polling_ms));
>>   	mutex_unlock(&devfreq->lock);
>>   }
>>   
>> @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq)
>>   {
>>   	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
>>   	if (devfreq->profile->polling_ms)
>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
>> +		schedule_delayed_work(&devfreq->work,
>>   			msecs_to_jiffies(devfreq->profile->polling_ms));
>>   }
>>   EXPORT_SYMBOL(devfreq_monitor_start);
>> @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>>   
>>   	if (!delayed_work_pending(&devfreq->work) &&
>>   			devfreq->profile->polling_ms)
>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
>> +		schedule_delayed_work(&devfreq->work,
>>   			msecs_to_jiffies(devfreq->profile->polling_ms));
>>   
>>   	devfreq->last_stat_updated = jiffies;
>> @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
>>   
>>   	/* if current delay is zero, start polling with new delay */
>>   	if (!cur_delay) {
>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
>> +		schedule_delayed_work(&devfreq->work,
>>   			msecs_to_jiffies(devfreq->profile->polling_ms));
>>   		goto out;
>>   	}
>> @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
>>   		cancel_delayed_work_sync(&devfreq->work);
>>   		mutex_lock(&devfreq->lock);
>>   		if (!devfreq->stop_polling)
>> -			queue_delayed_work(devfreq_wq, &devfreq->work,
>> +			schedule_delayed_work(&devfreq->work,
>>   			      msecs_to_jiffies(devfreq->profile->polling_ms));
>>   	}
>>   out:
>> @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void)
>>   		return PTR_ERR(devfreq_class);
>>   	}
>>   
>> -	devfreq_wq = create_freezable_workqueue("devfreq_wq");
>> -	if (!devfreq_wq) {
>> -		class_destroy(devfreq_class);
>> -		pr_err("%s: couldn't create workqueue\n", __FILE__);
>> -		return -ENOMEM;
>> -	}
>>   	devfreq_class->dev_groups = devfreq_groups;
>>   
>>   	return 0;
> 
> As commented on v1, the change from a custom to a system workqueue
> seems reasonable to me. However this patch also changes from a
> freezable workqueue to a non-freezable one. C&P of my comments on v1:
> 
> ``WQ_FREEZABLE``
>      A freezable wq participates in the freeze phase of the system
>      suspend operations.  Work items on the wq are drained and no
>      new work item starts execution until thawed.
> 
> I'm not entirely sure what the impact of this is.
> 
> I imagine suspend is potentially quicker because the wq isn't drained,
> but could works that execute during the suspend phase be a problem?
The devfreq supports suspend from v4.20-rc6, which picks OPP for a
device based on its DT 'opp-suspend'. For the devices which do not
choose the suspend OPP it is possible to enter that state with any
frequency. Queuing work for calling governor during suspend which
calculates the device's frequency for the next period is IMO not needed,
The 'next period' is actually suspend and is not related to
'predicted' load by the governor.

You are right, the suspend will be faster.

Regards,
Lukasz
> 
> Cheers
> 
> Matthias
> 
>
Matthias Kaehlcke - Feb. 12, 2019, 8:12 p.m.
On Tue, Feb 12, 2019 at 12:20:42PM +0100, Lukasz Luba wrote:
> Hi Matthias,
> 
> On 2/11/19 10:42 PM, Matthias Kaehlcke wrote:
> > Hi Lukasz,
> > 
> > On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote:
> >> There is no need for creating another workqueue in the system,
> >> the existing one should meet the requirements.
> >> This patch removes devfreq's custom workqueue and uses system one.
> >> It switches from queue_delayed_work() to schedule_delayed_work().
> >> It also does not wake up the system when it enters suspend (this
> >> functionality stays the same).
> >>
> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> >> ---
> >>   drivers/devfreq/devfreq.c | 25 ++++++-------------------
> >>   1 file changed, 6 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >> index 0ae3de7..882e717 100644
> >> --- a/drivers/devfreq/devfreq.c
> >> +++ b/drivers/devfreq/devfreq.c
> >> @@ -31,13 +31,6 @@
> >>   
> >>   static struct class *devfreq_class;
> >>   
> >> -/*
> >> - * devfreq core provides delayed work based load monitoring helper
> >> - * functions. Governors can use these or can implement their own
> >> - * monitoring mechanism.
> >> - */
> >> -static struct workqueue_struct *devfreq_wq;
> >> -
> >>   /* The list of all device-devfreq governors */
> >>   static LIST_HEAD(devfreq_governor_list);
> >>   /* The list of all device-devfreq */
> >> @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work)
> >>   	if (err)
> >>   		dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err);
> >>   
> >> -	queue_delayed_work(devfreq_wq, &devfreq->work,
> >> -				msecs_to_jiffies(devfreq->profile->polling_ms));
> >> +	schedule_delayed_work(&devfreq->work,
> >> +			      msecs_to_jiffies(devfreq->profile->polling_ms));
> >>   	mutex_unlock(&devfreq->lock);
> >>   }
> >>   
> >> @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq)
> >>   {
> >>   	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
> >>   	if (devfreq->profile->polling_ms)
> >> -		queue_delayed_work(devfreq_wq, &devfreq->work,
> >> +		schedule_delayed_work(&devfreq->work,
> >>   			msecs_to_jiffies(devfreq->profile->polling_ms));
> >>   }
> >>   EXPORT_SYMBOL(devfreq_monitor_start);
> >> @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
> >>   
> >>   	if (!delayed_work_pending(&devfreq->work) &&
> >>   			devfreq->profile->polling_ms)
> >> -		queue_delayed_work(devfreq_wq, &devfreq->work,
> >> +		schedule_delayed_work(&devfreq->work,
> >>   			msecs_to_jiffies(devfreq->profile->polling_ms));
> >>   
> >>   	devfreq->last_stat_updated = jiffies;
> >> @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
> >>   
> >>   	/* if current delay is zero, start polling with new delay */
> >>   	if (!cur_delay) {
> >> -		queue_delayed_work(devfreq_wq, &devfreq->work,
> >> +		schedule_delayed_work(&devfreq->work,
> >>   			msecs_to_jiffies(devfreq->profile->polling_ms));
> >>   		goto out;
> >>   	}
> >> @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
> >>   		cancel_delayed_work_sync(&devfreq->work);
> >>   		mutex_lock(&devfreq->lock);
> >>   		if (!devfreq->stop_polling)
> >> -			queue_delayed_work(devfreq_wq, &devfreq->work,
> >> +			schedule_delayed_work(&devfreq->work,
> >>   			      msecs_to_jiffies(devfreq->profile->polling_ms));
> >>   	}
> >>   out:
> >> @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void)
> >>   		return PTR_ERR(devfreq_class);
> >>   	}
> >>   
> >> -	devfreq_wq = create_freezable_workqueue("devfreq_wq");
> >> -	if (!devfreq_wq) {
> >> -		class_destroy(devfreq_class);
> >> -		pr_err("%s: couldn't create workqueue\n", __FILE__);
> >> -		return -ENOMEM;
> >> -	}
> >>   	devfreq_class->dev_groups = devfreq_groups;
> >>   
> >>   	return 0;
> > 
> > As commented on v1, the change from a custom to a system workqueue
> > seems reasonable to me. However this patch also changes from a
> > freezable workqueue to a non-freezable one. C&P of my comments on v1:
> > 
> > ``WQ_FREEZABLE``
> >      A freezable wq participates in the freeze phase of the system
> >      suspend operations.  Work items on the wq are drained and no
> >      new work item starts execution until thawed.
> > 
> > I'm not entirely sure what the impact of this is.
> > 
> > I imagine suspend is potentially quicker because the wq isn't drained,
> > but could works that execute during the suspend phase be a problem?
> The devfreq supports suspend from v4.20-rc6, which picks OPP for a
> device based on its DT 'opp-suspend'. For the devices which do not
> choose the suspend OPP it is possible to enter that state with any
> frequency. Queuing work for calling governor during suspend which
> calculates the device's frequency for the next period is IMO not needed,
> The 'next period' is actually suspend and is not related to
> 'predicted' load by the governor.

If I am not mistaken the monitor can still be running after a device
was suspended:

devfreq_suspend
  list_for_each_entry(devfreq, &devfreq_list, node)
    devfreq_suspend_device
      devfreq->governor->event_handler(devfreq,
                                       DEVFREQ_GOV_SUSPEND, NULL);

According to the comment of devfreq_monitor_suspend() the function is
supposed to be called by the governor in response to
DEVFREQ_GOV_SUSPEND, however this doesn't seem to be universally the case:

git grep devfreq_monitor_suspend
  drivers/devfreq/governor_simpleondemand.c:              devfreq_monitor_suspend(devfreq);
  drivers/devfreq/tegra-devfreq.c:                devfreq_monitor_suspend(devfreq);

i.e. the other governors don't seem to call devfreq_monitor_suspend().

Am I missing something?

Thanks

Matthias
Lukasz Luba - Feb. 12, 2019, 9:37 p.m.
Hi Matthias,

On 2/12/19 9:12 PM, Matthias Kaehlcke wrote:
> On Tue, Feb 12, 2019 at 12:20:42PM +0100, Lukasz Luba wrote:
>> Hi Matthias,
>>
>> On 2/11/19 10:42 PM, Matthias Kaehlcke wrote:
>>> Hi Lukasz,
>>>
>>> On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote:
>>>> There is no need for creating another workqueue in the system,
>>>> the existing one should meet the requirements.
>>>> This patch removes devfreq's custom workqueue and uses system one.
>>>> It switches from queue_delayed_work() to schedule_delayed_work().
>>>> It also does not wake up the system when it enters suspend (this
>>>> functionality stays the same).
>>>>
>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>>> ---
>>>>    drivers/devfreq/devfreq.c | 25 ++++++-------------------
>>>>    1 file changed, 6 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index 0ae3de7..882e717 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -31,13 +31,6 @@
>>>>    
>>>>    static struct class *devfreq_class;
>>>>    
>>>> -/*
>>>> - * devfreq core provides delayed work based load monitoring helper
>>>> - * functions. Governors can use these or can implement their own
>>>> - * monitoring mechanism.
>>>> - */
>>>> -static struct workqueue_struct *devfreq_wq;
>>>> -
>>>>    /* The list of all device-devfreq governors */
>>>>    static LIST_HEAD(devfreq_governor_list);
>>>>    /* The list of all device-devfreq */
>>>> @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work)
>>>>    	if (err)
>>>>    		dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err);
>>>>    
>>>> -	queue_delayed_work(devfreq_wq, &devfreq->work,
>>>> -				msecs_to_jiffies(devfreq->profile->polling_ms));
>>>> +	schedule_delayed_work(&devfreq->work,
>>>> +			      msecs_to_jiffies(devfreq->profile->polling_ms));
>>>>    	mutex_unlock(&devfreq->lock);
>>>>    }
>>>>    
>>>> @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq)
>>>>    {
>>>>    	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
>>>>    	if (devfreq->profile->polling_ms)
>>>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
>>>> +		schedule_delayed_work(&devfreq->work,
>>>>    			msecs_to_jiffies(devfreq->profile->polling_ms));
>>>>    }
>>>>    EXPORT_SYMBOL(devfreq_monitor_start);
>>>> @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>>>>    
>>>>    	if (!delayed_work_pending(&devfreq->work) &&
>>>>    			devfreq->profile->polling_ms)
>>>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
>>>> +		schedule_delayed_work(&devfreq->work,
>>>>    			msecs_to_jiffies(devfreq->profile->polling_ms));
>>>>    
>>>>    	devfreq->last_stat_updated = jiffies;
>>>> @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
>>>>    
>>>>    	/* if current delay is zero, start polling with new delay */
>>>>    	if (!cur_delay) {
>>>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
>>>> +		schedule_delayed_work(&devfreq->work,
>>>>    			msecs_to_jiffies(devfreq->profile->polling_ms));
>>>>    		goto out;
>>>>    	}
>>>> @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
>>>>    		cancel_delayed_work_sync(&devfreq->work);
>>>>    		mutex_lock(&devfreq->lock);
>>>>    		if (!devfreq->stop_polling)
>>>> -			queue_delayed_work(devfreq_wq, &devfreq->work,
>>>> +			schedule_delayed_work(&devfreq->work,
>>>>    			      msecs_to_jiffies(devfreq->profile->polling_ms));
>>>>    	}
>>>>    out:
>>>> @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void)
>>>>    		return PTR_ERR(devfreq_class);
>>>>    	}
>>>>    
>>>> -	devfreq_wq = create_freezable_workqueue("devfreq_wq");
>>>> -	if (!devfreq_wq) {
>>>> -		class_destroy(devfreq_class);
>>>> -		pr_err("%s: couldn't create workqueue\n", __FILE__);
>>>> -		return -ENOMEM;
>>>> -	}
>>>>    	devfreq_class->dev_groups = devfreq_groups;
>>>>    
>>>>    	return 0;
>>>
>>> As commented on v1, the change from a custom to a system workqueue
>>> seems reasonable to me. However this patch also changes from a
>>> freezable workqueue to a non-freezable one. C&P of my comments on v1:
>>>
>>> ``WQ_FREEZABLE``
>>>       A freezable wq participates in the freeze phase of the system
>>>       suspend operations.  Work items on the wq are drained and no
>>>       new work item starts execution until thawed.
>>>
>>> I'm not entirely sure what the impact of this is.
>>>
>>> I imagine suspend is potentially quicker because the wq isn't drained,
>>> but could works that execute during the suspend phase be a problem?
>> The devfreq supports suspend from v4.20-rc6, which picks OPP for a
>> device based on its DT 'opp-suspend'. For the devices which do not
>> choose the suspend OPP it is possible to enter that state with any
>> frequency. Queuing work for calling governor during suspend which
>> calculates the device's frequency for the next period is IMO not needed,
>> The 'next period' is actually suspend and is not related to
>> 'predicted' load by the governor.
> 
> If I am not mistaken the monitor can still be running after a device
> was suspended:
> 
> devfreq_suspend
>    list_for_each_entry(devfreq, &devfreq_list, node)
>      devfreq_suspend_device
>        devfreq->governor->event_handler(devfreq,
>                                         DEVFREQ_GOV_SUSPEND, NULL);
> 
> According to the comment of devfreq_monitor_suspend() the function is
> supposed to be called by the governor in response to
> DEVFREQ_GOV_SUSPEND, however this doesn't seem to be universally the case:
> 
> git grep devfreq_monitor_suspend
>    drivers/devfreq/governor_simpleondemand.c:              devfreq_monitor_suspend(devfreq);
>    drivers/devfreq/tegra-devfreq.c:                devfreq_monitor_suspend(devfreq);
> 
> i.e. the other governors don't seem to call devfreq_monitor_suspend().
> 
> Am I missing something?
Probably not.
Good catch, these governors should support case DEVFREQ_GOV_SUSPEND.
The system suspend which calls 'devfreq_suspend' does it when the
workqueues are frozen and sets the desired OPP for later resume.
The other use use cases (like pm_suspend) might assume that these
governors are ready for DEVFREQ_GOV_SUSPEND...
Do you like to write a patch for them (I can test it) or should I do it?

Regards,
Lukasz
> 
> Thanks
> 
> Matthias
> 
>
Matthias Kaehlcke - Feb. 13, 2019, 12:48 a.m.
Hi Lukasz,

On Tue, Feb 12, 2019 at 10:37:20PM +0100, Lukasz Luba wrote:
> Hi Matthias,
> 
> On 2/12/19 9:12 PM, Matthias Kaehlcke wrote:
> > On Tue, Feb 12, 2019 at 12:20:42PM +0100, Lukasz Luba wrote:
> >> Hi Matthias,
> >>
> >> On 2/11/19 10:42 PM, Matthias Kaehlcke wrote:
> >>> Hi Lukasz,
> >>>
> >>> On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote:
> >>>> There is no need for creating another workqueue in the system,
> >>>> the existing one should meet the requirements.
> >>>> This patch removes devfreq's custom workqueue and uses system one.
> >>>> It switches from queue_delayed_work() to schedule_delayed_work().
> >>>> It also does not wake up the system when it enters suspend (this
> >>>> functionality stays the same).
> >>>>
> >>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> >>>> ---
> >>>>    drivers/devfreq/devfreq.c | 25 ++++++-------------------
> >>>>    1 file changed, 6 insertions(+), 19 deletions(-)
> >>>>
> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >>>> index 0ae3de7..882e717 100644
> >>>> --- a/drivers/devfreq/devfreq.c
> >>>> +++ b/drivers/devfreq/devfreq.c
> >>>> @@ -31,13 +31,6 @@
> >>>>    
> >>>>    static struct class *devfreq_class;
> >>>>    
> >>>> -/*
> >>>> - * devfreq core provides delayed work based load monitoring helper
> >>>> - * functions. Governors can use these or can implement their own
> >>>> - * monitoring mechanism.
> >>>> - */
> >>>> -static struct workqueue_struct *devfreq_wq;
> >>>> -
> >>>>    /* The list of all device-devfreq governors */
> >>>>    static LIST_HEAD(devfreq_governor_list);
> >>>>    /* The list of all device-devfreq */
> >>>> @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work)
> >>>>    	if (err)
> >>>>    		dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err);
> >>>>    
> >>>> -	queue_delayed_work(devfreq_wq, &devfreq->work,
> >>>> -				msecs_to_jiffies(devfreq->profile->polling_ms));
> >>>> +	schedule_delayed_work(&devfreq->work,
> >>>> +			      msecs_to_jiffies(devfreq->profile->polling_ms));
> >>>>    	mutex_unlock(&devfreq->lock);
> >>>>    }
> >>>>    
> >>>> @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq)
> >>>>    {
> >>>>    	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
> >>>>    	if (devfreq->profile->polling_ms)
> >>>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
> >>>> +		schedule_delayed_work(&devfreq->work,
> >>>>    			msecs_to_jiffies(devfreq->profile->polling_ms));
> >>>>    }
> >>>>    EXPORT_SYMBOL(devfreq_monitor_start);
> >>>> @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
> >>>>    
> >>>>    	if (!delayed_work_pending(&devfreq->work) &&
> >>>>    			devfreq->profile->polling_ms)
> >>>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
> >>>> +		schedule_delayed_work(&devfreq->work,
> >>>>    			msecs_to_jiffies(devfreq->profile->polling_ms));
> >>>>    
> >>>>    	devfreq->last_stat_updated = jiffies;
> >>>> @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
> >>>>    
> >>>>    	/* if current delay is zero, start polling with new delay */
> >>>>    	if (!cur_delay) {
> >>>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
> >>>> +		schedule_delayed_work(&devfreq->work,
> >>>>    			msecs_to_jiffies(devfreq->profile->polling_ms));
> >>>>    		goto out;
> >>>>    	}
> >>>> @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
> >>>>    		cancel_delayed_work_sync(&devfreq->work);
> >>>>    		mutex_lock(&devfreq->lock);
> >>>>    		if (!devfreq->stop_polling)
> >>>> -			queue_delayed_work(devfreq_wq, &devfreq->work,
> >>>> +			schedule_delayed_work(&devfreq->work,
> >>>>    			      msecs_to_jiffies(devfreq->profile->polling_ms));
> >>>>    	}
> >>>>    out:
> >>>> @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void)
> >>>>    		return PTR_ERR(devfreq_class);
> >>>>    	}
> >>>>    
> >>>> -	devfreq_wq = create_freezable_workqueue("devfreq_wq");
> >>>> -	if (!devfreq_wq) {
> >>>> -		class_destroy(devfreq_class);
> >>>> -		pr_err("%s: couldn't create workqueue\n", __FILE__);
> >>>> -		return -ENOMEM;
> >>>> -	}
> >>>>    	devfreq_class->dev_groups = devfreq_groups;
> >>>>    
> >>>>    	return 0;
> >>>
> >>> As commented on v1, the change from a custom to a system workqueue
> >>> seems reasonable to me. However this patch also changes from a
> >>> freezable workqueue to a non-freezable one. C&P of my comments on v1:
> >>>
> >>> ``WQ_FREEZABLE``
> >>>       A freezable wq participates in the freeze phase of the system
> >>>       suspend operations.  Work items on the wq are drained and no
> >>>       new work item starts execution until thawed.
> >>>
> >>> I'm not entirely sure what the impact of this is.
> >>>
> >>> I imagine suspend is potentially quicker because the wq isn't drained,
> >>> but could works that execute during the suspend phase be a problem?
> >> The devfreq supports suspend from v4.20-rc6, which picks OPP for a
> >> device based on its DT 'opp-suspend'. For the devices which do not
> >> choose the suspend OPP it is possible to enter that state with any
> >> frequency. Queuing work for calling governor during suspend which
> >> calculates the device's frequency for the next period is IMO not needed,
> >> The 'next period' is actually suspend and is not related to
> >> 'predicted' load by the governor.
> > 
> > If I am not mistaken the monitor can still be running after a device
> > was suspended:
> > 
> > devfreq_suspend
> >    list_for_each_entry(devfreq, &devfreq_list, node)
> >      devfreq_suspend_device
> >        devfreq->governor->event_handler(devfreq,
> >                                         DEVFREQ_GOV_SUSPEND, NULL);
> > 
> > According to the comment of devfreq_monitor_suspend() the function is
> > supposed to be called by the governor in response to
> > DEVFREQ_GOV_SUSPEND, however this doesn't seem to be universally the case:
> > 
> > git grep devfreq_monitor_suspend
> >    drivers/devfreq/governor_simpleondemand.c:              devfreq_monitor_suspend(devfreq);
> >    drivers/devfreq/tegra-devfreq.c:                devfreq_monitor_suspend(devfreq);
> > 
> > i.e. the other governors don't seem to call devfreq_monitor_suspend().
> > 
> > Am I missing something?
> Probably not.
> Good catch, these governors should support case DEVFREQ_GOV_SUSPEND.
> The system suspend which calls 'devfreq_suspend' does it when the
> workqueues are frozen and sets the desired OPP for later resume.
> The other use use cases (like pm_suspend) might assume that these
> governors are ready for DEVFREQ_GOV_SUSPEND...

Thanks for the confirmation!

> Do you like to write a patch for them (I can test it) or should I do it?

I can send a patch, testing will be appreciated :)

Thanks

Matthias

Patch

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 0ae3de7..882e717 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -31,13 +31,6 @@ 
 
 static struct class *devfreq_class;
 
-/*
- * devfreq core provides delayed work based load monitoring helper
- * functions. Governors can use these or can implement their own
- * monitoring mechanism.
- */
-static struct workqueue_struct *devfreq_wq;
-
 /* The list of all device-devfreq governors */
 static LIST_HEAD(devfreq_governor_list);
 /* The list of all device-devfreq */
@@ -391,8 +384,8 @@  static void devfreq_monitor(struct work_struct *work)
 	if (err)
 		dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err);
 
-	queue_delayed_work(devfreq_wq, &devfreq->work,
-				msecs_to_jiffies(devfreq->profile->polling_ms));
+	schedule_delayed_work(&devfreq->work,
+			      msecs_to_jiffies(devfreq->profile->polling_ms));
 	mutex_unlock(&devfreq->lock);
 }
 
@@ -409,7 +402,7 @@  void devfreq_monitor_start(struct devfreq *devfreq)
 {
 	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
 	if (devfreq->profile->polling_ms)
-		queue_delayed_work(devfreq_wq, &devfreq->work,
+		schedule_delayed_work(&devfreq->work,
 			msecs_to_jiffies(devfreq->profile->polling_ms));
 }
 EXPORT_SYMBOL(devfreq_monitor_start);
@@ -473,7 +466,7 @@  void devfreq_monitor_resume(struct devfreq *devfreq)
 
 	if (!delayed_work_pending(&devfreq->work) &&
 			devfreq->profile->polling_ms)
-		queue_delayed_work(devfreq_wq, &devfreq->work,
+		schedule_delayed_work(&devfreq->work,
 			msecs_to_jiffies(devfreq->profile->polling_ms));
 
 	devfreq->last_stat_updated = jiffies;
@@ -516,7 +509,7 @@  void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
 
 	/* if current delay is zero, start polling with new delay */
 	if (!cur_delay) {
-		queue_delayed_work(devfreq_wq, &devfreq->work,
+		schedule_delayed_work(&devfreq->work,
 			msecs_to_jiffies(devfreq->profile->polling_ms));
 		goto out;
 	}
@@ -527,7 +520,7 @@  void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
 		cancel_delayed_work_sync(&devfreq->work);
 		mutex_lock(&devfreq->lock);
 		if (!devfreq->stop_polling)
-			queue_delayed_work(devfreq_wq, &devfreq->work,
+			schedule_delayed_work(&devfreq->work,
 			      msecs_to_jiffies(devfreq->profile->polling_ms));
 	}
 out:
@@ -1430,12 +1423,6 @@  static int __init devfreq_init(void)
 		return PTR_ERR(devfreq_class);
 	}
 
-	devfreq_wq = create_freezable_workqueue("devfreq_wq");
-	if (!devfreq_wq) {
-		class_destroy(devfreq_class);
-		pr_err("%s: couldn't create workqueue\n", __FILE__);
-		return -ENOMEM;
-	}
 	devfreq_class->dev_groups = devfreq_groups;
 
 	return 0;