Re: [PATCH v1] smsc911x: add second read of EEPROM mac when possible corruption seen

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 28 Aug 2025 16:44:52 -0500 Colin Foster wrote:
> When the EEPROM MAC is read by way of ADDRH, it can return all 0s the
> first time. Subsequent reads succeed.
> 
> Re-read the ADDRH when this behaviour is observed, in an attempt to
> correctly apply the EEPROM MAC address.

Please name the device, and FW version if applicable, on which you
observe the issue.

> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/net/ethernet/smsc/smsc911x.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
> index a2e511912e6a9..63ed221edc00a 100644
> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c
> @@ -2162,8 +2162,20 @@ static const struct net_device_ops smsc911x_netdev_ops = {
>  static void smsc911x_read_mac_address(struct net_device *dev)
>  {
>  	struct smsc911x_data *pdata = netdev_priv(dev);
> -	u32 mac_high16 = smsc911x_mac_read(pdata, ADDRH);
> -	u32 mac_low32 = smsc911x_mac_read(pdata, ADDRL);
> +	u32 mac_high16, mac_low32;
> +
> +	mac_high16 = smsc911x_mac_read(pdata, ADDRH);
> +	mac_low32 = smsc911x_mac_read(pdata, ADDRL);
> +
> +	/*

nit: netdev multi-line comment style doesn't place /* on a separate
line:
	

> +	 * The first mac_read always returns 0. Re-read it to get the
> +	 * full MAC

Always? Strange, why did nobody notice until now?

> +	 */
> +	if (mac_high16 == 0) {
> +		SMSC_TRACE(pdata, probe, "Re-read MAC ADDRH\n");
> +		mac_high16 = smsc911x_mac_read(pdata, ADDRH);
> +	}

> 	u8 addr[ETH_ALEN];

Please don't add code in the middle of variable declarations
-- 
pw-bot: cr