Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning
@ 2019-05-15 13:43 Biju Das
  2019-05-16 11:13 ` Simon Horman
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Biju Das @ 2019-05-15 13:43 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Biju Das, Dan Carpenter, Wolfram Sang, Simon Horman,
	Yoshihiro Shimoda, Simon Horman, Geert Uytterhoeven,
	Chris Paterson, Fabrizio Castro, linux-renesas-soc

Fix the below smatch warning by adding variable check rather than the
hardcoded value.
warn: array off by one? 'data->select_value[channel_num]'

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 * Ref [1]
  [1]:https://www.spinics.net/lists/linux-kernel-janitors/msg46715.html
---
 drivers/phy/renesas/phy-rcar-gen2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/renesas/phy-rcar-gen2.c b/drivers/phy/renesas/phy-rcar-gen2.c
index 8dc5710..ecb42ad 100644
--- a/drivers/phy/renesas/phy-rcar-gen2.c
+++ b/drivers/phy/renesas/phy-rcar-gen2.c
@@ -71,6 +71,7 @@ struct rcar_gen2_phy_driver {
 struct rcar_gen2_phy_data {
 	const struct phy_ops *gen2_phy_ops;
 	const u32 (*select_value)[PHYS_PER_CHANNEL];
+	const u32 last_channel;
 };
 
 static int rcar_gen2_phy_init(struct phy *p)
@@ -271,11 +272,13 @@ static const u32 usb20_select_value[][PHYS_PER_CHANNEL] = {
 static const struct rcar_gen2_phy_data rcar_gen2_usb_phy_data = {
 	.gen2_phy_ops = &rcar_gen2_phy_ops,
 	.select_value = pci_select_value,
+	.last_channel = 2,
 };
 
 static const struct rcar_gen2_phy_data rz_g1c_usb_phy_data = {
 	.gen2_phy_ops = &rz_g1c_phy_ops,
 	.select_value = usb20_select_value,
+	.last_channel = 0,
 };
 
 static const struct of_device_id rcar_gen2_phy_match_table[] = {
@@ -389,7 +392,7 @@ static int rcar_gen2_phy_probe(struct platform_device *pdev)
 		channel->selected_phy = -1;
 
 		error = of_property_read_u32(np, "reg", &channel_num);
-		if (error || channel_num > 2) {
+		if (error || channel_num > data->last_channel) {
 			dev_err(dev, "Invalid \"reg\" property\n");
 			return error;
 		}
-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning
  2019-05-15 13:43 [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning Biju Das
@ 2019-05-16 11:13 ` Simon Horman
  2019-05-16 12:16 ` Wolfram Sang
  2019-05-16 12:40 ` Geert Uytterhoeven
  2 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2019-05-16 11:13 UTC (permalink / raw)
  To: Biju Das
  Cc: Kishon Vijay Abraham I, Dan Carpenter, Wolfram Sang,
	Yoshihiro Shimoda, Geert Uytterhoeven, Chris Paterson,
	Fabrizio Castro, linux-renesas-soc

