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++) {
+		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 = {
-- 
2.39.5

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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