Re: [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs

[Mateusz Polchlopek]

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@xxxxxxxxxxxxxx>
> ---
>   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 = {