| Message ID | 20190601103735.27506-2-olteanv@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Fix link speed handling for SJA1105 DSA driver | expand |
On Sat, Jun 01, 2019 at 01:37:34PM +0300, Vladimir Oltean wrote: > The code in sja1105_adjust_port_config relies on the fact that an > invalid link speed is detected by sja1105_get_speed_cfg and returned as > -EINVAL. However storing this into an enum that only has positive > members will cast it into an unsigned value, and it will miss the > negative check. > > So make the -EINVAL value part of the enum, so that it is stored as a > signed number and passes the negative check. > > Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch") > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Hi Vladimir It seems like just using a switch statement would be simpler, and more likely to be correct. And it would avoid adding SJA1105_SPEED_INVALID = -EINVAL which feels hackish. Andrew
On Sat, 1 Jun 2019 at 19:03, Andrew Lunn <andrew@lunn.ch> wrote: > > On Sat, Jun 01, 2019 at 01:37:34PM +0300, Vladimir Oltean wrote: > > The code in sja1105_adjust_port_config relies on the fact that an > > invalid link speed is detected by sja1105_get_speed_cfg and returned as > > -EINVAL. However storing this into an enum that only has positive > > members will cast it into an unsigned value, and it will miss the > > negative check. > > > > So make the -EINVAL value part of the enum, so that it is stored as a > > signed number and passes the negative check. > > > > Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch") > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > > Hi Vladimir > > It seems like just using a switch statement would be simpler, and more > likely to be correct. And it would avoid adding SJA1105_SPEED_INVALID > = -EINVAL which feels hackish. > > Andrew Hi Andrew, You mean I should completely remove the sja1105_get_speed_cfg function? I suppose I can do that, I'm only using it in one place. Thanks, -Vladimir
On Sat, Jun 01, 2019 at 11:30:16PM +0300, Vladimir Oltean wrote: > On Sat, 1 Jun 2019 at 19:03, Andrew Lunn <andrew@lunn.ch> wrote: > > > > On Sat, Jun 01, 2019 at 01:37:34PM +0300, Vladimir Oltean wrote: > > > The code in sja1105_adjust_port_config relies on the fact that an > > > invalid link speed is detected by sja1105_get_speed_cfg and returned as > > > -EINVAL. However storing this into an enum that only has positive > > > members will cast it into an unsigned value, and it will miss the > > > negative check. > > > > > > So make the -EINVAL value part of the enum, so that it is stored as a > > > signed number and passes the negative check. > > > > > > Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch") > > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > > > > Hi Vladimir > > > > It seems like just using a switch statement would be simpler, and more > > likely to be correct. And it would avoid adding SJA1105_SPEED_INVALID > > = -EINVAL which feels hackish. > > > > Andrew > > Hi Andrew, > > You mean I should completely remove the sja1105_get_speed_cfg function? > I suppose I can do that, I'm only using it in one place. I think it is often better to use simple, obviously correct code. This seems to be one example. Andrew
diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h index 1e6d0f33a663..15df37ec63bf 100644 --- a/drivers/net/dsa/sja1105/sja1105.h +++ b/drivers/net/dsa/sja1105/sja1105.h @@ -160,6 +160,7 @@ typedef enum { SJA1105_SPEED_100MBPS = 2, SJA1105_SPEED_1000MBPS = 1, SJA1105_SPEED_AUTO = 0, + SJA1105_SPEED_INVALID = -EINVAL, } sja1105_speed_t; int sja1105pqrs_setup_rgmii_delay(const void *ctx, int port); diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index b89d979ba213..12b1af52d84b 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -719,7 +719,7 @@ static sja1105_speed_t sja1105_get_speed_cfg(unsigned int speed_mbps) for (i = SJA1105_SPEED_AUTO; i <= SJA1105_SPEED_1000MBPS; i++) if (sja1105_speed[i] == speed_mbps) return i; - return -EINVAL; + return SJA1105_SPEED_INVALID; } /* Set link speed and enable/disable traffic I/O in the MAC configuration
The code in sja1105_adjust_port_config relies on the fact that an invalid link speed is detected by sja1105_get_speed_cfg and returned as -EINVAL. However storing this into an enum that only has positive members will cast it into an unsigned value, and it will miss the negative check. So make the -EINVAL value part of the enum, so that it is stored as a signed number and passes the negative check. Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch") Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- drivers/net/dsa/sja1105/sja1105.h | 1 + drivers/net/dsa/sja1105/sja1105_main.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)