On Wed, May 15, 2019 at 02:43:06PM +0100, Biju Das wrote:
> Fix the below smatch warning by adding variable check rather than the
> hardcoded value.
> warn: array off by one? 'data->select_value[channel_num]'
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning
  2019-05-15 13:43 [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning Biju Das
  2019-05-16 11:13 ` Simon Horman
@ 2019-05-16 12:16 ` Wolfram Sang
  2019-05-16 12:32   ` Biju Das
  2019-05-16 12:34   ` Geert Uytterhoeven
  2019-05-16 12:40 ` Geert Uytterhoeven
  2 siblings, 2 replies; 8+ messages in thread
From: Wolfram Sang @ 2019-05-16 12:16 UTC (permalink / raw)
  To: Biju Das
  Cc: Kishon Vijay Abraham I, Dan Carpenter, Wolfram Sang,
	Simon Horman, Yoshihiro Shimoda, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	linux-renesas-soc

[-- Attachment #1: Type: text/plain, Size: 276 bytes --]

>  		error = of_property_read_u32(np, "reg", &channel_num);
> -		if (error || channel_num > 2) {
> +		if (error || channel_num > data->last_channel) {

Just an idea, I haven't tested it: Couldn't we use
ARRAY_SIZE(data->select_value) to avoid the extra member in the struct?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning
  2019-05-16 12:16 ` Wolfram Sang
@ 2019-05-16 12:32   ` Biju Das
  2019-05-16 12:34   ` Geert Uytterhoeven
  1 sibling, 0 replies; 8+ messages in thread
From: Biju Das @ 2019-05-16 12:32 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Kishon Vijay Abraham I, Dan Carpenter, Wolfram Sang,
	Simon Horman, Yoshihiro Shimoda, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	linux-renesas-soc

Hi Wolfram,

Thanks for the feedback.

> Subject: Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one
> warning
> 
> >  		error = of_property_read_u32(np, "reg", &channel_num);
> > -		if (error || channel_num > 2) {
> > +		if (error || channel_num > data->last_channel) {
> 
> Just an idea, I haven't tested it: Couldn't we use
> ARRAY_SIZE(data->select_value) to avoid the extra member in the struct?

I checked this now. It is giving the below build errors

In file included from ./include/linux/kernel.h:16:0,
                 from ./include/linux/clk.h:16,
                 from drivers/phy/renesas/phy-rcar-gen2.c:10:
drivers/phy/renesas/phy-rcar-gen2.c: In function 'rcar_gen2_phy_probe':
./include/linux/build_bug.h:16:45: error: negative width in bit-field '<anonymous>'
 #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
                                             ^
./include/linux/compiler.h:351:28: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
 #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
                            ^
./include/linux/kernel.h:47:59: note: in expansion of macro '__must_be_array'
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
                                                           ^
drivers/phy/renesas/phy-rcar-gen2.c:398:30: note: in expansion of macro 'ARRAY_SIZE'
   if (error || channel_num > ARRAY_SIZE(data->select_value)) {
                              ^
scripts/Makefile.build:278: recipe for target 'drivers/phy/renesas/phy-rcar-gen2.o' failed

regards,
Biju



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning
  2019-05-16 12:16 ` Wolfram Sang
  2019-05-16 12:32   ` Biju Das
@ 2019-05-16 12:34   ` Geert Uytterhoeven
  2019-05-16 13:25     ` Wolfram Sang
  1 sibling, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2019-05-16 12:34 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Biju Das, Kishon Vijay Abraham I, Dan Carpenter, Wolfram Sang,
	Simon Horman, Yoshihiro Shimoda, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	Linux-Renesas

Hi Wolfram,

On Thu, May 16, 2019 at 2:17 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>
> >               error = of_property_read_u32(np, "reg", &channel_num);
> > -             if (error || channel_num > 2) {
> > +             if (error || channel_num > data->last_channel) {
>
> Just an idea, I haven't tested it: Couldn't we use
> ARRAY_SIZE(data->select_value) to avoid the extra member in the struct?

No we can't, as the sizes of the arrays pointed to by the various
SoC-specific structs differ, and at this point in the code, we just have a
pointer, not a fixed-size array. So you cannot use ARRAY_SIZE().

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning
  2019-05-15 13:43 [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning Biju Das
  2019-05-16 11:13 ` Simon Horman
  2019-05-16 12:16 ` Wolfram Sang
@ 2019-05-16 12:40 ` Geert Uytterhoeven
  2019-05-16 13:03   ` Biju Das
  2 siblings, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2019-05-16 12:40 UTC (permalink / raw)
  To: Biju Das
  Cc: Kishon Vijay Abraham I, Dan Carpenter, Wolfram Sang,
	Simon Horman, Yoshihiro Shimoda, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	Linux-Renesas

Hi Biju,

On Wed, May 15, 2019 at 3:50 PM Biju Das <biju.das@bp.renesas.com> wrote:
> Fix the below smatch warning by adding variable check rather than the
> hardcoded value.
> warn: array off by one? 'data->select_value[channel_num]'
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

While your patch is correct (to the best of my knowledge), I think the
code can be made more maintainable by using ARRAY_SIZE().

> --- a/drivers/phy/renesas/phy-rcar-gen2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen2.c
> @@ -71,6 +71,7 @@ struct rcar_gen2_phy_driver {
>  struct rcar_gen2_phy_data {
>         const struct phy_ops *gen2_phy_ops;
>         const u32 (*select_value)[PHYS_PER_CHANNEL];
> +       const u32 last_channel;

num_channels? (which is one more than last_channel)

>  };
>
>  static int rcar_gen2_phy_init(struct phy *p)
> @@ -271,11 +272,13 @@ static const u32 usb20_select_value[][PHYS_PER_CHANNEL] = {
>  static const struct rcar_gen2_phy_data rcar_gen2_usb_phy_data = {
>         .gen2_phy_ops = &rcar_gen2_phy_ops,
>         .select_value = pci_select_value,
> +       .last_channel = 2,

.num_channels = ARRAY_SIZE(pci_select_value)

>  };
>
>  static const struct rcar_gen2_phy_data rz_g1c_usb_phy_data = {
>         .gen2_phy_ops = &rz_g1c_phy_ops,
>         .select_value = usb20_select_value,
> +       .last_channel = 0,

.num_channels = ARRAY_SIZE(usb20_select_value)

>  };
>
>  static const struct of_device_id rcar_gen2_phy_match_table[] = {
> @@ -389,7 +392,7 @@ static int rcar_gen2_phy_probe(struct platform_device *pdev)
>                 channel->selected_phy = -1;
>
>                 error = of_property_read_u32(np, "reg", &channel_num);
> -               if (error || channel_num > 2) {
> +               if (error || channel_num > data->last_channel) {

>= data->num_channels

>                         dev_err(dev, "Invalid \"reg\" property\n");
>                         return error;
>                 }

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning
  2019-05-16 12:40 ` Geert Uytterhoeven
@ 2019-05-16 13:03   ` Biju Das
  0 siblings, 0 replies; 8+ messages in thread
From: Biju Das @ 2019-05-16 13:03 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Kishon Vijay Abraham I, Dan Carpenter, Wolfram Sang,
	Simon Horman, Yoshihiro Shimoda, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	Linux-Renesas

Hi Geert, 

Thanks for the feedback.

> Subject: Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one
> warning
> 
> Hi Biju,
> 
> On Wed, May 15, 2019 at 3:50 PM Biju Das <biju.das@bp.renesas.com>
> wrote:
> > Fix the below smatch warning by adding variable check rather than the
> > hardcoded value.
> > warn: array off by one? 'data->select_value[channel_num]'
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> 
> Thanks for your patch!
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> While your patch is correct (to the best of my knowledge), I think the code
> can be made more maintainable by using ARRAY_SIZE().

Ok. I will send V2.

> > --- a/drivers/phy/renesas/phy-rcar-gen2.c
> > +++ b/drivers/phy/renesas/phy-rcar-gen2.c
> > @@ -71,6 +71,7 @@ struct rcar_gen2_phy_driver {  struct
> > rcar_gen2_phy_data {
> >         const struct phy_ops *gen2_phy_ops;
> >         const u32 (*select_value)[PHYS_PER_CHANNEL];
> > +       const u32 last_channel;
> 
> num_channels? (which is one more than last_channel)
OK.

> >  };
> >
> >  static int rcar_gen2_phy_init(struct phy *p) @@ -271,11 +272,13 @@
> > static const u32 usb20_select_value[][PHYS_PER_CHANNEL] = {  static
> > const struct rcar_gen2_phy_data rcar_gen2_usb_phy_data = {
> >         .gen2_phy_ops = &rcar_gen2_phy_ops,
> >         .select_value = pci_select_value,
> > +       .last_channel = 2,
> 
> .num_channels = ARRAY_SIZE(pci_select_value)
OK.

> >  };
> >
> >  static const struct rcar_gen2_phy_data rz_g1c_usb_phy_data = {
> >         .gen2_phy_ops = &rz_g1c_phy_ops,
> >         .select_value = usb20_select_value,
> > +       .last_channel = 0,
> 
> .num_channels = ARRAY_SIZE(usb20_select_value)

OK.
> >  };
> >
> >  static const struct of_device_id rcar_gen2_phy_match_table[] = { @@
> > -389,7 +392,7 @@ static int rcar_gen2_phy_probe(struct platform_device
> *pdev)
> >                 channel->selected_phy = -1;
> >
> >                 error = of_property_read_u32(np, "reg", &channel_num);
> > -               if (error || channel_num > 2) {
> > +               if (error || channel_num > data->last_channel) {
> 
> >= data->num_channels
OK.

> >                         dev_err(dev, "Invalid \"reg\" property\n");
> >                         return error;
> >                 }

Regards,
Biju

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning
  2019-05-16 12:34   ` Geert Uytterhoeven
@ 2019-05-16 13:25     ` Wolfram Sang
  0 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2019-05-16 13:25 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Biju Das, Kishon Vijay Abraham I, Dan Carpenter, Wolfram Sang,
	Simon Horman, Yoshihiro Shimoda, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, Fabrizio Castro,
	Linux-Renesas

[-- Attachment #1: Type: text/plain, Size: 228 bytes --]


> SoC-specific structs differ, and at this point in the code, we just have a
> pointer, not a fixed-size array. So you cannot use ARRAY_SIZE().

Right, just a pointer. But well, you found a middle path for ARRAY_SIZE.
Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-15 13:43 [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning Biju Das
2019-05-16 11:13 ` Simon Horman
2019-05-16 12:16 ` Wolfram Sang
2019-05-16 12:32   ` Biju Das
2019-05-16 12:34   ` Geert Uytterhoeven
2019-05-16 13:25     ` Wolfram Sang
2019-05-16 12:40 ` Geert Uytterhoeven
2019-05-16 13:03   ` Biju Das

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox