Messages in this thread | | | Subject | Re: [PATCH] net: dsa: sja1105: fix speed setting for 10 MBPS | From | Sergei Shtylyov <> | Date | Sat, 2 May 2020 15:21:20 +0300 |
| |
Hello!
On 01.05.2020 16:43, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com> > > The current logic for speed checking will never set the speed to 10 MBPS > because bmcr & BMCR_SPEED10 is always 0 since BMCR_SPEED10 is 0. Also > the erroneous setting where BMCR_SPEED1000 and BMCR_SPEED100 are both > set causes the speed to be 1000 MBS. Fix this by masking bps and checking > for just the expected settings of BMCR_SPEED1000, BMCR_SPEED100 and > BMCR_SPEED10 and defaulting to the unknown speed otherwise. > > Addresses-Coverity: ("Logically dead code") > Fixes: ffe10e679cec ("net: dsa: sja1105: Add support for the SGMII port") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/net/dsa/sja1105/sja1105_main.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c > index 472f4eb20c49..59a9038cdc4e 100644 > --- a/drivers/net/dsa/sja1105/sja1105_main.c > +++ b/drivers/net/dsa/sja1105/sja1105_main.c > @@ -1600,6 +1600,7 @@ static const char * const sja1105_reset_reasons[] = { > int sja1105_static_config_reload(struct sja1105_private *priv, > enum sja1105_reset_reason reason) > { > + const int mask = (BMCR_SPEED1000 | BMCR_SPEED100 | BMCR_SPEED10);
Why not declare it in the block it's used in? Also, () not needed here.
> struct ptp_system_timestamp ptp_sts_before; > struct ptp_system_timestamp ptp_sts_after; > struct sja1105_mac_config_entry *mac; > @@ -1684,14 +1685,16 @@ int sja1105_static_config_reload(struct sja1105_private *priv, > sja1105_sgmii_pcs_config(priv, an_enabled, false); > > if (!an_enabled) { > - int speed = SPEED_UNKNOWN; > + int speed;
Why not the following?
int mask = bmcr & (BMCR_SPEED1000 | BMCR_SPEED100 | BMCR_SPEED10);
> > - if (bmcr & BMCR_SPEED1000) > + if ((bmcr & mask) == BMCR_SPEED1000) > speed = SPEED_1000; > - else if (bmcr & BMCR_SPEED100) > + else if ((bmcr & mask) == BMCR_SPEED100) > speed = SPEED_100; > - else if (bmcr & BMCR_SPEED10) > + else if ((bmcr & mask) == BMCR_SPEED10) > speed = SPEED_10; > + else > + speed = SPEED_UNKNOWN; > > sja1105_sgmii_pcs_force_speed(priv, speed); > }
MBR, Sergei
| |