Patchwork [v2,12/19] PM / devfreq: tegra: Avoid inconsistency of current frequency value

login
register
mail settings
Submitter Dmitry Osipenko
Date April 15, 2019, 2:54 p.m.
Message ID <20190415145505.18397-13-digetx@gmail.com>
Download mbox | patch
Permalink /patch/773247/
State New
Headers show

Comments

Dmitry Osipenko - April 15, 2019, 2:54 p.m.
The frequency value potentially could change in-between. It doesn't
cause any real problem at all right now, but that could change in the
future. Hence let's avoid the inconsistency.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra-devfreq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Chanwoo Choi - April 16, 2019, 7:15 a.m.
Hi,

On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
> The frequency value potentially could change in-between. It doesn't
> cause any real problem at all right now, but that could change in the
> future. Hence let's avoid the inconsistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra-devfreq.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index a668e4fbc874..f1a6f951813a 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -496,13 +496,15 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>  {
>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>  	struct tegra_devfreq_device *actmon_dev;
> +	unsigned long cur_freq;
>  
> -	stat->current_frequency = tegra->cur_freq * KHZ;
> +	cur_freq = READ_ONCE(tegra->cur_freq);
>  
>  	/* To be used by the tegra governor */
>  	stat->private_data = tegra;
>  
>  	/* The below are to be used by the other governors */
> +	stat->current_frequency = cur_freq * KHZ;
>  
>  	actmon_dev = &tegra->devices[MCALL];
>  
> @@ -513,7 +515,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>  	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
>  
>  	/* Number of cycles in a sampling period */
> -	stat->total_time = ACTMON_SAMPLING_PERIOD * tegra->cur_freq;
> +	stat->total_time = ACTMON_SAMPLING_PERIOD * cur_freq;
>  
>  	stat->busy_time = min(stat->busy_time, stat->total_time);
>  
> 

The read/write access of tegra->cur_freq is in the single routine
of update_devfreq() as following. I think that there are no any
potential problem about the inconsistency of tegra->cur_freq.

IMHO, if there are no any problem now, I'm not sure that we need
to apply this patch.

update_devfreq()
{
	devfreq->governor->get_target_freq()
		devfreq_update_stats(devfreq)
			tegra_devfreq_get_dev_status()
				stat->current_frequency = tegra->cur_freq * KHZ;

	devfreq_set_target()
		tegra_devfreq_target()
			clk_set_min_rate(emc_rate, )
				tegra_actmon_rate_notify_cb()
					tegra->cur_freq = data->new_rate / KHZ;
		
			clk_set_rate(emc_rate, )
				tegra_actmon_rate_notify_cb()
					tegra->cur_freq = data->new_rate / KHZ;
}
Dmitry Osipenko - April 16, 2019, 3:40 p.m.
16.04.2019 10:15, Chanwoo Choi пишет:
> Hi,
> 
> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>> The frequency value potentially could change in-between. It doesn't
>> cause any real problem at all right now, but that could change in the
>> future. Hence let's avoid the inconsistency.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra-devfreq.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>> index a668e4fbc874..f1a6f951813a 100644
>> --- a/drivers/devfreq/tegra-devfreq.c
>> +++ b/drivers/devfreq/tegra-devfreq.c
>> @@ -496,13 +496,15 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>>  {
>>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>>  	struct tegra_devfreq_device *actmon_dev;
>> +	unsigned long cur_freq;
>>  
>> -	stat->current_frequency = tegra->cur_freq * KHZ;
>> +	cur_freq = READ_ONCE(tegra->cur_freq);
>>  
>>  	/* To be used by the tegra governor */
>>  	stat->private_data = tegra;
>>  
>>  	/* The below are to be used by the other governors */
>> +	stat->current_frequency = cur_freq * KHZ;
>>  
>>  	actmon_dev = &tegra->devices[MCALL];
>>  
>> @@ -513,7 +515,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>>  	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
>>  
>>  	/* Number of cycles in a sampling period */
>> -	stat->total_time = ACTMON_SAMPLING_PERIOD * tegra->cur_freq;
>> +	stat->total_time = ACTMON_SAMPLING_PERIOD * cur_freq;
>>  
>>  	stat->busy_time = min(stat->busy_time, stat->total_time);
>>  
>>
> 
> The read/write access of tegra->cur_freq is in the single routine
> of update_devfreq() as following. I think that there are no any
> potential problem about the inconsistency of tegra->cur_freq.

No, that's wrong assumption. The tegra->cur_freq is changed by the clock notifier that runs asynchronously with the devfreq driver when EMC clock rate is changed by something else in the kernel. 

> IMHO, if there are no any problem now, I'm not sure that we need
> to apply this patch.
> 
> update_devfreq()
> {
> 	devfreq->governor->get_target_freq()
> 		devfreq_update_stats(devfreq)
> 			tegra_devfreq_get_dev_status()
> 				stat->current_frequency = tegra->cur_freq * KHZ;
> 
> 	devfreq_set_target()
> 		tegra_devfreq_target()
> 			clk_set_min_rate(emc_rate, )
> 				tegra_actmon_rate_notify_cb()
> 					tegra->cur_freq = data->new_rate / KHZ;
> 		
> 			clk_set_rate(emc_rate, )
> 				tegra_actmon_rate_notify_cb()
> 					tegra->cur_freq = data->new_rate / KHZ;
> }
> 
> 

The cur_freq value is changed by the clock notifier that runs asynchronously with the rest of the devfreq driver. Hence potentially compiler may generate two separate fetches of the cur_freq value, then the clock rate could be changed by other CPU core simultaneously with tegra_devfreq_get_dev_status() or kernel may re-schedule preemptively, changing the clock rate in-between of the two fetches.
Chanwoo Choi - April 17, 2019, 12:35 a.m.
Hi,

On 19. 4. 17. 오전 12:40, Dmitry Osipenko wrote:
> 16.04.2019 10:15, Chanwoo Choi пишет:
>> Hi,
>>
>> On 19. 4. 15. 오후 11:54, Dmitry Osipenko wrote:
>>> The frequency value potentially could change in-between. It doesn't
>>> cause any real problem at all right now, but that could change in the
>>> future. Hence let's avoid the inconsistency.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/devfreq/tegra-devfreq.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
>>> index a668e4fbc874..f1a6f951813a 100644
>>> --- a/drivers/devfreq/tegra-devfreq.c
>>> +++ b/drivers/devfreq/tegra-devfreq.c
>>> @@ -496,13 +496,15 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>>>  {
>>>  	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
>>>  	struct tegra_devfreq_device *actmon_dev;
>>> +	unsigned long cur_freq;
>>>  
>>> -	stat->current_frequency = tegra->cur_freq * KHZ;
>>> +	cur_freq = READ_ONCE(tegra->cur_freq);
>>>  
>>>  	/* To be used by the tegra governor */
>>>  	stat->private_data = tegra;
>>>  
>>>  	/* The below are to be used by the other governors */
>>> +	stat->current_frequency = cur_freq * KHZ;
>>>  
>>>  	actmon_dev = &tegra->devices[MCALL];
>>>  
>>> @@ -513,7 +515,7 @@ static int tegra_devfreq_get_dev_status(struct device *dev,
>>>  	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
>>>  
>>>  	/* Number of cycles in a sampling period */
>>> -	stat->total_time = ACTMON_SAMPLING_PERIOD * tegra->cur_freq;
>>> +	stat->total_time = ACTMON_SAMPLING_PERIOD * cur_freq;
>>>  
>>>  	stat->busy_time = min(stat->busy_time, stat->total_time);
>>>  
>>>
>>
>> The read/write access of tegra->cur_freq is in the single routine
>> of update_devfreq() as following. I think that there are no any
>> potential problem about the inconsistency of tegra->cur_freq.
> 
> No, that's wrong assumption. The tegra->cur_freq is changed by the clock notifier that runs asynchronously with the devfreq driver when EMC clock rate is changed by something else in the kernel. 
> 
>> IMHO, if there are no any problem now, I'm not sure that we need
>> to apply this patch.
>>
>> update_devfreq()
>> {
>> 	devfreq->governor->get_target_freq()
>> 		devfreq_update_stats(devfreq)
>> 			tegra_devfreq_get_dev_status()
>> 				stat->current_frequency = tegra->cur_freq * KHZ;
>>
>> 	devfreq_set_target()
>> 		tegra_devfreq_target()
>> 			clk_set_min_rate(emc_rate, )
>> 				tegra_actmon_rate_notify_cb()
>> 					tegra->cur_freq = data->new_rate / KHZ;
>> 		
>> 			clk_set_rate(emc_rate, )
>> 				tegra_actmon_rate_notify_cb()
>> 					tegra->cur_freq = data->new_rate / KHZ;
>> }
>>
>>
> 
> The cur_freq value is changed by the clock notifier that runs asynchronously with the rest of the devfreq driver. Hence potentially compiler may generate two separate fetches of the cur_freq value, then the clock rate could be changed by other CPU core simultaneously with tegra_devfreq_get_dev_status() or kernel may re-schedule preemptively, changing the clock rate in-between of the two fetches.
> 
> 

Thanks. I understand why have to consider the inconsistency of clock
which is used on the multiple points.

Looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Patch

diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index a668e4fbc874..f1a6f951813a 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -496,13 +496,15 @@  static int tegra_devfreq_get_dev_status(struct device *dev,
 {
 	struct tegra_devfreq *tegra = dev_get_drvdata(dev);
 	struct tegra_devfreq_device *actmon_dev;
+	unsigned long cur_freq;
 
-	stat->current_frequency = tegra->cur_freq * KHZ;
+	cur_freq = READ_ONCE(tegra->cur_freq);
 
 	/* To be used by the tegra governor */
 	stat->private_data = tegra;
 
 	/* The below are to be used by the other governors */
+	stat->current_frequency = cur_freq * KHZ;
 
 	actmon_dev = &tegra->devices[MCALL];
 
@@ -513,7 +515,7 @@  static int tegra_devfreq_get_dev_status(struct device *dev,
 	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
 
 	/* Number of cycles in a sampling period */
-	stat->total_time = ACTMON_SAMPLING_PERIOD * tegra->cur_freq;
+	stat->total_time = ACTMON_SAMPLING_PERIOD * cur_freq;
 
 	stat->busy_time = min(stat->busy_time, stat->total_time);