[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/01/15 13:24, Mike Turquette wrote:
> Quoting Tomeu Vizoso (2015-01-23 03:03:30)
>> Moves clock state to struct clk_core, but takes care to change as little API as
>> possible.
>>
>> struct clk_hw still has a pointer to a struct clk, which is the
>> implementation's per-user clk instance, for backwards compatibility.
>>
>> The struct clk that clk_get_parent() returns isn't owned by the caller, but by
>> the clock implementation, so the former shouldn't call clk_put() on it.
>>
>> Because some boards in mach-omap2 still register clocks statically, their clock
>> registration had to be updated to take into account that the clock information
>> is stored in struct clk_core now.
> Tero, Paul & Tony,
>
> Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
> struct dpll_data, namely this snippet from
> arch/arm/mach-omap2/dpll3xxx.c:
>
>         parent = __clk_get_parent(hw->clk);
>
>         if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
>                 WARN(parent != dd->clk_bypass,
>                                 "here0, parent name is %s, bypass name is %s\n",
>                                 __clk_get_name(parent), __clk_get_name(dd->clk_bypass));
>                 r = _omap3_noncore_dpll_bypass(clk);
>         } else {
>                 WARN(parent != dd->clk_ref,
>                                 "here1, parent name is %s, ref name is %s\n",
>                                 __clk_get_name(parent), __clk_get_name(dd->clk_ref));
>                 r = _omap3_noncore_dpll_lock(clk);
>         }
>
> struct dpll_data has members clk_ref and clk_bypass which are struct clk
> pointers. This was always a bit of a violation of the clk.h contract
> since drivers are not supposed to deref struct clk pointers. 

Julia,

Is there a way we can write a coccinelle script to check for this? The
goal being to find all drivers that are comparing struct clk pointers or
attempting to dereference them. There are probably other frameworks that
could use the same type of check (regulator, gpiod, reset, pwm, etc.).
Probably anything that has a get/put API.

-Stephen

> Now that we
> generate unique pointers for each call to clk_get (clk_ref & clk_bypass
> are populated by of_clk_get in ti_clk_register_dpll) then the pointer
> comparisons above will never be equal (even if they resolve down to the
> same struct clk_core). I added the verbose traces to the WARNs above to
> illustrate the point: the names are always the same but the pointers
> differ.
>
> AFAICT this doesn't break anything, but booting on OMAP3+ results in
> noisy WARNs.
>
> I think the correct fix is to replace clk_bypass and clk_ref pointers
> with a simple integer parent_index. In fact we already have this index.
> See how the pointers are populated in ti_clk_register_dpll:
>
>
>         dd->clk_ref = of_clk_get(node, 0);
>         dd->clk_bypass = of_clk_get(node, 1);
>
> Tony, the same problem affects the FAPLL code which copy/pastes some of
> the DPLL code.
>
> Thoughts?
>
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Julia Lawall]

On Mon, 2 Feb 2015, Stephen Boyd wrote:

> On 02/01/15 13:24, Mike Turquette wrote:
> > Quoting Tomeu Vizoso (2015-01-23 03:03:30)
> >> Moves clock state to struct clk_core, but takes care to change as little API as
> >> possible.
> >>
> >> struct clk_hw still has a pointer to a struct clk, which is the
> >> implementation's per-user clk instance, for backwards compatibility.
> >>
> >> The struct clk that clk_get_parent() returns isn't owned by the caller, but by
> >> the clock implementation, so the former shouldn't call clk_put() on it.
> >>
> >> Because some boards in mach-omap2 still register clocks statically, their clock
> >> registration had to be updated to take into account that the clock information
> >> is stored in struct clk_core now.
> > Tero, Paul & Tony,
> >
> > Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
> > struct dpll_data, namely this snippet from
> > arch/arm/mach-omap2/dpll3xxx.c:
> >
> >         parent = __clk_get_parent(hw->clk);
> >
> >         if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
> >                 WARN(parent != dd->clk_bypass,
> >                                 "here0, parent name is %s, bypass name is %s\n",
> >                                 __clk_get_name(parent), __clk_get_name(dd->clk_bypass));
> >                 r = _omap3_noncore_dpll_bypass(clk);
> >         } else {
> >                 WARN(parent != dd->clk_ref,
> >                                 "here1, parent name is %s, ref name is %s\n",
> >                                 __clk_get_name(parent), __clk_get_name(dd->clk_ref));
> >                 r = _omap3_noncore_dpll_lock(clk);
> >         }
> >
> > struct dpll_data has members clk_ref and clk_bypass which are struct clk
> > pointers. This was always a bit of a violation of the clk.h contract
> > since drivers are not supposed to deref struct clk pointers. 
> 
> Julia,
> 
> Is there a way we can write a coccinelle script to check for this? The
> goal being to find all drivers that are comparing struct clk pointers or
> attempting to dereference them. There are probably other frameworks that
> could use the same type of check (regulator, gpiod, reset, pwm, etc.).
> Probably anything that has a get/put API.

