| Message ID | 20231013121936.364678-2-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | rswitch: Add PM ops | expand |
+ Geert Uytterhoeven On Fri, Oct 13, 2023 at 09:19:35PM +0900, Yoshihiro Shimoda wrote: > The array index should not be negative, so modify the condition of > rswitch_for_each_enabled_port_continue_reverse() macro, and then > use unsigned int instead. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/net/ethernet/renesas/rswitch.c | 8 +++++--- > drivers/net/ethernet/renesas/rswitch.h | 2 +- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c > index 112e605f104a..7640144db79b 100644 > --- a/drivers/net/ethernet/renesas/rswitch.c > +++ b/drivers/net/ethernet/renesas/rswitch.c > @@ -1405,7 +1405,8 @@ static void rswitch_ether_port_deinit_one(struct rswitch_device *rdev) > > static int rswitch_ether_port_init_all(struct rswitch_private *priv) > { > - int i, err; > + unsigned int i; > + int err; > > rswitch_for_each_enabled_port(priv, i) { > err = rswitch_ether_port_init_one(priv->rdev[i]); > @@ -1786,7 +1787,8 @@ static void rswitch_device_free(struct rswitch_private *priv, int index) > > static int rswitch_init(struct rswitch_private *priv) > { > - int i, err; > + unsigned int i; > + int err; > > for (i = 0; i < RSWITCH_NUM_PORTS; i++) > rswitch_etha_init(priv, i); Hi Shimoda-san, Immediately below this hunk, the following code appears. if (err < 0) { for (i--; i >= 0; i--) rswitch_device_free(priv, i); goto err_device_alloc; } I suspect that the for loop should be updated in a similar way to that in rswitch_for_each_enabled_port_continue_reverse as, now that i is unsigned, i >= 0 will always be true. As flagged by Smatch and Coccinelle. Otherwise this patch-set looks good to me. > @@ -1959,7 +1961,7 @@ static int renesas_eth_sw_probe(struct platform_device *pdev) > > static void rswitch_deinit(struct rswitch_private *priv) > { > - int i; > + unsigned int i; > > rswitch_gwca_hw_deinit(priv); > rcar_gen4_ptp_unregister(priv->ptp_priv); > diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h > index 04f49a7a5843..27c9d3872c0e 100644 > --- a/drivers/net/ethernet/renesas/rswitch.h > +++ b/drivers/net/ethernet/renesas/rswitch.h > @@ -20,7 +20,7 @@ > else > > #define rswitch_for_each_enabled_port_continue_reverse(priv, i) \ > - for (i--; i >= 0; i--) \ > + for (; i-- > 0; ) \ > if (priv->rdev[i]->disabled) \ > continue; \ > else
Hi Simon-san, > From: Simon Horman, Sent: Tuesday, October 17, 2023 4:56 AM > > + Geert Uytterhoeven > > On Fri, Oct 13, 2023 at 09:19:35PM +0900, Yoshihiro Shimoda wrote: > > The array index should not be negative, so modify the condition of > > rswitch_for_each_enabled_port_continue_reverse() macro, and then > > use unsigned int instead. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/net/ethernet/renesas/rswitch.c | 8 +++++--- > > drivers/net/ethernet/renesas/rswitch.h | 2 +- > > 2 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c > > index 112e605f104a..7640144db79b 100644 > > --- a/drivers/net/ethernet/renesas/rswitch.c > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > @@ -1405,7 +1405,8 @@ static void rswitch_ether_port_deinit_one(struct rswitch_device *rdev) > > > > static int rswitch_ether_port_init_all(struct rswitch_private *priv) > > { > > - int i, err; > > + unsigned int i; > > + int err; > > > > rswitch_for_each_enabled_port(priv, i) { > > err = rswitch_ether_port_init_one(priv->rdev[i]); > > @@ -1786,7 +1787,8 @@ static void rswitch_device_free(struct rswitch_private *priv, int index) > > > > static int rswitch_init(struct rswitch_private *priv) > > { > > - int i, err; > > + unsigned int i; > > + int err; > > > > for (i = 0; i < RSWITCH_NUM_PORTS; i++) > > rswitch_etha_init(priv, i); > > Hi Shimoda-san, > > Immediately below this hunk, the following code appears. > > if (err < 0) { > for (i--; i >= 0; i--) > rswitch_device_free(priv, i); > goto err_device_alloc; > } > > I suspect that the for loop should be updated in a similar way to > that in rswitch_for_each_enabled_port_continue_reverse as, > now that i is unsigned, i >= 0 will always be true. > > As flagged by Smatch and Coccinelle. Thank you for your comment! I should have checked the patch-set by such tools... Anyway, I'll submit v3 patches. > Otherwise this patch-set looks good to me. Thank you for your review! Best regards, Yoshihiro Shimoda > > @@ -1959,7 +1961,7 @@ static int renesas_eth_sw_probe(struct platform_device *pdev) > > > > static void rswitch_deinit(struct rswitch_private *priv) > > { > > - int i; > > + unsigned int i; > > > > rswitch_gwca_hw_deinit(priv); > > rcar_gen4_ptp_unregister(priv->ptp_priv); > > diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h > > index 04f49a7a5843..27c9d3872c0e 100644 > > --- a/drivers/net/ethernet/renesas/rswitch.h > > +++ b/drivers/net/ethernet/renesas/rswitch.h > > @@ -20,7 +20,7 @@ > > else > > > > #define rswitch_for_each_enabled_port_continue_reverse(priv, i) \ > > - for (i--; i >= 0; i--) \ > > + for (; i-- > 0; ) \ > > if (priv->rdev[i]->disabled) \ > > continue; \ > > else > > -- > pw-bot: changes-requested
diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c index 112e605f104a..7640144db79b 100644 --- a/drivers/net/ethernet/renesas/rswitch.c +++ b/drivers/net/ethernet/renesas/rswitch.c @@ -1405,7 +1405,8 @@ static void rswitch_ether_port_deinit_one(struct rswitch_device *rdev) static int rswitch_ether_port_init_all(struct rswitch_private *priv) { - int i, err; + unsigned int i; + int err; rswitch_for_each_enabled_port(priv, i) { err = rswitch_ether_port_init_one(priv->rdev[i]); @@ -1786,7 +1787,8 @@ static void rswitch_device_free(struct rswitch_private *priv, int index) static int rswitch_init(struct rswitch_private *priv) { - int i, err; + unsigned int i; + int err; for (i = 0; i < RSWITCH_NUM_PORTS; i++) rswitch_etha_init(priv, i); @@ -1959,7 +1961,7 @@ static int renesas_eth_sw_probe(struct platform_device *pdev) static void rswitch_deinit(struct rswitch_private *priv) { - int i; + unsigned int i; rswitch_gwca_hw_deinit(priv); rcar_gen4_ptp_unregister(priv->ptp_priv); diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h index 04f49a7a5843..27c9d3872c0e 100644 --- a/drivers/net/ethernet/renesas/rswitch.h +++ b/drivers/net/ethernet/renesas/rswitch.h @@ -20,7 +20,7 @@ else #define rswitch_for_each_enabled_port_continue_reverse(priv, i) \ - for (i--; i >= 0; i--) \ + for (; i-- > 0; ) \ if (priv->rdev[i]->disabled) \ continue; \ else
The array index should not be negative, so modify the condition of rswitch_for_each_enabled_port_continue_reverse() macro, and then use unsigned int instead. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/net/ethernet/renesas/rswitch.c | 8 +++++--- drivers/net/ethernet/renesas/rswitch.h | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-)