[PATCH 0/3] clk: remove dead code allocations of parent lookup tables

[PATCH 0/3] clk: remove dead code allocations of parent lookup tables

[Vladimir Zapolskiy]

The change simplifies core operations over multi-parent clocks (clock
registration, clk_set_parent() and clk_get_parent() internals) by
rejecting a possibilty to skip failed memory allocation during clock
registration time and falling back to it on the next [gs]et_parent.

No functional change is intended.

Vladimir Zapolskiy (3):
  clk: don't fetch parent index for non multi-parent clocks
  clk: simplify array allocation of clock parents for lookup
  clk: remove redundant clock parents lookup table allocations

 drivers/clk/clk.c | 49 +++++++++++++++++++------------------------------
 1 file changed, 19 insertions(+), 30 deletions(-)

-- 
2.5.0

[PATCH 1/3] clk: don't fetch parent index for non multi-parent clocks

[Vladimir Zapolskiy]

If clk_set_parent(clk, parent) is called when 'clk' is a root clock or
'clk' has a single parent different from 'parent' clock, a vain
attempt to find 'parent' index is still performed, which causes
either an allocation of 0-byte size lookup array or never used
sizeof(struct clk*) array.

Like in case of invalid clk_set_parent() of a multi-parent clock,
return -EINVAL in the cases described above without trying to allocate
a lookup table.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/clk/clk.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b4db67a..129ac0a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1794,8 +1794,13 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
 	if (core->parent == parent)
 		goto out;
 
+	if (core->num_parents <= 1) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/* verify ops for for multi-parent clks */
-	if ((core->num_parents > 1) && (!core->ops->set_parent)) {
+	if (!core->ops->set_parent) {
 		ret = -ENOSYS;
 		goto out;
 	}
-- 
2.1.4

[PATCH 2/3] clk: simplify array allocation of clock parents for lookup

[Vladimir Zapolskiy]

The change intends to centralize allocation of clock's clk_core
parents array on clock registration, originally is done from multiple
places:
* __clk_init()
* __clk_init_parent()
* clk_fetch_parent_index()

The original approach historically was needed, but at the moment
all assumptions supporting it are wrong:

1) GFP_KERNEL heap is available at the time of a clock registration,
   for example it is used to dynamically allocate struct clk_core,
   clock name and clock parent names, if allocation fails, return
   -ENOMEM as usual instead of repeating attempts to allocate the
   array in future,

2) due to the layering scheme clk_core parents can not be defined
   statically by the drivers, instead the drivers provide string
   arrays of clock parent names and clock parents for a lookup array
   are always obtained from that data.

To ensure that there is no need to check for allocated status of the
parent lookup table and/or allocate the table from other places within
the common clock framework, don't register a clock, if there is no
enough memory.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/clk/clk.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 129ac0a..2fb0dae 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2371,27 +2371,25 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
 
 	/*
 	 * Allocate an array of struct clk *'s to avoid unnecessary string
-	 * look-ups of clk's possible parents.  This can fail for clocks passed
-	 * in to clk_init during early boot; thus any access to core->parents[]
-	 * must always check for a NULL pointer and try to populate it if
-	 * necessary.
-	 *
-	 * If core->parents is not NULL we skip this entire block.  This allows
-	 * for clock drivers to statically initialize core->parents.
+	 * look-ups of clk's possible parents.
 	 */