Comparing or dereferencing pointers of a particular type should be 
straightforward to check for.  Is there an example of how to use the 
parent_index value to fix the problem?

julia


> 
> -Stephen
> 
> > Now that we
> > generate unique pointers for each call to clk_get (clk_ref & clk_bypass
> > are populated by of_clk_get in ti_clk_register_dpll) then the pointer
> > comparisons above will never be equal (even if they resolve down to the
> > same struct clk_core). I added the verbose traces to the WARNs above to
> > illustrate the point: the names are always the same but the pointers
> > differ.
> >
> > AFAICT this doesn't break anything, but booting on OMAP3+ results in
> > noisy WARNs.
> >
> > I think the correct fix is to replace clk_bypass and clk_ref pointers
> > with a simple integer parent_index. In fact we already have this index.
> > See how the pointers are populated in ti_clk_register_dpll:
> >
> >
> >         dd->clk_ref = of_clk_get(node, 0);
> >         dd->clk_bypass = of_clk_get(node, 1);
> >
> > Tony, the same problem affects the FAPLL code which copy/pastes some of
> > the DPLL code.
> >
> > Thoughts?
> >
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> 

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/02/15 13:31, Julia Lawall wrote:
>
> On Mon, 2 Feb 2015, Stephen Boyd wrote:
>
>> Julia,
>>
>> Is there a way we can write a coccinelle script to check for this? The
>> goal being to find all drivers that are comparing struct clk pointers or
>> attempting to dereference them. There are probably other frameworks that
>> could use the same type of check (regulator, gpiod, reset, pwm, etc.).
>> Probably anything that has a get/put API.
> Comparing or dereferencing pointers of a particular type should be 
> straightforward to check for.  Is there an example of how to use the 
> parent_index value to fix the problem?
>

I'm not sure how to fix this case with parent_index values generically.
I imagine it would be highly specific to the surrounding code to the
point where we couldn't fix it automatically. For example, if it's a clk
consumer (not provider like in this case) using an index wouldn't be the
right fix. We would need some sort of clk API that we don't currently
have and I would wonder why clock consumers even care to compare such
pointers in the first place.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Mike Turquette]

Quoting Stephen Boyd (2015-02-02 14:35:59)
> On 02/02/15 13:31, Julia Lawall wrote:
> >
> > On Mon, 2 Feb 2015, Stephen Boyd wrote:
> >
> >> Julia,
> >>
> >> Is there a way we can write a coccinelle script to check for this? The
> >> goal being to find all drivers that are comparing struct clk pointers or
> >> attempting to dereference them. There are probably other frameworks that
> >> could use the same type of check (regulator, gpiod, reset, pwm, etc.).
> >> Probably anything that has a get/put API.
> > Comparing or dereferencing pointers of a particular type should be 
> > straightforward to check for.  Is there an example of how to use the 
> > parent_index value to fix the problem?
> >
> 
> I'm not sure how to fix this case with parent_index values generically.
> I imagine it would be highly specific to the surrounding code to the
> point where we couldn't fix it automatically. For example, if it's a clk
> consumer (not provider like in this case) using an index wouldn't be the
> right fix. We would need some sort of clk API that we don't currently
> have and I would wonder why clock consumers even care to compare such
> pointers in the first place.

Ack. Is there precedent for a "Don't do that" kind of response?

Regards,
Mike

> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Quentin Lambert]

Hello,
Julia asked me to have a look and see if I can help.

