diff mbox

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

Message ID 54D3900A.9060200@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Quentin Lambert Feb. 5, 2015, 3:45 p.m. UTC 
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?
/// 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@j0 == x2
|
*  x1@j0 != x2
|
*  *x1@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)

Comments

Quentin Lambert Feb. 5, 2015, 4:02 p.m. UTC | #1 
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)



--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Feb. 6, 2015, 1:49 a.m. UTC | #2 
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.
Stephen Boyd Feb. 6, 2015, 2:15 a.m. UTC | #3 
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.
Quentin Lambert Feb. 6, 2015, 9:01 a.m. UTC | #4 
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julia Lawall Feb. 6, 2015, 9:12 a.m. UTC | #5 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Feb. 6, 2015, 5:15 p.m. UTC | #6 
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.
diff mbox

Patch

diff -u -p ./arch/arm/mach-imx/mach-imx6q.c /tmp/nothing/mach-imx/mach-imx6q.c
--- ./arch/arm/mach-imx/mach-imx6q.c
+++ /tmp/nothing/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))
diff -u -p ./arch/arm/mach-shmobile/clock-r8a73a4.c /tmp/nothing/mach-shmobile/clock-r8a73a4.c
--- ./arch/arm/mach-shmobile/clock-r8a73a4.c
+++ /tmp/nothing/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)
diff -u -p ./arch/arm/mach-shmobile/clock-sh7372.c /tmp/nothing/mach-shmobile/clock-sh7372.c
--- ./arch/arm/mach-shmobile/clock-sh7372.c
+++ /tmp/nothing/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)
diff -u -p ./arch/arm/mach-shmobile/clock-r8a7740.c /tmp/nothing/mach-shmobile/clock-r8a7740.c
--- ./arch/arm/mach-shmobile/clock-r8a7740.c
+++ /tmp/nothing/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)
diff -u -p ./arch/arm/mach-omap2/clkt_clksel.c /tmp/nothing/mach-omap2/clkt_clksel.c
--- ./arch/arm/mach-omap2/clkt_clksel.c
+++ /tmp/nothing/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) {
diff -u -p ./arch/arm/mach-omap2/timer.c /tmp/nothing/mach-omap2/timer.c
--- ./arch/arm/mach-omap2/timer.c
+++ /tmp/nothing/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__,