Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning
> 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! signature.asc Description: PGP signature
RE: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning
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
> 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
> > Signed-off-by: Biju Das
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven
>
> 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
Hi Biju,
On Wed, May 15, 2019 at 3:50 PM 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
> Signed-off-by: Biju Das
Thanks for your patch!
Reviewed-by: Geert Uytterhoeven
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 -- ge...@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
Hi Wolfram,
On Thu, May 16, 2019 at 2:17 PM Wolfram Sang 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 -- ge...@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
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
''
#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
> 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?
signature.asc
Description: PGP signature
Re: [PATCH] phy: renesas: phy-rcar-gen2: Fix the array off by one warning
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 > Signed-off-by: Biju Das Reviewed-by: Simon Horman
