* [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 related [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, other threads:[~2019-05-16 13:25 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).