| Message ID | 20231019122850.1199821-8-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: provide Wake on LAN support | expand |
On Thu, Oct 19, 2023 at 02:28:48PM +0200, Oleksij Rempel wrote: > Enhance the ksz_switch_macaddr_get() function to handle errors that may > occur during the call to ksz_write8(). Specifically, this update checks > the return value of ksz_write8(), which may fail if regmap ranges > validation is not passed and returns the error code. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> > --- > drivers/net/dsa/microchip/ksz_common.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index 7b05de6fe987..79052a54880c 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -3612,7 +3612,7 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port, > struct ksz_switch_macaddr *switch_macaddr; > struct ksz_device *dev = ds->priv; > const u16 *regs = dev->info->regs; > - int i; > + int i, ret; > > /* Make sure concurrent MAC address changes are blocked */ > ASSERT_RTNL(); > @@ -3639,8 +3639,11 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port, > dev->switch_macaddr = switch_macaddr; > > /* Program the switch MAC address to hardware */ > - for (i = 0; i < ETH_ALEN; i++) > - ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]); > + for (i = 0; i < ETH_ALEN; i++) { > + ret = ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]); > + if (ret) > + return ret; > + } I understand that you intend the error to be fatal, but this leaks memory and a refcount.
On Thu, Oct 19, 2023 at 08:13:08PM +0300, Vladimir Oltean wrote: > On Thu, Oct 19, 2023 at 02:28:48PM +0200, Oleksij Rempel wrote: > > Enhance the ksz_switch_macaddr_get() function to handle errors that may > > occur during the call to ksz_write8(). Specifically, this update checks > > the return value of ksz_write8(), which may fail if regmap ranges > > validation is not passed and returns the error code. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> > > --- > > drivers/net/dsa/microchip/ksz_common.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > > index 7b05de6fe987..79052a54880c 100644 > > --- a/drivers/net/dsa/microchip/ksz_common.c > > +++ b/drivers/net/dsa/microchip/ksz_common.c > > @@ -3612,7 +3612,7 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port, > > struct ksz_switch_macaddr *switch_macaddr; > > struct ksz_device *dev = ds->priv; > > const u16 *regs = dev->info->regs; > > - int i; > > + int i, ret; > > > > /* Make sure concurrent MAC address changes are blocked */ > > ASSERT_RTNL(); > > @@ -3639,8 +3639,11 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port, > > dev->switch_macaddr = switch_macaddr; > > > > /* Program the switch MAC address to hardware */ > > - for (i = 0; i < ETH_ALEN; i++) > > - ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]); > > + for (i = 0; i < ETH_ALEN; i++) { > > + ret = ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]); > > + if (ret) > > + return ret; > > + } > > I understand that you intend the error to be fatal, but this leaks memory and a refcount. I didn't estimate correctly what would happen. If ksz_write8() fails as you say, we will leave behind the reference to dev->switch_macaddr, which points to valid memory. Which means that the second time around, the call to ksz_switch_macaddr_get() will succeed, because the driver thinks that the address has been programmed to hardware, and it is unaware of the previous failure. But it will not work. Am I correct? If so, it needs to tear down properly, because if ksz_switch_macaddr_get() fails once to write to hardware, it should fail always.
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 7b05de6fe987..79052a54880c 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -3612,7 +3612,7 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port, struct ksz_switch_macaddr *switch_macaddr; struct ksz_device *dev = ds->priv; const u16 *regs = dev->info->regs; - int i; + int i, ret; /* Make sure concurrent MAC address changes are blocked */ ASSERT_RTNL(); @@ -3639,8 +3639,11 @@ int ksz_switch_macaddr_get(struct dsa_switch *ds, int port, dev->switch_macaddr = switch_macaddr; /* Program the switch MAC address to hardware */ - for (i = 0; i < ETH_ALEN; i++) - ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]); + for (i = 0; i < ETH_ALEN; i++) { + ret = ksz_write8(dev, regs[REG_SW_MAC_ADDR] + i, addr[i]); + if (ret) + return ret; + } return 0; }