[PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning

[PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning

[Biju Das]

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

Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning

[Simon Horman]

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>

Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning

[Wolfram Sang]

[-- 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 --]

RE: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning

[Biju Das]

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

Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning

[Geert Uytterhoeven]

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

Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning

[Geert Uytterhoeven]

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

RE: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning

[Biju Das]

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

Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning

[Wolfram Sang]

[-- 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 --]