| Message ID | 20190415145505.18397-13-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | NVIDIA Tegra devfreq improvements and Tegra20/30 support | expand |
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; }
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.
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>
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 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(-)