| Message ID | 20170612151748.7380-4-krzk@kernel.org (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
On 12 June 2017 at 17:17, Krzysztof Kozlowski <krzk@kernel.org> wrote: > Add lockdep checks for holding mutex protecting the list of domains. > This might expose misuse even though only file-scope functions use it > for now. I think it seems a bit silly to use these lockdep checks as these functions are as you state above, static functions. Moreover there are called from a quite limited amount of places. Do you really think this add some value? Kind regards Uffe > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > --- > drivers/base/power/domain.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 983f086ab235..9d3d3c2a5979 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -726,6 +726,8 @@ static bool pm_genpd_present(const struct generic_pm_domain *genpd) > { > const struct generic_pm_domain *gpd; > > + lockdep_assert_held(&gpd_list_lock); > + > if (IS_ERR_OR_NULL(genpd)) > return false; > > @@ -1321,6 +1323,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd, > struct gpd_link *link, *itr; > int ret = 0; > > + lockdep_assert_held(&gpd_list_lock); > + > if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain) > || genpd == subdomain) > return -EINVAL; > @@ -1528,6 +1532,8 @@ static int genpd_remove(struct generic_pm_domain *genpd) > { > struct gpd_link *l, *link; > > + lockdep_assert_held(&gpd_list_lock); > + > if (IS_ERR_OR_NULL(genpd)) > return -EINVAL; > > -- > 2.9.3 >
On Mon, Jun 12, 2017 at 09:09:59PM +0200, Ulf Hansson wrote: > On 12 June 2017 at 17:17, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > Add lockdep checks for holding mutex protecting the list of domains. > > This might expose misuse even though only file-scope functions use it > > for now. > > I think it seems a bit silly to use these lockdep checks as these > functions are as you state above, static functions. Moreover there are > called from a quite limited amount of places. > > Do you really think this add some value? In ideal world, these would not need lockdeps because we do not make mistakes. I agree, that mostly the exposed functions to other kernel modules should be protected, as lockdep annotation is a more advanced way of documenting the need of locking. Does it mean that file-scope functions should not be annotated? Even in such case function can be misused as the code is getting more and more complicated. What is best example, one of these calls (pm_genpd_present()) was already used in wrong way, without locking when iterating over the list. Having lockdep would point this early. Best regards, Krzysztof > > Kind regards > Uffe > > > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > > --- > > drivers/base/power/domain.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > index 983f086ab235..9d3d3c2a5979 100644 > > --- a/drivers/base/power/domain.c > > +++ b/drivers/base/power/domain.c > > @@ -726,6 +726,8 @@ static bool pm_genpd_present(const struct generic_pm_domain *genpd) > > { > > const struct generic_pm_domain *gpd; > > > > + lockdep_assert_held(&gpd_list_lock); > > + > > if (IS_ERR_OR_NULL(genpd)) > > return false; > > > > @@ -1321,6 +1323,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd, > > struct gpd_link *link, *itr; > > int ret = 0; > > > > + lockdep_assert_held(&gpd_list_lock); > > + > > if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain) > > || genpd == subdomain) > > return -EINVAL; > > @@ -1528,6 +1532,8 @@ static int genpd_remove(struct generic_pm_domain *genpd) > > { > > struct gpd_link *l, *link; > > > > + lockdep_assert_held(&gpd_list_lock); > > + > > if (IS_ERR_OR_NULL(genpd)) > > return -EINVAL; > > > > -- > > 2.9.3 > >
On 13 June 2017 at 09:12, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Mon, Jun 12, 2017 at 09:09:59PM +0200, Ulf Hansson wrote: >> On 12 June 2017 at 17:17, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> > Add lockdep checks for holding mutex protecting the list of domains. >> > This might expose misuse even though only file-scope functions use it >> > for now. >> >> I think it seems a bit silly to use these lockdep checks as these >> functions are as you state above, static functions. Moreover there are >> called from a quite limited amount of places. >> >> Do you really think this add some value? > > In ideal world, these would not need lockdeps because we do not make > mistakes. I agree, that mostly the exposed functions to other kernel > modules should be protected, as lockdep annotation is a more advanced > way of documenting the need of locking. > > Does it mean that file-scope functions should not be annotated? Even in > such case function can be misused as the code is getting more and more > complicated. > > What is best example, one of these calls (pm_genpd_present()) was > already used in wrong way, without locking when iterating over the list. > Having lockdep would point this early. Well, I think we found a potential bug because of reviewing the code. Not because of adding lockdep_assert_held(). Perhaps adding protection with lockdep_assert_held() could be meaningful also for static functions, but only in cases of complicated code. I don't think that is the case here. Anyway, if other people thinks $subject patch is good idea, then I drop my case. :-) [...] Kind regards Uffe
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 983f086ab235..9d3d3c2a5979 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -726,6 +726,8 @@ static bool pm_genpd_present(const struct generic_pm_domain *genpd) { const struct generic_pm_domain *gpd; + lockdep_assert_held(&gpd_list_lock); + if (IS_ERR_OR_NULL(genpd)) return false; @@ -1321,6 +1323,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd, struct gpd_link *link, *itr; int ret = 0; + lockdep_assert_held(&gpd_list_lock); + if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain) || genpd == subdomain) return -EINVAL; @@ -1528,6 +1532,8 @@ static int genpd_remove(struct generic_pm_domain *genpd) { struct gpd_link *l, *link; + lockdep_assert_held(&gpd_list_lock); + if (IS_ERR_OR_NULL(genpd)) return -EINVAL;
Add lockdep checks for holding mutex protecting the list of domains. This might expose misuse even though only file-scope functions use it for now. Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- drivers/base/power/domain.c | 6 ++++++ 1 file changed, 6 insertions(+)