-	if (core->num_parents > 1 && !core->parents) {
+	if (core->num_parents > 1) {
 		core->parents = kcalloc(core->num_parents, sizeof(struct clk *),
 					GFP_KERNEL);
+		if (!core->parents) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
 		/*
 		 * clk_core_lookup returns NULL for parents that have not been
 		 * clk_init'd; thus any access to clk->parents[] must check
 		 * for a NULL pointer.  We can always perform lazy lookups for
 		 * missing parents later on.
 		 */
-		if (core->parents)
-			for (i = 0; i < core->num_parents; i++)
-				core->parents[i] =
-					clk_core_lookup(core->parent_names[i]);
+		for (i = 0; i < core->num_parents; i++)
+			core->parents[i] =
+				clk_core_lookup(core->parent_names[i]);
 	}
 
 	core->parent = __clk_init_parent(core);
-- 
2.1.4

[PATCH 3/3] clk: remove redundant clock parents lookup table allocations

[Vladimir Zapolskiy]

Since clock parents lookup table for clocks with multiple parent
clocks is always allocated during clock registration, remove similar
allocations from __clk_init_parent() and clk_fetch_parent_index().

The change also corrects a pointer type of a single lookup table
entry on calculation of the lookup table size.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/clk/clk.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2fb0dae..f8872f9 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1067,13 +1067,6 @@ static int clk_fetch_parent_index(struct clk_core *core,
 {
 	int i;
 
-	if (!core->parents) {
-		core->parents = kcalloc(core->num_parents,
-					sizeof(struct clk *), GFP_KERNEL);
-		if (!core->parents)
-			return -ENOMEM;
-	}
-
 	/*
 	 * find index of new parent clock using cached parent ptrs,
 	 * or if not yet cached, use string name comparison and cache
@@ -1711,18 +1704,11 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
 	}
 
 	/*
-	 * Do our best to cache parent clocks in core->parents.  This prevents
-	 * unnecessary and expensive lookups.  We don't set core->parent here;
-	 * that is done by the calling function.
+	 * We don't set core->parent here; that is done by the calling function.
 	 */
 
 	index = core->ops->get_parent(core->hw);
 
-	if (!core->parents)
-		core->parents =
-			kcalloc(core->num_parents, sizeof(struct clk *),
-					GFP_KERNEL);
-
 	ret = clk_core_get_parent_by_index(core, index);
 
 out:
@@ -2374,8 +2360,8 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
 	 * look-ups of clk's possible parents.
 	 */
 	if (core->num_parents > 1) {
-		core->parents = kcalloc(core->num_parents, sizeof(struct clk *),
-					GFP_KERNEL);
+		core->parents = kcalloc(core->num_parents,
+					sizeof(struct clk_core *), GFP_KERNEL);
 		if (!core->parents) {
 			ret = -ENOMEM;
 			goto out;
-- 
2.1.4

Re: [PATCH 3/3] clk: remove redundant clock parents lookup table allocations

[Masahiro Yamada]

Hi Vladimir,


2016-01-07 12:16 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
> Since clock parents lookup table for clocks with multiple parent
> clocks is always allocated during clock registration, remove similar
> allocations from __clk_init_parent() and clk_fetch_parent_index().
>
> The change also corrects a pointer type of a single lookup table
> entry on calculation of the lookup table size.
>
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  drivers/clk/clk.c | 20 +++-----------------
>  1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 2fb0dae..f8872f9 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1067,13 +1067,6 @@ static int clk_fetch_parent_index(struct clk_core *core,
>  {
>         int i;
>
> -       if (!core->parents) {
> -               core->parents = kcalloc(core->num_parents,
> -                                       sizeof(struct clk *), GFP_KERNEL);
> -               if (!core->parents)
> -                       return -ENOMEM;
> -       }
> -
>         /*
>          * find index of new parent clock using cached parent ptrs,
>          * or if not yet cached, use string name comparison and cache
> @@ -1711,18 +1704,11 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
>         }
>
>         /*
> -        * Do our best to cache parent clocks in core->parents.  This prevents
> -        * unnecessary and expensive lookups.  We don't set core->parent here;
> -        * that is done by the calling function.
> +        * We don't set core->parent here; that is done by the calling function.
>          */
>
>         index = core->ops->get_parent(core->hw);
>
> -       if (!core->parents)
> -               core->parents =
> -                       kcalloc(core->num_parents, sizeof(struct clk *),
> -                                       GFP_KERNEL);
> -
>         ret = clk_core_get_parent_by_index(core, index);
>
>  out:
> @@ -2374,8 +2360,8 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
>          * look-ups of clk's possible parents.
>          */
>         if (core->num_parents > 1) {
> -               core->parents = kcalloc(core->num_parents, sizeof(struct clk *),
> -                                       GFP_KERNEL);
> +               core->parents = kcalloc(core->num_parents,
> +                                       sizeof(struct clk_core *), GFP_KERNEL);
>                 if (!core->parents) {
>                         ret = -ENOMEM;
>                         goto out;


You are doing the similar thing as mine.

See
https://patchwork.kernel.org/patch/7925571/


This series gives a big conflict with my clean-up series,
which Michael said he would apply after v4.5-rc1.

See
http://www.serverphorums.com/read.php?12,1376630


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 3/3] clk: remove redundant clock parents lookup table allocations

[Vladimir Zapolskiy]

Hi Masahiro,

On 07.01.2016 11:00, Masahiro Yamada wrote:
> Hi Vladimir,
> 
> 
> 2016-01-07 12:16 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>> Since clock parents lookup table for clocks with multiple parent
>> clocks is always allocated during clock registration, remove similar
>> allocations from __clk_init_parent() and clk_fetch_parent_index().
>>
>> The change also corrects a pointer type of a single lookup table
>> entry on calculation of the lookup table size.
>>
>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>> ---
>>  drivers/clk/clk.c | 20 +++-----------------
>>  1 file changed, 3 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 2fb0dae..f8872f9 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1067,13 +1067,6 @@ static int clk_fetch_parent_index(struct clk_core *core,
>>  {
>>         int i;
>>
>> -       if (!core->parents) {
>> -               core->parents = kcalloc(core->num_parents,
>> -                                       sizeof(struct clk *), GFP_KERNEL);
>> -               if (!core->parents)
>> -                       return -ENOMEM;
>> -       }
>> -
>>         /*
>>          * find index of new parent clock using cached parent ptrs,
>>          * or if not yet cached, use string name comparison and cache
>> @@ -1711,18 +1704,11 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
>>         }
>>
>>         /*
>> -        * Do our best to cache parent clocks in core->parents.  This prevents
>> -        * unnecessary and expensive lookups.  We don't set core->parent here;
>> -        * that is done by the calling function.
>> +        * We don't set core->parent here; that is done by the calling function.
>>          */
>>
>>         index = core->ops->get_parent(core->hw);
>>
>> -       if (!core->parents)
>> -               core->parents =
>> -                       kcalloc(core->num_parents, sizeof(struct clk *),
>> -                                       GFP_KERNEL);
>> -
>>         ret = clk_core_get_parent_by_index(core, index);
>>
>>  out:
>> @@ -2374,8 +2360,8 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
>>          * look-ups of clk's possible parents.
>>          */
>>         if (core->num_parents > 1) {
>> -               core->parents = kcalloc(core->num_parents, sizeof(struct clk *),
>> -                                       GFP_KERNEL);
>> +               core->parents = kcalloc(core->num_parents,
>> +                                       sizeof(struct clk_core *), GFP_KERNEL);
>>                 if (!core->parents) {
>>                         ret = -ENOMEM;
>>                         goto out;
> 
> 
> You are doing the similar thing as mine.
> 
> See
> https://patchwork.kernel.org/patch/7925571/

Ok, I was not aware of your work, unfortunately.

> 
> This series gives a big conflict with my clean-up series,
> which Michael said he would apply after v4.5-rc1.
> 
> See
> http://www.serverphorums.com/read.php?12,1376630
> 
> 

No problem, of course your work goes first.

Obviously I have to review your changes more carefully, but do you
have something similar to my 1/3? If no, then it is a regression.

---
With best wishes,
Vladimir

Re: [PATCH 3/3] clk: remove redundant clock parents lookup table allocations

[Masahiro Yamada]

Hi Vladimir,

2016-01-07 19:44 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
> Hi Masahiro,
>
> On 07.01.2016 11:00, Masahiro Yamada wrote:
>> Hi Vladimir,
>>
>>
>> 2016-01-07 12:16 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>>> Since clock parents lookup table for clocks with multiple parent
>>> clocks is always allocated during clock registration, remove similar
>>> allocations from __clk_init_parent() and clk_fetch_parent_index().
>>>
>>> The change also corrects a pointer type of a single lookup table
>>> entry on calculation of the lookup table size.
>>>
>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>> ---
>>>  drivers/clk/clk.c | 20 +++-----------------
>>>  1 file changed, 3 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>> index 2fb0dae..f8872f9 100644
>>> --- a/drivers/clk/clk.c
>>> +++ b/drivers/clk/clk.c
>>> @@ -1067,13 +1067,6 @@ static int clk_fetch_parent_index(struct clk_core *core,
>>>  {
>>>         int i;
>>>
>>> -       if (!core->parents) {
>>> -               core->parents = kcalloc(core->num_parents,
>>> -                                       sizeof(struct clk *), GFP_KERNEL);
>>> -               if (!core->parents)
>>> -                       return -ENOMEM;
>>> -       }
>>> -
>>>         /*
>>>          * find index of new parent clock using cached parent ptrs,
>>>          * or if not yet cached, use string name comparison and cache
>>> @@ -1711,18 +1704,11 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
>>>         }
>>>
>>>         /*
>>> -        * Do our best to cache parent clocks in core->parents.  This prevents
>>> -        * unnecessary and expensive lookups.  We don't set core->parent here;
>>> -        * that is done by the calling function.
>>> +        * We don't set core->parent here; that is done by the calling function.
>>>          */
>>>
>>>         index = core->ops->get_parent(core->hw);
>>>
>>> -       if (!core->parents)
>>> -               core->parents =
>>> -                       kcalloc(core->num_parents, sizeof(struct clk *),
>>> -                                       GFP_KERNEL);
>>> -
>>>         ret = clk_core_get_parent_by_index(core, index);
>>>
>>>  out:
>>> @@ -2374,8 +2360,8 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
>>>          * look-ups of clk's possible parents.
>>>          */
>>>         if (core->num_parents > 1) {
>>> -               core->parents = kcalloc(core->num_parents, sizeof(struct clk *),
>>> -                                       GFP_KERNEL);
>>> +               core->parents = kcalloc(core->num_parents,
>>> +                                       sizeof(struct clk_core *), GFP_KERNEL);
>>>                 if (!core->parents) {
>>>                         ret = -ENOMEM;
>>>                         goto out;
>>
>>
>> You are doing the similar thing as mine.
>>
>> See
>> https://patchwork.kernel.org/patch/7925571/
>
> Ok, I was not aware of your work, unfortunately.
>
>>
>> This series gives a big conflict with my clean-up series,
>> which Michael said he would apply after v4.5-rc1.
>>
>> See
>> http://www.serverphorums.com/read.php?12,1376630
>>
>>
>
> No problem, of course your work goes first.
>
> Obviously I have to review your changes more carefully, but do you
> have something similar to my 1/3? If no, then it is a regression.


No.

I solved the problem in a different way.
https://patchwork.kernel.org/patch/7925571/

In my way, core->parents is always allocated.
1/3 is not needed.

If I am wrong, please point out.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 3/3] clk: remove redundant clock parents lookup table allocations

[Vladimir Zapolskiy]

Hi Masahiro,

On 07.01.2016 13:33, Masahiro Yamada wrote:
> Hi Vladimir,
> 
> 2016-01-07 19:44 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>> Hi Masahiro,
>>
>> On 07.01.2016 11:00, Masahiro Yamada wrote:
>>> Hi Vladimir,
>>>
>>>
>>> 2016-01-07 12:16 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>>>> Since clock parents lookup table for clocks with multiple parent
>>>> clocks is always allocated during clock registration, remove similar
>>>> allocations from __clk_init_parent() and clk_fetch_parent_index().
>>>>
>>>> The change also corrects a pointer type of a single lookup table
>>>> entry on calculation of the lookup table size.
>>>>
>>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>>> ---
>>>>  drivers/clk/clk.c | 20 +++-----------------
>>>>  1 file changed, 3 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>>> index 2fb0dae..f8872f9 100644
>>>> --- a/drivers/clk/clk.c
>>>> +++ b/drivers/clk/clk.c
>>>> @@ -1067,13 +1067,6 @@ static int clk_fetch_parent_index(struct clk_core *core,
>>>>  {
>>>>         int i;
>>>>
>>>> -       if (!core->parents) {
>>>> -               core->parents = kcalloc(core->num_parents,
>>>> -                                       sizeof(struct clk *), GFP_KERNEL);
>>>> -               if (!core->parents)
>>>> -                       return -ENOMEM;
>>>> -       }
>>>> -
>>>>         /*
>>>>          * find index of new parent clock using cached parent ptrs,
>>>>          * or if not yet cached, use string name comparison and cache
>>>> @@ -1711,18 +1704,11 @@ static struct clk_core *__clk_init_parent(struct clk_core *core)
>>>>         }
>>>>
>>>>         /*
>>>> -        * Do our best to cache parent clocks in core->parents.  This prevents
>>>> -        * unnecessary and expensive lookups.  We don't set core->parent here;
>>>> -        * that is done by the calling function.
>>>> +        * We don't set core->parent here; that is done by the calling function.
>>>>          */
>>>>
>>>>         index = core->ops->get_parent(core->hw);
>>>>
>>>> -       if (!core->parents)
>>>> -               core->parents =
>>>> -                       kcalloc(core->num_parents, sizeof(struct clk *),
>>>> -                                       GFP_KERNEL);
>>>> -
>>>>         ret = clk_core_get_parent_by_index(core, index);
>>>>
>>>>  out:
>>>> @@ -2374,8 +2360,8 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
>>>>          * look-ups of clk's possible parents.
>>>>          */
>>>>         if (core->num_parents > 1) {
>>>> -               core->parents = kcalloc(core->num_parents, sizeof(struct clk *),
>>>> -                                       GFP_KERNEL);
>>>> +               core->parents = kcalloc(core->num_parents,
>>>> +                                       sizeof(struct clk_core *), GFP_KERNEL);
>>>>                 if (!core->parents) {
>>>>                         ret = -ENOMEM;
>>>>                         goto out;
>>>
>>>
>>> You are doing the similar thing as mine.
>>>
>>> See
>>> https://patchwork.kernel.org/patch/7925571/
>>
>> Ok, I was not aware of your work, unfortunately.
>>
>>>
>>> This series gives a big conflict with my clean-up series,
>>> which Michael said he would apply after v4.5-rc1.
>>>
>>> See
>>> http://www.serverphorums.com/read.php?12,1376630
>>>
>>>
>>
>> No problem, of course your work goes first.
>>
>> Obviously I have to review your changes more carefully, but do you
>> have something similar to my 1/3? If no, then it is a regression.
> 
> 
> No.
> 
> I solved the problem in a different way.
> https://patchwork.kernel.org/patch/7925571/
> 
> In my way, core->parents is always allocated.

what's about the case of root clocks, when (core->num_parents == 0) ?

Yours unconditional

@@ -2560,12 +2537,20 @@  struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		}
 	}
 
+	/* avoid unnecessary string look-ups of clk_core's possible parents. */
+	core->parents = kcalloc(core->num_parents, sizeof(*core->parents),
+				GFP_KERNEL);
+	if (!core->parents) {
+		ret = -ENOMEM;
+		goto fail_parents;
+	};
+

looks suspicious, but I have to test and review the background changes as well,
most probably I'm missing something.

> 1/3 is not needed.

Actually 1/3 solves a bit different kind of problem, but again I have to
review/test your series.

Shortly you may test it beforehand by a fuzz testing -- see the details
in 1/3 commit message, you may insert a known in advance wrong

  ret = clk_set_parent(clk, parent);

and get the result.

> If I am wrong, please point out.
> 
> 

I'll test and review your series today later on or tomorrow more carefully.

--
With best wishes,
Vladimir

Re: [PATCH 0/3] clk: remove dead code allocations of parent lookup tables

[Vladimir Zapolskiy]

On 07.01.2016 05:16, Vladimir Zapolskiy wrote:
> The change simplifies core operations over multi-parent clocks (clock
> registration, clk_set_parent() and clk_get_parent() internals) by
> rejecting a possibilty to skip failed memory allocation during clock
> registration time and falling back to it on the next [gs]et_parent.
> 
> No functional change is intended.
> 
> Vladimir Zapolskiy (3):
>   clk: don't fetch parent index for non multi-parent clocks
>   clk: simplify array allocation of clock parents for lookup
>   clk: remove redundant clock parents lookup table allocations
> 
>  drivers/clk/clk.c | 49 +++++++++++++++++++------------------------------
>  1 file changed, 19 insertions(+), 30 deletions(-)
> 

Please ignore this series, the discussed Masahiro's changeset completely
covers the proposed improvements (and it adds even more of them).

--
With best wishes,
Vladimir