Message ID | 20200724180857.22119-1-krzk@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [RFC] memory: exynos5422-dmc: Document mutex scope | expand |
Hi Krzysztof, On 7/24/20 7:08 PM, Krzysztof Kozlowski wrote: > Document scope of the mutex used by driver. > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > It seems mutex was introduced to protect: > 1. setting actual frequency/voltage, > 2. dmc->curr_rate (in exynos5_dmc_get_cur_freq()). > > However dmc->curr_rate in exynos5_dmc_get_status() is not protected. Is > it a bug? The callback get_dev_status() from devfreq->profile, which here is the exynos5_dmc_get_status() should be already called with devfreq->lock mutex hold, like e.g from simple_ondemand governor or directly using update_devfreq exported function: update_devfreq() ->get_target_freq() devfreq_update_stats() df->profile->get_dev_status() The dmc->curr_rate is also used from sysfs interface from devfreq. The local dmc lock serializes also this use case (when the HW freq has changed but not set yet into curr_rate. > --- > drivers/memory/samsung/exynos5422-dmc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c > index 93e9c2429c0d..0388066a7d96 100644 > --- a/drivers/memory/samsung/exynos5422-dmc.c > +++ b/drivers/memory/samsung/exynos5422-dmc.c > @@ -114,6 +114,7 @@ struct exynos5_dmc { > void __iomem *base_drexi0; > void __iomem *base_drexi1; > struct regmap *clk_regmap; > + /* Protects curr_rate and frequency/voltage setting section */ > struct mutex lock; > unsigned long curr_rate; > unsigned long curr_volt; > I assume this missing comment for the lock was required by some scripts. In this case LGTM: Reviewed-by: Lukasz Luba <lukasz.luba@arm.com> Regards, Lukasz
On Tue, Aug 04, 2020 at 11:40:07AM +0100, Lukasz Luba wrote: > Hi Krzysztof, > > On 7/24/20 7:08 PM, Krzysztof Kozlowski wrote: > > Document scope of the mutex used by driver. > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > > > --- > > > > It seems mutex was introduced to protect: > > 1. setting actual frequency/voltage, > > 2. dmc->curr_rate (in exynos5_dmc_get_cur_freq()). > > > > However dmc->curr_rate in exynos5_dmc_get_status() is not protected. Is > > it a bug? > > The callback get_dev_status() from devfreq->profile, which here is the > exynos5_dmc_get_status() should be already called with devfreq->lock > mutex hold, like e.g from simple_ondemand governor or directly > using update_devfreq exported function: > update_devfreq() > ->get_target_freq() > devfreq_update_stats() > df->profile->get_dev_status() > > The dmc->curr_rate is also used from sysfs interface from devfreq. > The local dmc lock serializes also this use case (when the HW freq > has changed but not set yet into curr_rate. These are different locks. You cannot protect dmc->curr_rate with devfreq->lock in one place and dmc-lock in other place. This won't protect it. > > --- > > drivers/memory/samsung/exynos5422-dmc.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c > > index 93e9c2429c0d..0388066a7d96 100644 > > --- a/drivers/memory/samsung/exynos5422-dmc.c > > +++ b/drivers/memory/samsung/exynos5422-dmc.c > > @@ -114,6 +114,7 @@ struct exynos5_dmc { > > void __iomem *base_drexi0; > > void __iomem *base_drexi1; > > struct regmap *clk_regmap; > > + /* Protects curr_rate and frequency/voltage setting section */ > > struct mutex lock; > > unsigned long curr_rate; > > unsigned long curr_volt; > > > > I assume this missing comment for the lock was required by some scripts. > In this case LGTM: > > Reviewed-by: Lukasz Luba <lukasz.luba@arm.com> Such comments are always useful. It is also pointed by strict checkpatch: CHECK: struct mutex definition without comment Best regards, Krzysztof
On 8/9/20 10:12 AM, Krzysztof Kozlowski wrote: > On Tue, Aug 04, 2020 at 11:40:07AM +0100, Lukasz Luba wrote: >> Hi Krzysztof, >> >> On 7/24/20 7:08 PM, Krzysztof Kozlowski wrote: >>> Document scope of the mutex used by driver. >>> >>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> >>> >>> --- >>> >>> It seems mutex was introduced to protect: >>> 1. setting actual frequency/voltage, >>> 2. dmc->curr_rate (in exynos5_dmc_get_cur_freq()). >>> >>> However dmc->curr_rate in exynos5_dmc_get_status() is not protected. Is >>> it a bug? >> >> The callback get_dev_status() from devfreq->profile, which here is the >> exynos5_dmc_get_status() should be already called with devfreq->lock >> mutex hold, like e.g from simple_ondemand governor or directly >> using update_devfreq exported function: >> update_devfreq() >> ->get_target_freq() >> devfreq_update_stats() >> df->profile->get_dev_status() >> >> The dmc->curr_rate is also used from sysfs interface from devfreq. >> The local dmc lock serializes also this use case (when the HW freq >> has changed but not set yet into curr_rate. > > These are different locks. You cannot protect dmc->curr_rate with > devfreq->lock in one place and dmc-lock in other place. This won't > protect it. There are different paths that framework goes and mainly they are protected by the df->lock. But I tend to agree, I will send a patch which adds some locking. Regards, Lukasz
diff --git a/drivers/memory/samsung/exynos5422-dmc.c b/drivers/memory/samsung/exynos5422-dmc.c index 93e9c2429c0d..0388066a7d96 100644 --- a/drivers/memory/samsung/exynos5422-dmc.c +++ b/drivers/memory/samsung/exynos5422-dmc.c @@ -114,6 +114,7 @@ struct exynos5_dmc { void __iomem *base_drexi0; void __iomem *base_drexi1; struct regmap *clk_regmap; + /* Protects curr_rate and frequency/voltage setting section */ struct mutex lock; unsigned long curr_rate; unsigned long curr_volt;
Document scope of the mutex used by driver. Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- It seems mutex was introduced to protect: 1. setting actual frequency/voltage, 2. dmc->curr_rate (in exynos5_dmc_get_cur_freq()). However dmc->curr_rate in exynos5_dmc_get_status() is not protected. Is it a bug? --- drivers/memory/samsung/exynos5422-dmc.c | 1 + 1 file changed, 1 insertion(+)