| Message ID | 20241216120941.1690908-2-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 30c63abaee9024ed7524325b3eeb7f2d26727c31 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | lan78xx: Preparations for PHYlink | expand |
On 12/16/2024 1:09 PM, Oleksij Rempel wrote: > Update `lan78xx_get_regs` to handle errors during register and PHY > reads. Log warnings for failed reads and exit the function early if an > error occurs. Drop all previously logged registers to signal > inconsistent readings to the user space. This ensures that invalid data > is not returned to users. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/usb/lan78xx.c | 36 ++++++++++++++++++++++++++++++------ > 1 file changed, 30 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c > index 4661d131b190..270345fcad65 100644 > --- a/drivers/net/usb/lan78xx.c > +++ b/drivers/net/usb/lan78xx.c > @@ -2108,20 +2108,44 @@ static void > lan78xx_get_regs(struct net_device *netdev, struct ethtool_regs *regs, > void *buf) > { > - u32 *data = buf; > - int i, j; > struct lan78xx_net *dev = netdev_priv(netdev); > + unsigned int data_count = 0; > + u32 *data = buf; > + int i, j, ret; > > /* Read Device/MAC registers */ > - for (i = 0; i < ARRAY_SIZE(lan78xx_regs); i++) > - lan78xx_read_reg(dev, lan78xx_regs[i], &data[i]); > + for (i = 0; i < ARRAY_SIZE(lan78xx_regs); i++) { > + ret = lan78xx_read_reg(dev, lan78xx_regs[i], &data[i]); > + if (ret < 0) { > + netdev_warn(dev->net, > + "failed to read register 0x%08x\n", > + lan78xx_regs[i]); > + goto clean_data; > + } > + > + data_count++; > + } > > if (!netdev->phydev) > return; > > /* Read PHY registers */ > - for (j = 0; j < 32; i++, j++) > - data[i] = phy_read(netdev->phydev, j); > + for (j = 0; j < 32; i++, j++) { Maybe during this refactor is it worth to add some #define with number of registers to be read? > + ret = phy_read(netdev->phydev, j); > + if (ret < 0) { > + netdev_warn(dev->net, > + "failed to read PHY register 0x%02x\n", j); > + goto clean_data; > + } > + > + data[i] = ret; > + data_count++; > + } > + > + return; > + > +clean_data: > + memset(data, 0, data_count * sizeof(u32)); > } > > static const struct ethtool_ops lan78xx_ethtool_ops = {
On Mon, Dec 16, 2024 at 02:07:37PM +0100, Mateusz Polchlopek wrote: > > > On 12/16/2024 1:09 PM, Oleksij Rempel wrote: > > if (!netdev->phydev) > > return; > > /* Read PHY registers */ > > - for (j = 0; j < 32; i++, j++) > > - data[i] = phy_read(netdev->phydev, j); > > + for (j = 0; j < 32; i++, j++) { > > Maybe during this refactor is it worth to add some #define with > number of registers to be read? I prefer to remove this part. Please see patch 5
On 12/16/2024 2:19 PM, Oleksij Rempel wrote: > On Mon, Dec 16, 2024 at 02:07:37PM +0100, Mateusz Polchlopek wrote: >> >> >> On 12/16/2024 1:09 PM, Oleksij Rempel wrote: >>> if (!netdev->phydev) >>> return; >>> /* Read PHY registers */ >>> - for (j = 0; j < 32; i++, j++) >>> - data[i] = phy_read(netdev->phydev, j); >>> + for (j = 0; j < 32; i++, j++) { >> >> Maybe during this refactor is it worth to add some #define with >> number of registers to be read? > > I prefer to remove this part. Please see patch 5 > Ach, now I see that. So please forget about my comment
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 4661d131b190..270345fcad65 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -2108,20 +2108,44 @@ static void lan78xx_get_regs(struct net_device *netdev, struct ethtool_regs *regs, void *buf) { - u32 *data = buf; - int i, j; struct lan78xx_net *dev = netdev_priv(netdev); + unsigned int data_count = 0; + u32 *data = buf; + int i, j, ret; /* Read Device/MAC registers */ - for (i = 0; i < ARRAY_SIZE(lan78xx_regs); i++) - lan78xx_read_reg(dev, lan78xx_regs[i], &data[i]); + for (i = 0; i < ARRAY_SIZE(lan78xx_regs); i++) { + ret = lan78xx_read_reg(dev, lan78xx_regs[i], &data[i]); + if (ret < 0) { + netdev_warn(dev->net, + "failed to read register 0x%08x\n", + lan78xx_regs[i]); + goto clean_data; + } + + data_count++; + } if (!netdev->phydev) return; /* Read PHY registers */ - for (j = 0; j < 32; i++, j++) - data[i] = phy_read(netdev->phydev, j); + for (j = 0; j < 32; i++, j++) { + ret = phy_read(netdev->phydev, j); + if (ret < 0) { + netdev_warn(dev->net, + "failed to read PHY register 0x%02x\n", j); + goto clean_data; + } + + data[i] = ret; + data_count++; + } + + return; + +clean_data: + memset(data, 0, data_count * sizeof(u32)); } static const struct ethtool_ops lan78xx_ethtool_ops = {
Update `lan78xx_get_regs` to handle errors during register and PHY reads. Log warnings for failed reads and exit the function early if an error occurs. Drop all previously logged registers to signal inconsistent readings to the user space. This ensures that invalid data is not returned to users. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/usb/lan78xx.c | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-)