On 02/02/2015 23:50, Mike Turquette wrote:
> Quoting Stephen Boyd (2015-02-02 14:35:59)
>> On 02/02/15 13:31, Julia Lawall wrote:
>>> On Mon, 2 Feb 2015, Stephen Boyd wrote:
>>>
>>>> Julia,
>>>>
>>>> Is there a way we can write a coccinelle script to check for this? The
>>>> goal being to find all drivers that are comparing struct clk pointers or
>>>> attempting to dereference them. There are probably other frameworks that
>>>> could use the same type of check (regulator, gpiod, reset, pwm, etc.).
>>>> Probably anything that has a get/put API.
>>> Comparing or dereferencing pointers of a particular type should be
>>> straightforward to check for.  Is there an example of how to use the
>>> parent_index value to fix the problem?
>>>
>> I'm not sure how to fix this case with parent_index values generically.
>> I imagine it would be highly specific to the surrounding code to the
>> point where we couldn't fix it automatically. For example, if it's a clk
>> consumer (not provider like in this case) using an index wouldn't be the
>> right fix. We would need some sort of clk API that we don't currently
>> have and I would wonder why clock consumers even care to compare such
>> pointers in the first place.
> Ack. Is there precedent for a "Don't do that" kind of response?
>
> Regards,
> Mike
>
>> -- 
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> _______________________________________________
> Cocci mailing list
> Cocci at systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
I have found these three cases using Coccinnelle in the mach-omap2
directory.



