[PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov]

The  pin array handled by sh_pfc_map_pins() may contain holes representing non-
existing pins. We have to first count the valid pins in order to calculate the
size  of the memory to be allocated, then to skip over the non-existing pins
when  initializing the allocated arrays, and then to return the number of valid
pins  from sh_pfc_map_pins() instead of 0 on success.

As we have to touch devm_kzalloc() calls anyway, use more fitting devm_kcalloc()
instead which additionally checks the array size. And since PINMUX_TYPE_NONE is
#define'd as 0, stop re-initializing already zeroed out 'pmx->configs' array.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
The patch is against the 'devel' branch of Linus W.'s 'linux-pinctrl.git' repo.
This patch should be applied before my R8A7794 PFC support patch and before
Laurent's patches removing non-existent GPIOs for R8A779[01], otherwise they
would cause the kernel  to hang while booting!

 drivers/pinctrl/sh-pfc/pinctrl.c |   32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
===================================================================
--- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pinctrl.c
+++ linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -571,33 +571,39 @@ static const struct pinconf_ops sh_pfc_p
 /* PFC ranges -> pinctrl pin descs */
 static int sh_pfc_map_pins(struct sh_pfc *pfc, struct sh_pfc_pinctrl *pmx)
 {
-	unsigned int i;
+	const struct sh_pfc_pin *info;
+	struct pinctrl_pin_desc *pin;
+	unsigned int i, n;
+
+	/* Count the valid pins. */
+	for (i = 0, info = pfc->info->pins, n = 0;
+	     i < pfc->info->nr_pins; i++, info++) {
+		if (info->enum_id || info->configs)
+			n++;
+	}
 
 	/* Allocate and initialize the pins and configs arrays. */
-	pmx->pins = devm_kzalloc(pfc->dev,
-				 sizeof(*pmx->pins) * pfc->info->nr_pins,
-				 GFP_KERNEL);
+	pmx->pins = devm_kcalloc(pfc->dev, n, sizeof(*pmx->pins), GFP_KERNEL);
 	if (unlikely(!pmx->pins))
 		return -ENOMEM;
 
-	pmx->configs = devm_kzalloc(pfc->dev,
-				    sizeof(*pmx->configs) * pfc->info->nr_pins,
+	pmx->configs = devm_kcalloc(pfc->dev, n, sizeof(*pmx->configs),
 				    GFP_KERNEL);
 	if (unlikely(!pmx->configs))
 		return -ENOMEM;
 
-	for (i = 0; i < pfc->info->nr_pins; ++i) {
-		const struct sh_pfc_pin *info = &pfc->info->pins[i];
-		struct sh_pfc_pin_config *cfg = &pmx->configs[i];
-		struct pinctrl_pin_desc *pin = &pmx->pins[i];
+	for (i = 0, info = pfc->info->pins, pin = pmx->pins;
+	     i < pfc->info->nr_pins; i++, info++) {
+		if (!info->enum_id && !info->configs)
+			continue;
 
 		/* If the pin number is equal to -1 all pins are considered */
 		pin->number = info->pin != (u16)-1 ? info->pin : i;
 		pin->name = info->name;
-		cfg->type = PINMUX_TYPE_NONE;
+		pin++;
 	}
 
-	return 0;
+	return n;
 }
 
 int sh_pfc_register_pinctrl(struct sh_pfc *pfc)
@@ -622,7 +628,7 @@ int sh_pfc_register_pinctrl(struct sh_pf
 	pmx->pctl_desc.pmxops = &sh_pfc_pinmux_ops;
 	pmx->pctl_desc.confops = &sh_pfc_pinconf_ops;
 	pmx->pctl_desc.pins = pmx->pins;
-	pmx->pctl_desc.npins = pfc->info->nr_pins;
+	pmx->pctl_desc.npins = ret;
 
 	pmx->pctl = pinctrl_register(&pmx->pctl_desc, pfc->dev, pmx);
 	if (pmx->pctl == NULL)

Re: [PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Laurent Pinchart]

Hi Sergei,

Thank you for the patch.

On Saturday 28 February 2015 02:39:19 Sergei Shtylyov wrote:
> The  pin array handled by sh_pfc_map_pins() may contain holes representing
> non- existing pins. We have to first count the valid pins in order to
> calculate the size  of the memory to be allocated, then to skip over the
> non-existing pins when  initializing the allocated arrays, and then to
> return the number of valid pins  from sh_pfc_map_pins() instead of 0 on
> success.
> 
> As we have to touch devm_kzalloc() calls anyway, use more fitting
> devm_kcalloc() instead which additionally checks the array size. And since
> PINMUX_TYPE_NONE is #define'd as 0, stop re-initializing already zeroed out
> 'pmx->configs' array.

I agree with this optimization, but I think it deserves a comment in the 
source code that explicitly mentions PINMUX_TYPE_NONE, to make sure any later 
rework changing the PINMUX_TYPE_NONE value would catch this.

> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> The patch is against the 'devel' branch of Linus W.'s 'linux-pinctrl.git'
> repo. This patch should be applied before my R8A7794 PFC support patch and
> before Laurent's patches removing non-existent GPIOs for R8A779[01],
> otherwise they would cause the kernel  to hang while booting!
> 
>  drivers/pinctrl/sh-pfc/pinctrl.c |   32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
> ===================================================================
> --- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pinctrl.c
> +++ linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
> @@ -571,33 +571,39 @@ static const struct pinconf_ops sh_pfc_p
>  /* PFC ranges -> pinctrl pin descs */
>  static int sh_pfc_map_pins(struct sh_pfc *pfc, struct sh_pfc_pinctrl *pmx)
>  {
> -	unsigned int i;
> +	const struct sh_pfc_pin *info;
> +	struct pinctrl_pin_desc *pin;
> +	unsigned int i, n;

Could you please rename n to num_pins, and declare it on its own line to match 
the coding style of the file ?

> +
> +	/* Count the valid pins. */
> +	for (i = 0, info = pfc->info->pins, n = 0;
> +	     i < pfc->info->nr_pins; i++, info++) {
> +		if (info->enum_id || info->configs)

Why do you need to test info->configs as well ? I thought enum_id == 0 is 
always reserved, am I getting it wrong ?

> +			n++;
> +	}
> 
>  	/* Allocate and initialize the pins and configs arrays. */
> -	pmx->pins = devm_kzalloc(pfc->dev,
> -				 sizeof(*pmx->pins) * pfc->info->nr_pins,
> -				 GFP_KERNEL);
> +	pmx->pins = devm_kcalloc(pfc->dev, n, sizeof(*pmx->pins), GFP_KERNEL);
>  	if (unlikely(!pmx->pins))
>  		return -ENOMEM;
> 
> -	pmx->configs = devm_kzalloc(pfc->dev,
> -				    sizeof(*pmx->configs) * pfc->info->nr_pins,
> +	pmx->configs = devm_kcalloc(pfc->dev, n, sizeof(*pmx->configs),
>  				    GFP_KERNEL);
>  	if (unlikely(!pmx->configs))
>  		return -ENOMEM;
> 
> -	for (i = 0; i < pfc->info->nr_pins; ++i) {
> -		const struct sh_pfc_pin *info = &pfc->info->pins[i];
> -		struct sh_pfc_pin_config *cfg = &pmx->configs[i];
> -		struct pinctrl_pin_desc *pin = &pmx->pins[i];
> +	for (i = 0, info = pfc->info->pins, pin = pmx->pins;
> +	     i < pfc->info->nr_pins; i++, info++) {

I would keep info as a local variable to avoid splitting the for () on 
multiple lines. Same comment for the counter loop above.

> +		if (!info->enum_id && !info->configs)
> +			continue;
> 
>  		/* If the pin number is equal to -1 all pins are considered */
>  		pin->number = info->pin != (u16)-1 ? info->pin : i;
>  		pin->name = info->name;
> -		cfg->type = PINMUX_TYPE_NONE;
> +		pin++;
>  	}
> 
> -	return 0;
> +	return n;
>  }
> 
>  int sh_pfc_register_pinctrl(struct sh_pfc *pfc)
> @@ -622,7 +628,7 @@ int sh_pfc_register_pinctrl(struct sh_pf
>  	pmx->pctl_desc.pmxops = &sh_pfc_pinmux_ops;
>  	pmx->pctl_desc.confops = &sh_pfc_pinconf_ops;
>  	pmx->pctl_desc.pins = pmx->pins;
> -	pmx->pctl_desc.npins = pfc->info->nr_pins;
> +	pmx->pctl_desc.npins = ret;
> 
>  	pmx->pctl = pinctrl_register(&pmx->pctl_desc, pfc->dev, pmx);
>  	if (pmx->pctl == NULL)

Shouldn't you also fix sh_pfc_init_ranges() ? It includes the following code 
that doesn't seem to handle holes properly.

        for (i = 1, nr_ranges = 1; i < pfc->info->nr_pins; ++i) {
                if (pfc->info->pins[i-1].pin != pfc->info->pins[i].pin - 1)
                        nr_ranges++;
        }

Please make sure that sh_pfc_get_pin_index() doesn't start blowing up 
afterwards though.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Linus Walleij]

On Sat, Feb 28, 2015 at 12:39 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:

> The  pin array handled by sh_pfc_map_pins() may contain holes representing non-
> existing pins. We have to first count the valid pins in order to calculate the
> size  of the memory to be allocated, then to skip over the non-existing pins
> when  initializing the allocated arrays, and then to return the number of valid
> pins  from sh_pfc_map_pins() instead of 0 on success.
>
> As we have to touch devm_kzalloc() calls anyway, use more fitting devm_kcalloc()
> instead which additionally checks the array size. And since PINMUX_TYPE_NONE is
> #define'd as 0, stop re-initializing already zeroed out 'pmx->configs' array.
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> ---
> The patch is against the 'devel' branch of Linus W.'s 'linux-pinctrl.git' repo.
> This patch should be applied before my R8A7794 PFC support patch and before
> Laurent's patches removing non-existent GPIOs for R8A779[01], otherwise they
> would cause the kernel  to hang while booting!

OK not applying this until ACKed by Laurent,
and in the meantime I'm taking the three applied R8A7794 patches
out of my tree again.

Can you please send these depending patches as a series with this
patch as 1/4 and the three R8A7794 patches as 2,3,4/4? Thanks.

Yours,
Linus Walleij

Re: [PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Laurent Pinchart]

Hi Linus,

On Friday 06 March 2015 11:43:45 Linus Walleij wrote:
> On Sat, Feb 28, 2015 at 12:39 AM, Sergei Shtylyov wrote:
> > The  pin array handled by sh_pfc_map_pins() may contain holes representing
> > non- existing pins. We have to first count the valid pins in order to
> > calculate the size  of the memory to be allocated, then to skip over the
> > non-existing pins when  initializing the allocated arrays, and then to
> > return the number of valid pins  from sh_pfc_map_pins() instead of 0 on
> > success.
> > 
> > As we have to touch devm_kzalloc() calls anyway, use more fitting
> > devm_kcalloc() instead which additionally checks the array size. And
> > since PINMUX_TYPE_NONE is #define'd as 0, stop re-initializing already
> > zeroed out 'pmx->configs' array.
> > 
> > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> > 
> > ---
> > The patch is against the 'devel' branch of Linus W.'s 'linux-pinctrl.git'
> > repo. This patch should be applied before my R8A7794 PFC support patch
> > and before Laurent's patches removing non-existent GPIOs for R8A779[01],
> > otherwise they would cause the kernel  to hang while booting!
> 
> OK not applying this until ACKed by Laurent,
> and in the meantime I'm taking the three applied R8A7794 patches
> out of my tree again.

Could you also drop "pinctrl: sh-pfc: r8a7790: Remove non existing GPIO pins" 
and "pinctrl: sh-pfc: r8a7791: Remove non existing GPIO pins" for the same 
reason ? Sorry for the trouble.

> Can you please send these depending patches as a series with this
> patch as 1/4 and the three R8A7794 patches as 2,3,4/4? Thanks.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov]

On 3/6/2015 1:43 PM, Linus Walleij wrote:

>> The  pin array handled by sh_pfc_map_pins() may contain holes representing non-
>> existing pins. We have to first count the valid pins in order to calculate the
>> size  of the memory to be allocated, then to skip over the non-existing pins
>> when  initializing the allocated arrays, and then to return the number of valid
>> pins  from sh_pfc_map_pins() instead of 0 on success.

>> As we have to touch devm_kzalloc() calls anyway, use more fitting devm_kcalloc()
>> instead which additionally checks the array size. And since PINMUX_TYPE_NONE is
>> #define'd as 0, stop re-initializing already zeroed out 'pmx->configs' array.

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>> ---
>> The patch is against the 'devel' branch of Linus W.'s 'linux-pinctrl.git' repo.
>> This patch should be applied before my R8A7794 PFC support patch and before
>> Laurent's patches removing non-existent GPIOs for R8A779[01], otherwise they
>> would cause the kernel  to hang while booting!

> OK not applying this until ACKed by Laurent,
> and in the meantime I'm taking the three applied R8A7794 patches
> out of my tree again.

    Thanks!

> Can you please send these depending patches as a series with this
> patch as 1/4 and the three R8A7794 patches as 2,3,4/4? Thanks.

    OK, will try to remember. But what about Lauren't 2 patches also dependent 
on this one?

> Yours,
> Linus Walleij

WBR, Sergei

Re: [PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Laurent Pinchart]

Hi Sergei,

On Friday 06 March 2015 15:42:05 Sergei Shtylyov wrote:
> On 3/6/2015 1:43 PM, Linus Walleij wrote:
> >> The  pin array handled by sh_pfc_map_pins() may contain holes
> >> representing non- existing pins. We have to first count the valid pins
> >> in order to calculate the size  of the memory to be allocated, then to
> >> skip over the non-existing pins when  initializing the allocated arrays,
> >> and then to return the number of valid pins  from sh_pfc_map_pins()
> >> instead of 0 on success.
> >> 
> >> As we have to touch devm_kzalloc() calls anyway, use more fitting
> >> devm_kcalloc() instead which additionally checks the array size. And
> >> since PINMUX_TYPE_NONE is #define'd as 0, stop re-initializing already
> >> zeroed out 'pmx->configs' array.
> >> 
> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >> 
> >> ---
> >> The patch is against the 'devel' branch of Linus W.'s 'linux-pinctrl.git'
> >> repo. This patch should be applied before my R8A7794 PFC support patch
> >> and before Laurent's patches removing non-existent GPIOs for R8A779[01],
> >> otherwise they would cause the kernel  to hang while booting!
> > 
> > OK not applying this until ACKed by Laurent,
> > and in the meantime I'm taking the three applied R8A7794 patches
> > out of my tree again.
> 
> Thanks!
> 
> > Can you please send these depending patches as a series with this
> > patch as 1/4 and the three R8A7794 patches as 2,3,4/4? Thanks.
> 
> OK, will try to remember. But what about Lauren't 2 patches also
> dependent on this one?

If you don't mind, please also include them in your series.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Linus Walleij]

On Fri, Mar 6, 2015 at 11:58 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Linus,
>
> On Friday 06 March 2015 11:43:45 Linus Walleij wrote:
>> On Sat, Feb 28, 2015 at 12:39 AM, Sergei Shtylyov wrote:
>> > The  pin array handled by sh_pfc_map_pins() may contain holes representing
>> > non- existing pins. We have to first count the valid pins in order to
>> > calculate the size  of the memory to be allocated, then to skip over the
>> > non-existing pins when  initializing the allocated arrays, and then to
>> > return the number of valid pins  from sh_pfc_map_pins() instead of 0 on
>> > success.
>> >
>> > As we have to touch devm_kzalloc() calls anyway, use more fitting
>> > devm_kcalloc() instead which additionally checks the array size. And
>> > since PINMUX_TYPE_NONE is #define'd as 0, stop re-initializing already
>> > zeroed out 'pmx->configs' array.
>> >
>> > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> >
>> > ---
>> > The patch is against the 'devel' branch of Linus W.'s 'linux-pinctrl.git'
>> > repo. This patch should be applied before my R8A7794 PFC support patch
>> > and before Laurent's patches removing non-existent GPIOs for R8A779[01],
>> > otherwise they would cause the kernel  to hang while booting!
>>
>> OK not applying this until ACKed by Laurent,
>> and in the meantime I'm taking the three applied R8A7794 patches
>> out of my tree again.
>
> Could you also drop "pinctrl: sh-pfc: r8a7790: Remove non existing GPIO pins"
> and "pinctrl: sh-pfc: r8a7791: Remove non existing GPIO pins" for the same
> reason ? Sorry for the trouble.

OK dropped them (rebased .... argh hope noone will kick my ass)

Yours,
Linus Walleij

Re: [PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov]

Hello.

On 03/03/2015 03:24 AM, Laurent Pinchart wrote:

    Sorry for the belated reply, I was switched to EtherAVB soon after posting 
this patch. :-)

>> The  pin array handled by sh_pfc_map_pins() may contain holes representing
>> non- existing pins. We have to first count the valid pins in order to
>> calculate the size  of the memory to be allocated, then to skip over the
>> non-existing pins when  initializing the allocated arrays, and then to
>> return the number of valid pins  from sh_pfc_map_pins() instead of 0 on
>> success.

>> As we have to touch devm_kzalloc() calls anyway, use more fitting
>> devm_kcalloc() instead which additionally checks the array size. And since
>> PINMUX_TYPE_NONE is #define'd as 0, stop re-initializing already zeroed out
>> 'pmx->configs' array.

> I agree with this optimization, but I think it deserves a comment in the
> source code that explicitly mentions PINMUX_TYPE_NONE, to make sure any later
> rework changing the PINMUX_TYPE_NONE value would catch this.

    OK, will do.

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>> ---
>> The patch is against the 'devel' branch of Linus W.'s 'linux-pinctrl.git'
>> repo. This patch should be applied before my R8A7794 PFC support patch and
>> before Laurent's patches removing non-existent GPIOs for R8A779[01],
>> otherwise they would cause the kernel  to hang while booting!

>>   drivers/pinctrl/sh-pfc/pinctrl.c |   32 +++++++++++++++++++-------------
>>   1 file changed, 19 insertions(+), 13 deletions(-)

>> Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
>> ===================================================================
>> --- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pinctrl.c
>> +++ linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c
>> @@ -571,33 +571,39 @@ static const struct pinconf_ops sh_pfc_p
>>   /* PFC ranges -> pinctrl pin descs */
>>   static int sh_pfc_map_pins(struct sh_pfc *pfc, struct sh_pfc_pinctrl *pmx)
>>   {
>> -	unsigned int i;
>> +	const struct sh_pfc_pin *info;
>> +	struct pinctrl_pin_desc *pin;
>> +	unsigned int i, n;

> Could you please rename n to num_pins, and declare it on its own line to match
> the coding style of the file ?

    Will do.

>> +
>> +	/* Count the valid pins. */
>> +	for (i = 0, info = pfc->info->pins, n = 0;
>> +	     i < pfc->info->nr_pins; i++, info++) {
>> +		if (info->enum_id || info->configs)

> Why do you need to test info->configs as well ? I thought enum_id == 0 is
> always reserved, am I getting it wrong ?

    Look at SH_PFC_PIN_NAMED() and its users.

>> +			n++;
>> +	}
>>
>>   	/* Allocate and initialize the pins and configs arrays. */
>> -	pmx->pins = devm_kzalloc(pfc->dev,
>> -				 sizeof(*pmx->pins) * pfc->info->nr_pins,
>> -				 GFP_KERNEL);
>> +	pmx->pins = devm_kcalloc(pfc->dev, n, sizeof(*pmx->pins), GFP_KERNEL);
>>   	if (unlikely(!pmx->pins))
>>   		return -ENOMEM;
>>
>> -	pmx->configs = devm_kzalloc(pfc->dev,
>> -				    sizeof(*pmx->configs) * pfc->info->nr_pins,
>> +	pmx->configs = devm_kcalloc(pfc->dev, n, sizeof(*pmx->configs),
>>   				    GFP_KERNEL);
>>   	if (unlikely(!pmx->configs))
>>   		return -ENOMEM;
>>
>> -	for (i = 0; i < pfc->info->nr_pins; ++i) {
>> -		const struct sh_pfc_pin *info = &pfc->info->pins[i];
>> -		struct sh_pfc_pin_config *cfg = &pmx->configs[i];
>> -		struct pinctrl_pin_desc *pin = &pmx->pins[i];
>> +	for (i = 0, info = pfc->info->pins, pin = pmx->pins;
>> +	     i < pfc->info->nr_pins; i++, info++) {

> I would keep info as a local variable to avoid splitting the for () on
> multiple lines. Same comment for the counter loop above.

    I don't want to declare the same variable twice, so prefer doing gcc's 
work of optimizing loop induction variable myself in this case. If you insist, 
this can be changed...

[...]
>> @@ -622,7 +628,7 @@ int sh_pfc_register_pinctrl(struct sh_pf
>>   	pmx->pctl_desc.pmxops = &sh_pfc_pinmux_ops;
>>   	pmx->pctl_desc.confops = &sh_pfc_pinconf_ops;
>>   	pmx->pctl_desc.pins = pmx->pins;
>> -	pmx->pctl_desc.npins = pfc->info->nr_pins;
>> +	pmx->pctl_desc.npins = ret;
>>
>>   	pmx->pctl = pinctrl_register(&pmx->pctl_desc, pfc->dev, pmx);
>>   	if (pmx->pctl == NULL)

> Shouldn't you also fix sh_pfc_init_ranges() ? It includes the following code
> that doesn't seem to handle holes properly.

>          for (i = 1, nr_ranges = 1; i < pfc->info->nr_pins; ++i) {
>                  if (pfc->info->pins[i-1].pin != pfc->info->pins[i].pin - 1)
>                          nr_ranges++;
>          }

    Sorry, missed it, will try to deal with this as well...

> Please make sure that sh_pfc_get_pin_index() doesn't start blowing up
> afterwards though.

    Will do.

WBR, Sergei

Re: [PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov]

Hello.

On 03/03/2015 03:24 AM, Laurent Pinchart wrote:

>> The  pin array handled by sh_pfc_map_pins() may contain holes representing
>> non- existing pins. We have to first count the valid pins in order to
>> calculate the size  of the memory to be allocated, then to skip over the
>> non-existing pins when  initializing the allocated arrays, and then to
>> return the number of valid pins  from sh_pfc_map_pins() instead of 0 on
>> success.
>>
>> As we have to touch devm_kzalloc() calls anyway, use more fitting
>> devm_kcalloc() instead which additionally checks the array size. And since
>> PINMUX_TYPE_NONE is #define'd as 0, stop re-initializing already zeroed out
>> 'pmx->configs' array.

> I agree with this optimization, but I think it deserves a comment in the
> source code that explicitly mentions PINMUX_TYPE_NONE, to make sure any later
> rework changing the PINMUX_TYPE_NONE value would catch this.

    Note that this is not just "drove by" optimization, I was trying to avoid
the very need to explicitly initialize 'pmx->configs' array to PINMUX_TYPE_NONE...

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

[...]

>> @@ -622,7 +628,7 @@ int sh_pfc_register_pinctrl(struct sh_pf
>>   	pmx->pctl_desc.pmxops = &sh_pfc_pinmux_ops;
>>   	pmx->pctl_desc.confops = &sh_pfc_pinconf_ops;
>>   	pmx->pctl_desc.pins = pmx->pins;
>> -	pmx->pctl_desc.npins = pfc->info->nr_pins;
>> +	pmx->pctl_desc.npins = ret;
>>
>>   	pmx->pctl = pinctrl_register(&pmx->pctl_desc, pfc->dev, pmx);
>>   	if (pmx->pctl == NULL)

> Shouldn't you also fix sh_pfc_init_ranges() ? It includes the following code
> that doesn't seem to handle holes properly.

>          for (i = 1, nr_ranges = 1; i < pfc->info->nr_pins; ++i) {
>                  if (pfc->info->pins[i-1].pin != pfc->info->pins[i].pin - 1)
>                          nr_ranges++;
>          }

> Please make sure that sh_pfc_get_pin_index() doesn't start blowing up
> afterwards though.

    I think I'm seeing a problem with this function (and it's not due to my 
changes). Its result is often used to index 'pfc->info->pins' array. While 
this is working now, when the holes are not yet allowed, it's going to break 
when we start supporting the holes since that array couldn't be indexed this 
way anymore (via ranges). This function is good only for 'pmx->configs' and 
the like where we have already removed the holes. It looks like this holes 
thing is going to be too complex, so how about leaving things as they are? :-)

WBR, Sergei

Re: [PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Geert Uytterhoeven]

On Thu, Apr 30, 2015 at 12:38 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 03/03/2015 03:24 AM, Laurent Pinchart wrote:
>>> The  pin array handled by sh_pfc_map_pins() may contain holes
>>> representing
>>> non- existing pins. We have to first count the valid pins in order to
>>> calculate the size  of the memory to be allocated, then to skip over the
>>> non-existing pins when  initializing the allocated arrays, and then to
>>> return the number of valid pins  from sh_pfc_map_pins() instead of 0 on
>>> success.
>>>
>>> As we have to touch devm_kzalloc() calls anyway, use more fitting
>>> devm_kcalloc() instead which additionally checks the array size. And
>>> since
>>> PINMUX_TYPE_NONE is #define'd as 0, stop re-initializing already zeroed
>>> out
>>> 'pmx->configs' array.
>
>
>> I agree with this optimization, but I think it deserves a comment in the
>> source code that explicitly mentions PINMUX_TYPE_NONE, to make sure any
>> later
>> rework changing the PINMUX_TYPE_NONE value would catch this.
>
>
>    Note that this is not just "drove by" optimization, I was trying to avoid
> the very need to explicitly initialize 'pmx->configs' array to
> PINMUX_TYPE_NONE...
>
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
>
> [...]
>
>>> @@ -622,7 +628,7 @@ int sh_pfc_register_pinctrl(struct sh_pf
>>>         pmx->pctl_desc.pmxops = &sh_pfc_pinmux_ops;
>>>         pmx->pctl_desc.confops = &sh_pfc_pinconf_ops;
>>>         pmx->pctl_desc.pins = pmx->pins;
>>> -       pmx->pctl_desc.npins = pfc->info->nr_pins;
>>> +       pmx->pctl_desc.npins = ret;
>>>
>>>         pmx->pctl = pinctrl_register(&pmx->pctl_desc, pfc->dev, pmx);
>>>         if (pmx->pctl == NULL)
>
>
>> Shouldn't you also fix sh_pfc_init_ranges() ? It includes the following
>> code
>> that doesn't seem to handle holes properly.
>
>
>>          for (i = 1, nr_ranges = 1; i < pfc->info->nr_pins; ++i) {
>>                  if (pfc->info->pins[i-1].pin != pfc->info->pins[i].pin -
>> 1)
>>                          nr_ranges++;
>>          }
>
>
>> Please make sure that sh_pfc_get_pin_index() doesn't start blowing up
>> afterwards though.
>
>
>    I think I'm seeing a problem with this function (and it's not due to my
> changes). Its result is often used to index 'pfc->info->pins' array. While
> this is working now, when the holes are not yet allowed, it's going to break
> when we start supporting the holes since that array couldn't be indexed this
> way anymore (via ranges). This function is good only for 'pmx->configs' and
> the like where we have already removed the holes. It looks like this holes
> thing is going to be too complex, so how about leaving things as they are?
> :-)

Any conclusion on this?
If yes, please resend, incl. the r8a7794 pfc patches that depend on it.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] sh-pfc: handle pin array holes in sh_pfc_map_pins()

[Sergei Shtylyov]

Hello.

On 05/27/2015 11:52 AM, Geert Uytterhoeven wrote:

>>>> The  pin array handled by sh_pfc_map_pins() may contain holes
>>>> representing
>>>> non- existing pins. We have to first count the valid pins in order to
>>>> calculate the size  of the memory to be allocated, then to skip over the
>>>> non-existing pins when  initializing the allocated arrays, and then to
>>>> return the number of valid pins  from sh_pfc_map_pins() instead of 0 on
>>>> success.

>>>> As we have to touch devm_kzalloc() calls anyway, use more fitting
>>>> devm_kcalloc() instead which additionally checks the array size. And
>>>> since
>>>> PINMUX_TYPE_NONE is #define'd as 0, stop re-initializing already zeroed
>>>> out 'pmx->configs' array.

>>> I agree with this optimization, but I think it deserves a comment in the
>>> source code that explicitly mentions PINMUX_TYPE_NONE, to make sure any
>>> later
>>> rework changing the PINMUX_TYPE_NONE value would catch this.

>>     Note that this is not just "drove by" optimization, I was trying to avoid
>> the very need to explicitly initialize 'pmx->configs' array to
>> PINMUX_TYPE_NONE...

>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>> [...]

>>>> @@ -622,7 +628,7 @@ int sh_pfc_register_pinctrl(struct sh_pf
>>>>          pmx->pctl_desc.pmxops = &sh_pfc_pinmux_ops;
>>>>          pmx->pctl_desc.confops = &sh_pfc_pinconf_ops;
>>>>          pmx->pctl_desc.pins = pmx->pins;
>>>> -       pmx->pctl_desc.npins = pfc->info->nr_pins;
>>>> +       pmx->pctl_desc.npins = ret;
>>>>
>>>>          pmx->pctl = pinctrl_register(&pmx->pctl_desc, pfc->dev, pmx);
>>>>          if (pmx->pctl == NULL)

>>> Shouldn't you also fix sh_pfc_init_ranges() ? It includes the following
>>> code
>>> that doesn't seem to handle holes properly.

>>>           for (i = 1, nr_ranges = 1; i < pfc->info->nr_pins; ++i) {
>>>                   if (pfc->info->pins[i-1].pin != pfc->info->pins[i].pin -
>>> 1)
>>>                           nr_ranges++;
>>>           }

>>> Please make sure that sh_pfc_get_pin_index() doesn't start blowing up
>>> afterwards though.

>>     I think I'm seeing a problem with this function (and it's not due to my
>> changes). Its result is often used to index 'pfc->info->pins' array. While
>> this is working now, when the holes are not yet allowed, it's going to break
>> when we start supporting the holes since that array couldn't be indexed this
>> way anymore (via ranges). This function is good only for 'pmx->configs' and
>> the like where we have already removed the holes. It looks like this holes
>> thing is going to be too complex, so how about leaving things as they are?
>> :-)

> Any conclusion on this?

    I did overestimate the complexity, it seems. :-)
    I have just removed the calls to sh_pfc_get_pin_index() on 
'pfc->info->pins' and its alias.

> If yes, please resend, incl. the r8a7794 pfc patches that depend on it.

    Will repost the patches RSN.

> Gr{oetje,eeting}s,
>                          Geert

WBR, Sergei