diff mbox

[v2,3/8] PM / Domains: Add lockdep asserts for domains list mutex

Message ID 20170612151748.7380-4-krzk@kernel.org (mailing list archive)
State Deferred
Headers show

Commit Message

Krzysztof Kozlowski June 12, 2017, 3:17 p.m. UTC
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(+)

Comments

Ulf Hansson June 12, 2017, 7:09 p.m. UTC | #1
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
>
Krzysztof Kozlowski June 13, 2017, 7:12 a.m. UTC | #2
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
> >
Ulf Hansson June 13, 2017, 8:34 a.m. UTC | #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 mbox

Patch

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;