* [PATCH v2 net 0/1] Fix link speed handling for SJA1105 DSA driver @ 2019-06-02 23:31 Vladimir Oltean 2019-06-02 23:31 ` [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below Vladimir Oltean 0 siblings, 1 reply; 6+ messages in thread From: Vladimir Oltean @ 2019-06-02 23:31 UTC (permalink / raw) To: f.fainelli, vivien.didelot, andrew, davem; +Cc: netdev, Vladimir Oltean This patchset avoids two bugs in the logic handling of the enum sja1105_speed_t which caused link speeds of 10 and 100 Mbps to not be interpreted correctly and thus not be applied to the switch MACs. v1 patchset can be found at: https://www.spinics.net/lists/netdev/msg574477.html Changes from v1: Applied Andrew Lunn's suggestion of removing the sja1105_get_speed_cfg function altogether instead of trying to fix it. Vladimir Oltean (1): net: dsa: sja1105: Fix link speed not working at 100 Mbps and below drivers/net/dsa/sja1105/sja1105_main.c | 32 +++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below 2019-06-02 23:31 [PATCH v2 net 0/1] Fix link speed handling for SJA1105 DSA driver Vladimir Oltean @ 2019-06-02 23:31 ` Vladimir Oltean 2019-06-03 0:50 ` Andrew Lunn 2019-06-04 18:52 ` David Miller 0 siblings, 2 replies; 6+ messages in thread From: Vladimir Oltean @ 2019-06-02 23:31 UTC (permalink / raw) To: f.fainelli, vivien.didelot, andrew, davem; +Cc: netdev, Vladimir Oltean The hardware values for link speed are held in the sja1105_speed_t enum. However they do not increase in the order that sja1105_get_speed_cfg was iterating over them (basically from SJA1105_SPEED_AUTO - 0 - to SJA1105_SPEED_1000MBPS - 1 - skipping the other two). Another bug is that 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 take the simplest approach and remove the sja1105_get_speed_cfg function and replace it with a simple switch-case statement. Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch") Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Suggested-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/dsa/sja1105/sja1105_main.c | 32 +++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index 5412c3551bcc..25bb64ce0432 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -710,16 +710,6 @@ static int sja1105_speed[] = { [SJA1105_SPEED_1000MBPS] = 1000, }; -static sja1105_speed_t sja1105_get_speed_cfg(unsigned int speed_mbps) -{ - int i; - - for (i = SJA1105_SPEED_AUTO; i <= SJA1105_SPEED_1000MBPS; i++) - if (sja1105_speed[i] == speed_mbps) - return i; - return -EINVAL; -} - /* Set link speed and enable/disable traffic I/O in the MAC configuration * for a specific port. * @@ -742,8 +732,21 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries; mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries; - speed = sja1105_get_speed_cfg(speed_mbps); - if (speed_mbps && speed < 0) { + switch (speed_mbps) { + case 0: + /* No speed update requested */ + speed = SJA1105_SPEED_AUTO; + break; + case 10: + speed = SJA1105_SPEED_10MBPS; + break; + case 100: + speed = SJA1105_SPEED_100MBPS; + break; + case 1000: + speed = SJA1105_SPEED_1000MBPS; + break; + default: dev_err(dev, "Invalid speed %iMbps\n", speed_mbps); return -EINVAL; } @@ -753,10 +756,7 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, * and we no longer need to store it in the static config (already told * hardware we want auto during upload phase). */ - if (speed_mbps) - mac[port].speed = speed; - else - mac[port].speed = SJA1105_SPEED_AUTO; + mac[port].speed = speed; /* On P/Q/R/S, one can read from the device via the MAC reconfiguration * tables. On E/T, MAC reconfig tables are not readable, only writable. -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below 2019-06-02 23:31 ` [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below Vladimir Oltean @ 2019-06-03 0:50 ` Andrew Lunn 2019-06-03 13:04 ` Vladimir Oltean 2019-06-04 18:52 ` David Miller 1 sibling, 1 reply; 6+ messages in thread From: Andrew Lunn @ 2019-06-03 0:50 UTC (permalink / raw) To: Vladimir Oltean; +Cc: f.fainelli, vivien.didelot, davem, netdev On Mon, Jun 03, 2019 at 02:31:37AM +0300, Vladimir Oltean wrote: > The hardware values for link speed are held in the sja1105_speed_t enum. > However they do not increase in the order that sja1105_get_speed_cfg was > iterating over them (basically from SJA1105_SPEED_AUTO - 0 - to > SJA1105_SPEED_1000MBPS - 1 - skipping the other two). > > Another bug is that 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 take the simplest approach and remove the sja1105_get_speed_cfg > function and replace it with a simple switch-case statement. > > Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch") > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > Suggested-by: Andrew Lunn <andrew@lunn.ch> > --- > drivers/net/dsa/sja1105/sja1105_main.c | 32 +++++++++++++------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c > index 5412c3551bcc..25bb64ce0432 100644 > --- a/drivers/net/dsa/sja1105/sja1105_main.c > +++ b/drivers/net/dsa/sja1105/sja1105_main.c > @@ -710,16 +710,6 @@ static int sja1105_speed[] = { > [SJA1105_SPEED_1000MBPS] = 1000, > }; > > -static sja1105_speed_t sja1105_get_speed_cfg(unsigned int speed_mbps) > -{ > - int i; > - > - for (i = SJA1105_SPEED_AUTO; i <= SJA1105_SPEED_1000MBPS; i++) > - if (sja1105_speed[i] == speed_mbps) > - return i; > - return -EINVAL; > -} > - > /* Set link speed and enable/disable traffic I/O in the MAC configuration > * for a specific port. > * > @@ -742,8 +732,21 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, > mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries; > mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries; > > - speed = sja1105_get_speed_cfg(speed_mbps); > - if (speed_mbps && speed < 0) { > + switch (speed_mbps) { > + case 0: > + /* No speed update requested */ > + speed = SJA1105_SPEED_AUTO; > + break; > + case 10: > + speed = SJA1105_SPEED_10MBPS; > + break; > + case 100: > + speed = SJA1105_SPEED_100MBPS; > + break; > + case 1000: > + speed = SJA1105_SPEED_1000MBPS; > + break; > + default: > dev_err(dev, "Invalid speed %iMbps\n", speed_mbps); > return -EINVAL; > } Thanks for the re-write. This looks more obviously correct. One minor nit-pick. We have SPEED_10, SPEED_100, SPEED_1000, etc. It would be good to use them. With that change Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below 2019-06-03 0:50 ` Andrew Lunn @ 2019-06-03 13:04 ` Vladimir Oltean 2019-06-03 13:06 ` Andrew Lunn 0 siblings, 1 reply; 6+ messages in thread From: Vladimir Oltean @ 2019-06-03 13:04 UTC (permalink / raw) To: Andrew Lunn; +Cc: Florian Fainelli, Vivien Didelot, David S. Miller, netdev On Mon, 3 Jun 2019 at 03:50, Andrew Lunn <andrew@lunn.ch> wrote: > > On Mon, Jun 03, 2019 at 02:31:37AM +0300, Vladimir Oltean wrote: > > The hardware values for link speed are held in the sja1105_speed_t enum. > > However they do not increase in the order that sja1105_get_speed_cfg was > > iterating over them (basically from SJA1105_SPEED_AUTO - 0 - to > > SJA1105_SPEED_1000MBPS - 1 - skipping the other two). > > > > Another bug is that 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 take the simplest approach and remove the sja1105_get_speed_cfg > > function and replace it with a simple switch-case statement. > > > > Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch") > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > > Suggested-by: Andrew Lunn <andrew@lunn.ch> > > --- > > drivers/net/dsa/sja1105/sja1105_main.c | 32 +++++++++++++------------- > > 1 file changed, 16 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c > > index 5412c3551bcc..25bb64ce0432 100644 > > --- a/drivers/net/dsa/sja1105/sja1105_main.c > > +++ b/drivers/net/dsa/sja1105/sja1105_main.c > > @@ -710,16 +710,6 @@ static int sja1105_speed[] = { > > [SJA1105_SPEED_1000MBPS] = 1000, > > }; > > > > -static sja1105_speed_t sja1105_get_speed_cfg(unsigned int speed_mbps) > > -{ > > - int i; > > - > > - for (i = SJA1105_SPEED_AUTO; i <= SJA1105_SPEED_1000MBPS; i++) > > - if (sja1105_speed[i] == speed_mbps) > > - return i; > > - return -EINVAL; > > -} > > - > > /* Set link speed and enable/disable traffic I/O in the MAC configuration > > * for a specific port. > > * > > @@ -742,8 +732,21 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port, > > mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries; > > mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries; > > > > - speed = sja1105_get_speed_cfg(speed_mbps); > > - if (speed_mbps && speed < 0) { > > + switch (speed_mbps) { > > + case 0: > > + /* No speed update requested */ > > + speed = SJA1105_SPEED_AUTO; > > + break; > > + case 10: > > + speed = SJA1105_SPEED_10MBPS; > > + break; > > + case 100: > > + speed = SJA1105_SPEED_100MBPS; > > + break; > > + case 1000: > > + speed = SJA1105_SPEED_1000MBPS; > > + break; > > + default: > > dev_err(dev, "Invalid speed %iMbps\n", speed_mbps); > > return -EINVAL; > > } > > Thanks for the re-write. This looks more obviously correct. One minor > nit-pick. We have SPEED_10, SPEED_100, SPEED_1000, etc. It would be > good to use them. > > With that change > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Andrew Hi Andrew, If I made the change you're suggesting, I would need to replace 0 with SPEED_UNKNOWN and thus I would conflict with this net-next change: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=af7cd0366ee994e8b35985d407261dc0ed9dfb4d I think it's simpler to wait until Dave merges net into net-next and then submit this 1000 -> SPEED_1000 change as a net-next one, and let the fix itself go into net. Sounds ok? Regards, -Vladimir ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below 2019-06-03 13:04 ` Vladimir Oltean @ 2019-06-03 13:06 ` Andrew Lunn 0 siblings, 0 replies; 6+ messages in thread From: Andrew Lunn @ 2019-06-03 13:06 UTC (permalink / raw) To: Vladimir Oltean; +Cc: Florian Fainelli, Vivien Didelot, David S. Miller, netdev > Hi Andrew, > > If I made the change you're suggesting, I would need to replace 0 with > SPEED_UNKNOWN and thus I would conflict with this net-next change: > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=af7cd0366ee994e8b35985d407261dc0ed9dfb4d > > I think it's simpler to wait until Dave merges net into net-next and > then submit this 1000 -> SPEED_1000 change as a net-next one, and let > the fix itself go into net. > Sounds ok? Fine. Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below 2019-06-02 23:31 ` [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below Vladimir Oltean 2019-06-03 0:50 ` Andrew Lunn @ 2019-06-04 18:52 ` David Miller 1 sibling, 0 replies; 6+ messages in thread From: David Miller @ 2019-06-04 18:52 UTC (permalink / raw) To: olteanv; +Cc: f.fainelli, vivien.didelot, andrew, netdev From: Vladimir Oltean <olteanv@gmail.com> Date: Mon, 3 Jun 2019 02:31:37 +0300 > The hardware values for link speed are held in the sja1105_speed_t enum. > However they do not increase in the order that sja1105_get_speed_cfg was > iterating over them (basically from SJA1105_SPEED_AUTO - 0 - to > SJA1105_SPEED_1000MBPS - 1 - skipping the other two). > > Another bug is that 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 take the simplest approach and remove the sja1105_get_speed_cfg > function and replace it with a simple switch-case statement. > > Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch") > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > Suggested-by: Andrew Lunn <andrew@lunn.ch> Applied. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-04 18:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-06-02 23:31 [PATCH v2 net 0/1] Fix link speed handling for SJA1105 DSA driver Vladimir Oltean 2019-06-02 23:31 ` [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below Vladimir Oltean 2019-06-03 0:50 ` Andrew Lunn 2019-06-03 13:04 ` Vladimir Oltean 2019-06-03 13:06 ` Andrew Lunn 2019-06-04 18:52 ` David Miller
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).