./arch/arm/mach-omap2/clkt_clksel.c
@@ -67,7 +67,6 @@ static const struct clksel *_get_clksel_
                 return NULL;

         for (clks = clk->clksel; clks->parent; clks++)
                 if (clks->parent == src_clk)
                         break; /* Found the requested parent */

         if (!clks->parent) {
./arch/arm/mach-omap2/dpll3xxx.c
@@ -551,7 +549,6 @@ int omap3_noncore_dpll_set_rate(struct c
         if (!dd)
                 return -EINVAL;

         if (__clk_get_parent(hw->clk) != dd->clk_ref)
                 return -EINVAL;

         if (dd->last_rounded_rate == 0)
./arch/arm/mach-omap2/timer.c
@@ -298,7 +298,6 @@ static int __init omap_dm_timer_init_one
         if (IS_ERR(src))
                 return PTR_ERR(src);

         if (clk_get_parent(timer->fclk) != src) {
                 r = clk_set_parent(timer->fclk, src);
                 if (r < 0) {
                         pr_warn("%s: %s cannot set source\n", __func__,


Are they instances of your issue?
Do you have any suggestion on how to fix them?
If you want me to I can enlarge the search to other directories.


Quentin

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/03/15 08:04, Quentin Lambert wrote:
> Hello,
> Julia asked me to have a look and see if I can help.
>
> I have found these three cases using Coccinnelle in the mach-omap2
> directory.
>
>
>
> ./arch/arm/mach-omap2/clkt_clksel.c
> @@ -67,7 +67,6 @@ static const struct clksel *_get_clksel_
>                 return NULL;
>
>         for (clks = clk->clksel; clks->parent; clks++)
>                 if (clks->parent == src_clk)

This probably needs to compare hw pointers again given that it's in the
clk-provider code. It would be nice if we could use indices instead though.

>                         break; /* Found the requested parent */
>
>         if (!clks->parent) {
> ./arch/arm/mach-omap2/dpll3xxx.c
> @@ -551,7 +549,6 @@ int omap3_noncore_dpll_set_rate(struct c
>         if (!dd)
>                 return -EINVAL;
>
>         if (__clk_get_parent(hw->clk) != dd->clk_ref)

This one is what this thread has started on about. Comparing hw pointers
is ok for now...

>                 return -EINVAL;
>
>         if (dd->last_rounded_rate == 0)
> ./arch/arm/mach-omap2/timer.c
> @@ -298,7 +298,6 @@ static int __init omap_dm_timer_init_one
>         if (IS_ERR(src))
>                 return PTR_ERR(src);
>
>         if (clk_get_parent(timer->fclk) != src) {

This one looks like an optimization. We can just always try to set the
parent and if it's already the current parent then the core should bail
out early without an error. So the fix would be to just remove the if
condition.

>                 r = clk_set_parent(timer->fclk, src);
>                 if (r < 0) {
>                         pr_warn("%s: %s cannot set source\n", __func__,
>
>
> Are they instances of your issue?

Yes these all look like instances of the problem.

> If you want me to I can enlarge the search to other directories.

Yes please do. And if you could share the coccinelle patch that would be
great. Thanks.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Quentin Lambert]

On 05/02/2015 00:26, Stephen Boyd wrote:
>> If you want me to I can enlarge the search to other directories.
> Yes please do. And if you could share the coccinelle patch that would be
> great. Thanks.
>
structclk.cocci is the coccinelle patch
structclk-arm.patch is the result I got when applying it to the arch/arm 
directory

Is there anything else I can do to help?


-------------- next part --------------
A non-text attachment was scrubbed...
Name: structclk-arm.patch
Type: text/x-patch
Size: 2512 bytes
Desc: not available
URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20150205/7f8a2e33/attachment.bin>
-------------- next part --------------
/// Find any attempt to compare or dereference struct clk pointers.
///
// Confidence: High
// Copyright: (C) 2015 Quentin Lambert, INRIA/LiP6. GPLv2
// URL: http://coccinelle.lip6.fr/
// Options: --recursive-includes --relax-include-path
// Options: --include-headers-for-types

virtual context
virtual org
virtual report


// ----------------------------------------------------------------------------

@comparison_dereference depends on context || org || report@
struct clk *x1, x2;
position j0;
@@

(
*  x1 at j0 == x2
|
*  x1 at j0 != x2
|
*  *x1 at j0
)

// ----------------------------------------------------------------------------

@script:python comparison_dereference_org depends on org@
j0 << comparison_dereference.j0;
@@

msg = "WARNING trying to compare or dereference struct clk pointers."
coccilib.org.print_todo(j0[0], msg)

// ----------------------------------------------------------------------------

@script:python comparison_dereference_report depends on report@
j0 << comparison_dereference.j0;
@@

msg = "WARNING trying to compare or dereference struct clk pointers."
coccilib.report.print_report(j0[0], msg)

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Quentin Lambert]

Sorry let me do that properly.

On 05/02/2015 16:45, Quentin Lambert wrote:
>
> On 05/02/2015 00:26, Stephen Boyd wrote:
>>> If you want me to I can enlarge the search to other directories.
>> Yes please do. And if you could share the coccinelle patch that would be
>> great. Thanks.
>>
> structclk.cocci is the coccinelle patch
> structclk-arm.patch is the result I got when applying it to the 
> arch/arm directory
>
> Is there anything else I can do to help?
>
>

These are the new instances I found by applying the patch to arch/arm 
directory:

./arch/arm/mach-imx/mach-imx6q.c
@@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
       * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
       * (external OSC), and we need to clear the bit.
       */
      clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
                         IMX6Q_GPR1_ENET_CLK_SEL_PAD;
      gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
      if (!IS_ERR(gpr))

./arch/arm/mach-shmobile/clock-r8a73a4.c
@@ -139,7 +139,6 @@ static int pll_set_parent(struct clk *cl

      /* Search the parent */
      for (i = 0; i < clk->parent_num; i++)
          if (clk->parent_table[i] == parent)
              break;

      if (i == clk->parent_num)

./arch/arm/mach-shmobile/clock-sh7372.c
@@ -223,7 +223,6 @@ static int pllc2_set_parent(struct clk *

      /* Search the parent */
      for (i = 0; i < clk->parent_num; i++)
          if (clk->parent_table[i] == parent)
              break;

      if (i == clk->parent_num)

./arch/arm/mach-shmobile/clock-r8a7740.c
@@ -195,7 +195,6 @@ static int usb24s_set_parent(struct clk

      /* Search the parent */
      for (i = 0; i < clk->parent_num; i++)
          if (clk->parent_table[i] == parent)
              break;

      if (i == clk->parent_num)

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/05/15 08:02, Quentin Lambert wrote:
> Sorry let me do that properly.
>
> On 05/02/2015 16:45, Quentin Lambert wrote:
>>
>> On 05/02/2015 00:26, Stephen Boyd wrote:
>>>> If you want me to I can enlarge the search to other directories.
>>> Yes please do. And if you could share the coccinelle patch that
>>> would be
>>> great. Thanks.
>>>
>> structclk.cocci is the coccinelle patch
>> structclk-arm.patch is the result I got when applying it to the
>> arch/arm directory
>>
>> Is there anything else I can do to help?
>>
>>
>
> These are the new instances I found by applying the patch to arch/arm
> directory:
>
> ./arch/arm/mach-imx/mach-imx6q.c
> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>       * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
>       * (external OSC), and we need to clear the bit.
>       */
>      clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
>                         IMX6Q_GPR1_ENET_CLK_SEL_PAD;
>      gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>      if (!IS_ERR(gpr))

This one looks like a real problem and it could probably use a
clk_equal(struct clk *a, struct clk *b) function.

>
> ./arch/arm/mach-shmobile/clock-r8a73a4.c
> @@ -139,7 +139,6 @@ static int pll_set_parent(struct clk *cl
>
>      /* Search the parent */
>      for (i = 0; i < clk->parent_num; i++)
>          if (clk->parent_table[i] == parent)
>              break;
>
>      if (i == clk->parent_num)
>
> ./arch/arm/mach-shmobile/clock-sh7372.c
> @@ -223,7 +223,6 @@ static int pllc2_set_parent(struct clk *
>
>      /* Search the parent */
>      for (i = 0; i < clk->parent_num; i++)
>          if (clk->parent_table[i] == parent)
>              break;
>
>      if (i == clk->parent_num)
>
> ./arch/arm/mach-shmobile/clock-r8a7740.c
> @@ -195,7 +195,6 @@ static int usb24s_set_parent(struct clk
>
>      /* Search the parent */
>      for (i = 0; i < clk->parent_num; i++)
>          if (clk->parent_table[i] == parent)
>              break;
>
>      if (i == clk->parent_num)
>
>
>

I don't think shmobile uses the CCF so this should be safe, but we
should probably fix them up to also use a clk_equal() function that just
does pointer comparisons.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/05/15 07:45, Quentin Lambert wrote:
>
> On 05/02/2015 00:26, Stephen Boyd wrote:
>>> If you want me to I can enlarge the search to other directories.
>> Yes please do. And if you could share the coccinelle patch that would be
>> great. Thanks.
>>
> structclk.cocci is the coccinelle patch
> structclk-arm.patch is the result I got when applying it to the
> arch/arm directory
>
> Is there anything else I can do to help?
>
>

Thanks for the coccinelle patch. Thinking more about it, I don't think
we care if the pointer is dereferenced because that would require a
definition of struct clk and that is most likely not the case outside of
the clock framework. Did you scan the entire kernel? I'm running it now
but it seems to be taking a while.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Quentin Lambert]

On 06/02/2015 03:15, Stephen Boyd wrote:
> Thanks for the coccinelle patch. Thinking more about it, I don't think
> we care if the pointer is dereferenced because that would require a
> definition of struct clk and that is most likely not the case outside of
> the clock framework. Did you scan the entire kernel?
No I haven't.
> I'm running it now
> but it seems to be taking a while.
>
Yes, that's why, as a first step, I chose to limit the scan to the arm 
directory.

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Julia Lawall]

On Fri, 6 Feb 2015, Quentin Lambert wrote:

>
> On 06/02/2015 03:15, Stephen Boyd wrote:
> > Thanks for the coccinelle patch. Thinking more about it, I don't think
> > we care if the pointer is dereferenced because that would require a
> > definition of struct clk and that is most likely not the case outside of
> > the clock framework. Did you scan the entire kernel?
> No I haven't.
> > I'm running it now
> > but it seems to be taking a while.
> >
> Yes, that's why, as a first step, I chose to limit the scan to the arm
> directory.

Are you sure to be using all of the options provided:

// Options: --recursive-includes --relax-include-path
// Options: --include-headers-for-types

And are you using 1.0.0-rc23 or 1.0.0-rc24?  Those should save parsed
header files so that they don't have to be parsed over and over.

If you are using rc24, then you can use the -j option for parallelism.
But then you should also use an option like --chunksize 10 (I don't know
what number would be good), because the default is chunksize 1, and in
that case the saved parsed header files are not reused, because the fies
are all processed individually.  In general, it is only the files within a
chunk that will share parsed header files.

julia

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/06/15 01:12, Julia Lawall wrote:
>
> On Fri, 6 Feb 2015, Quentin Lambert wrote:
>
>> On 06/02/2015 03:15, Stephen Boyd wrote:
>>> Thanks for the coccinelle patch. Thinking more about it, I don't think
>>> we care if the pointer is dereferenced because that would require a
>>> definition of struct clk and that is most likely not the case outside of
>>> the clock framework. Did you scan the entire kernel?
>> No I haven't.
>>> I'm running it now
>>> but it seems to be taking a while.
>>>
>> Yes, that's why, as a first step, I chose to limit the scan to the arm
>> directory.
> Are you sure to be using all of the options provided:
>
> // Options: --recursive-includes --relax-include-path
> // Options: --include-headers-for-types
>
> And are you using 1.0.0-rc23 or 1.0.0-rc24?  Those should save parsed
> header files so that they don't have to be parsed over and over.
>
> If you are using rc24, then you can use the -j option for parallelism.
> But then you should also use an option like --chunksize 10 (I don't know
> what number would be good), because the default is chunksize 1, and in
> that case the saved parsed header files are not reused, because the fies
> are all processed individually.  In general, it is only the files within a
> chunk that will share parsed header files.

Thanks for the info.

$ spatch --version
spatch version 1.0.0-rc22 with Python support and with PCRE support

so I guess I need to update. I tried throwing it into
scripts/coccinelle/misc and then did

make O=../obj/ coccicheck
COCCI=../kernel/scripts/coccinelle/misc/structclk.cocci

at the toplevel but it didn't find anything.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 02/05/15 18:15, Stephen Boyd wrote:
> On 02/05/15 07:45, Quentin Lambert wrote:
>> On 05/02/2015 00:26, Stephen Boyd wrote:
>>>> If you want me to I can enlarge the search to other directories.
>>> Yes please do. And if you could share the coccinelle patch that would be
>>> great. Thanks.
>>>
>> structclk.cocci is the coccinelle patch
>> structclk-arm.patch is the result I got when applying it to the
>> arch/arm directory
>>
>> Is there anything else I can do to help?
>>
>>
> Thanks for the coccinelle patch. Thinking more about it, I don't think
> we care if the pointer is dereferenced because that would require a
> definition of struct clk and that is most likely not the case outside of
> the clock framework. Did you scan the entire kernel? I'm running it now
> but it seems to be taking a while.
>

I ran the script on all files that include <linux/clk.h>. I've also
trimmed out mips and unicore32 because they're not using the common
clock framework.

diff = 
--- arch/arm/mach-imx/mach-imx6q.c
+++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
@@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
 	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
 	 * (external OSC), and we need to clear the bit.
 	 */
-	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
 				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
 	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
 	if (!IS_ERR(gpr))
diff = 
--- drivers/gpu/drm/armada/armada_510.c
+++ /tmp/cocci-output-12321-a5f298-armada_510.c
@@ -53,7 +53,6 @@ static int armada510_crtc_compute_clock(
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-	if (dcrtc->clk != clk) {
 		ret = clk_prepare_enable(clk);
 		if (ret)
 			return ret;
drivers/gpu/drm/armada/armada_510.c:56:5-15: WARNING trying to compare or dereference struct clk pointers.
diff = 
--- drivers/pwm/pwm-atmel-hlcdc.c
+++ /tmp/cocci-output-12679-3c5195-pwm-atmel-hlcdc.c
@@ -91,7 +91,6 @@ static int atmel_hlcdc_pwm_config(struct
 
 	pwmcfg = ATMEL_HLCDC_PWMPS(pres);
 
-	if (new_clk != chip->cur_clk) {
 		u32 gencfg = 0;
 		int ret;
 
drivers/pwm/pwm-atmel-hlcdc.c:94:5-12: WARNING trying to compare or dereference struct clk pointers.
diff = 
--- drivers/tty/serial/samsung.c
+++ /tmp/cocci-output-12827-715e72-samsung.c
@@ -750,7 +750,6 @@ static void s3c24xx_serial_set_termios(s
 
 	/* check to see if we need  to change clock source */
 
-	if (ourport->baudclk != clk) {
 		s3c24xx_serial_setsource(port, clk_sel);
 
 		if (!IS_ERR(ourport->baudclk)) {
drivers/tty/serial/samsung.c:753:5-21: WARNING trying to compare or dereference struct clk pointers.
diff = 
--- sound/soc/fsl/fsl_esai.c
+++ /tmp/cocci-output-13020-d518c3-fsl_esai.c
@@ -269,7 +269,6 @@ static int fsl_esai_set_dai_sysclk(struc
 	}
 
 	/* Only EXTAL source can be output directly without using PSR and PM */
-	if (ratio == 1 && clksrc == esai_priv->extalclk) {
 		/* Bypass all the dividers if not being needed */
 		ecr |= tx ? ESAI_ECR_ETO : ESAI_ECR_ERO;
 		goto out;
sound/soc/fsl/fsl_esai.c:272:19-25: WARNING trying to compare or dereference struct clk pointers.
diff = 
--- sound/soc/fsl/fsl_spdif.c
+++ /tmp/cocci-output-13024-7acb1d-fsl_spdif.c
@@ -1054,7 +1054,6 @@ static u32 fsl_spdif_txclk_caldiv(struct
 				enum spdif_txrate index, bool round)
 {
 	const u32 rate[] = { 32000, 44100, 48000, 96000, 192000 };
-	bool is_sysclk = clk == spdif_priv->sysclk;
 	u64 rate_ideal, rate_actual, sub;
 	u32 sysclk_dfmin, sysclk_dfmax;
 	u32 txclk_df, sysclk_df, arate;
@@ -1148,7 +1147,6 @@ static int fsl_spdif_probe_txclk(struct
 			spdif_priv->txclk_src[index], rate[index]);
 	dev_dbg(&pdev->dev, "use txclk df %d for %dHz sample rate\n",
 			spdif_priv->txclk_df[index], rate[index]);
-	if (spdif_priv->txclk[index] == spdif_priv->sysclk)
 		dev_dbg(&pdev->dev, "use sysclk df %d for %dHz sample rate\n",
 				spdif_priv->sysclk_df[index], rate[index]);
 	dev_dbg(&pdev->dev, "the best rate for %dHz sample rate is %dHz\n",
sound/soc/fsl/fsl_spdif.c:1151:5-29: WARNING trying to compare or dereference struct clk pointers.
sound/soc/fsl/fsl_spdif.c:1057:18-21: WARNING trying to compare or dereference struct clk pointers.
diff = 
--- sound/soc/kirkwood/kirkwood-i2s.c
+++ /tmp/cocci-output-13041-3200a6-kirkwood-i2s.c
@@ -579,7 +579,6 @@ static int kirkwood_i2s_dev_probe(struct
 		if (PTR_ERR(priv->extclk) == -EPROBE_DEFER)
 			return -EPROBE_DEFER;
 	} else {
-		if (priv->extclk == priv->clk) {
 			devm_clk_put(&pdev->dev, priv->extclk);
 			priv->extclk = ERR_PTR(-EINVAL);
 		} else {
sound/soc/kirkwood/kirkwood-i2s.c:582:6-18: WARNING trying to compare or dereference struct clk pointers.


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Sebastian Andrzej Siewior]

On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
> diff = 
> --- arch/arm/mach-imx/mach-imx6q.c
> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
>  	 * (external OSC), and we need to clear the bit.
>  	 */
> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>  	if (!IS_ERR(gpr))

Any idea how to do the comparison here? Or should we rely that the bootloader
sets this properly (it managed already to select a frequency)? The phy has no
clock node in current DT's so we can check this.

Sebastian

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 03/12/15 10:20, Sebastian Andrzej Siewior wrote:
> On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
>> diff = 
>> --- arch/arm/mach-imx/mach-imx6q.c
>> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
>> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
>>  	 * (external OSC), and we need to clear the bit.
>>  	 */
>> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
>>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
>>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>>  	if (!IS_ERR(gpr))
> Any idea how to do the comparison here? Or should we rely that the bootloader
> sets this properly (it managed already to select a frequency)? The phy has no
> clock node in current DT's so we can check this.
>

This has been fixed by adding a clk_is_match() helper and using that to
compare instead of comparing raw pointers. It would be nice if we could
replace the patch with something else that doesn't require this helper
though. It looks like this is static board configuration, so I wonder
why we didn't just have a DT property that indicates how the gpr should
be configured for this particular board.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Shawn Guo]

On Thu, Mar 12, 2015 at 12:43:40PM -0700, Stephen Boyd wrote:
> On 03/12/15 10:20, Sebastian Andrzej Siewior wrote:
> > On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
> >> diff = 
> >> --- arch/arm/mach-imx/mach-imx6q.c
> >> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
> >> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
> >>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
> >>  	 * (external OSC), and we need to clear the bit.
> >>  	 */
> >> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
> >>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
> >>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> >>  	if (!IS_ERR(gpr))
> > Any idea how to do the comparison here? Or should we rely that the bootloader
> > sets this properly (it managed already to select a frequency)? The phy has no
> > clock node in current DT's so we can check this.
> >
> 
> This has been fixed by adding a clk_is_match() helper and using that to
> compare instead of comparing raw pointers. It would be nice if we could
> replace the patch with something else that doesn't require this helper
> though. It looks like this is static board configuration, so I wonder
> why we didn't just have a DT property that indicates how the gpr should
> be configured for this particular board.

We did not add a DT property for it, because there was already enough
info (clock configuration) in DT for kernel to figure it out.

Shawn

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Sebastian Andrzej Siewior]

Hi Shawn,

On Fri, Mar 13, 2015 at 11:29:32AM +0800, Shawn Guo wrote:

> We did not add a DT property for it, because there was already enough
> info (clock configuration) in DT for kernel to figure it out.
Correct. My understanding is whatever can be figured out without DT should
be done that way.

Is there a way to get this clock-select bit set without
enable_fec_anatop_clock() in u-boot? Because this one selects the clock and
frequency and also sets the proper bit in gpr1. My question is simply why do
we need to do this here as well.

> Shawn

Sebastian

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Shawn Guo]

On Fri, Mar 13, 2015 at 09:20:10AM +0100, Sebastian Andrzej Siewior wrote:
> Hi Shawn,
> 
> On Fri, Mar 13, 2015 at 11:29:32AM +0800, Shawn Guo wrote:
> 
> > We did not add a DT property for it, because there was already enough
> > info (clock configuration) in DT for kernel to figure it out.
> Correct. My understanding is whatever can be figured out without DT should
> be done that way.
> 
> Is there a way to get this clock-select bit set without
> enable_fec_anatop_clock() in u-boot? Because this one selects the clock and
> frequency and also sets the proper bit in gpr1. My question is simply why do
> we need to do this here as well.

I'm not sure I follow your question.  But we had been doing this setup
in kernel function imx6q_1588_init() for a while.  The problem we're
having now is that the clk core change broke it since v4.0-rc1.  And the
fix is already queued there [1].

Shawn

[1] https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-fixes

[Cocci] [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

[Stephen Boyd]

On 03/12/15 20:29, Shawn Guo wrote:
> On Thu, Mar 12, 2015 at 12:43:40PM -0700, Stephen Boyd wrote:
>> On 03/12/15 10:20, Sebastian Andrzej Siewior wrote:
>>> On 2015-02-17 14:01:04 [-0800], Stephen Boyd wrote:
>>>> diff = 
>>>> --- arch/arm/mach-imx/mach-imx6q.c
>>>> +++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
>>>> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
>>>>  	 * set bit IOMUXC_GPR1[21].  Or the PTP clock must be from pad
>>>>  	 * (external OSC), and we need to clear the bit.
>>>>  	 */
>>>> -	clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
>>>>  				       IMX6Q_GPR1_ENET_CLK_SEL_PAD;
>>>>  	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>>>>  	if (!IS_ERR(gpr))
>>> Any idea how to do the comparison here? Or should we rely that the bootloader
>>> sets this properly (it managed already to select a frequency)? The phy has no
>>> clock node in current DT's so we can check this.
>>>
>> This has been fixed by adding a clk_is_match() helper and using that to
>> compare instead of comparing raw pointers. It would be nice if we could
>> replace the patch with something else that doesn't require this helper
>> though. It looks like this is static board configuration, so I wonder
>> why we didn't just have a DT property that indicates how the gpr should
>> be configured for this particular board.
> We did not add a DT property for it, because there was already enough
> info (clock configuration) in DT for kernel to figure it out.

Ok well then if everything is in DT we don't need to be comparing clock
pointers at all, right? We should be able to write up some DT parsing
logic to figure it out?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project