Re: [PATCH] Add LAN78XX OTP_ACCESS flag support

[Rakesh Babu Saladi <Saladi.Rakeshbabu@microchip.com>]

Hi Fabian,

Please see the comments inline.

Thanks,
Rakesh.

On Sat, 2024-10-26 at 01:05 +0200, Fabian Benschuh wrote:
> With this flag we can now use ethtool to access the OTP:
> ethtool --set-priv-flags eth0 OTP_ACCESS on
> ethtool -e eth0  # this will read OTP if OTP_ACCESS is on, else
> EEPROM
> 
> When writing to OTP we need to set OTP_ACCESS on and write with the
> correct magic 0x7873 for OTP
> ---
>  drivers/net/usb/lan78xx.c | 55 ++++++++++++++++++++++++++++++++-----
> --
>  1 file changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 8adf77e3557e..2fc9b9b138b0 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -85,6 +85,7 @@
>  #define EEPROM_INDICATOR		(0xA5)
>  #define EEPROM_MAC_OFFSET		(0x01)
>  #define MAX_EEPROM_SIZE			512
> +#define MAX_OTP_SIZE			512
>  #define OTP_INDICATOR_1			(0xF3)
>  #define OTP_INDICATOR_2			(0xF7)
>  
> @@ -172,6 +173,7 @@
>  #define INT_EP_GPIO_2			(2)
>  #define INT_EP_GPIO_1			(1)
>  #define INT_EP_GPIO_0			(0)
> +#define LAN78XX_NET_FLAG_OTP		BIT(0)
Please move this macro to other macros defined with prefix "LAN78XX_"
and probably please rename it to "LAN78XX_ADAPTER_ETHTOOL_FLAG_OTP".

>  
>  static const char lan78xx_gstrings[][ETH_GSTRING_LEN] = {
>  	"RX FCS Errors",
> @@ -446,6 +448,7 @@ struct lan78xx_net {
>  	unsigned int		burst_cap;
>  
>  	unsigned long		flags;
> +	u32			priv_flags;
It would be good to include variables of single type to be grouped
together. There are variables of type u32, please move this variable to
that group.

>  
>  	wait_queue_head_t	*wait;
>  	unsigned char		suspend_count;
> @@ -1542,6 +1545,10 @@ static void lan78xx_status(struct lan78xx_net
> *dev, struct urb *urb)
>  
>  static int lan78xx_ethtool_get_eeprom_len(struct net_device *netdev)
>  {
> +	struct lan78xx_net *dev = netdev_priv(netdev);
> +
> +	if (dev->priv_flags & LAN78XX_NET_FLAG_OTP)
> +		return MAX_OTP_SIZE;
>  	return MAX_EEPROM_SIZE;
>  }
>  
> @@ -1555,9 +1562,10 @@ static int lan78xx_ethtool_get_eeprom(struct
> net_device *netdev,
>  	if (ret)
>  		return ret;
>  
> -	ee->magic = LAN78XX_EEPROM_MAGIC;
> -
> -	ret = lan78xx_read_raw_eeprom(dev, ee->offset, ee->len, data);
> +	if (dev->priv_flags & LAN78XX_NET_FLAG_OTP)
> +		ret = lan78xx_read_raw_otp(dev, ee->offset, ee->len,
> data);
> +	else
> +		ret = lan78xx_read_raw_eeprom(dev, ee->offset, ee->len, 
> data);
>  
>  	usb_autopm_put_interface(dev->intf);
>  
> @@ -1577,30 +1585,39 @@ static int lan78xx_ethtool_set_eeprom(struct
> net_device *netdev,
>  	/* Invalid EEPROM_INDICATOR at offset zero will result in a
> failure
>  	 * to load data from EEPROM
>  	 */
> -	if (ee->magic == LAN78XX_EEPROM_MAGIC)
> -		ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee-
> >len, data);
> -	else if ((ee->magic == LAN78XX_OTP_MAGIC) &&
> -		 (ee->offset == 0) &&
> -		 (ee->len == 512) &&
> -		 (data[0] == OTP_INDICATOR_1))
> -		ret = lan78xx_write_raw_otp(dev, ee->offset, ee->len,
> data);
> +	if (dev->priv_flags & LAN78XX_NET_FLAG_OTP) {
> +		/* Beware!  OTP is One Time Programming ONLY! */
> +		if (ee->magic == LAN78XX_OTP_MAGIC)
> +		    ret = lan78xx_write_raw_otp(dev, ee->offset, ee-
> >len, data);
> +	} else {
> +		if (ee->magic == LAN78XX_EEPROM_MAGIC)
> +		    ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee-
> >len, data);
In the function "lan78xx_write_raw_eeprom.otp", please add the
condition check if the offset + length are within the
EEPROM/OTP_MAX_SIZE limits and proceed further to write if the
condition is met.

> +	}
>  
>  	usb_autopm_put_interface(dev->intf);
>  
>  	return ret;
>  }
>  
> +static const char lan78xx_priv_flags_strings[][ETH_GSTRING_LEN] = {
> +	"OTP_ACCESS",
> +};
> +
>  static void lan78xx_get_strings(struct net_device *netdev, u32
> stringset,
>  				u8 *data)
>  {
>  	if (stringset == ETH_SS_STATS)
>  		memcpy(data, lan78xx_gstrings,
> sizeof(lan78xx_gstrings));
> +	else if (stringset == ETH_SS_PRIV_FLAGS)
> +		memcpy(data, lan78xx_priv_flags_strings,
> sizeof(lan78xx_priv_flags_strings));
>  }
>  
>  static int lan78xx_get_sset_count(struct net_device *netdev, int
> sset)
>  {
>  	if (sset == ETH_SS_STATS)
>  		return ARRAY_SIZE(lan78xx_gstrings);
> +	else if (sset == ETH_SS_PRIV_FLAGS)
> +		return ARRAY_SIZE(lan78xx_priv_flags_strings);
>  	else
>  		return -EOPNOTSUPP;
>  }
> @@ -1617,6 +1634,22 @@ static void lan78xx_get_stats(struct
> net_device *netdev,
>  	mutex_unlock(&dev->stats.access_lock);
>  }
>  
> +static u32 lan78xx_ethtool_get_priv_flags(struct net_device *netdev)
> +{
> +	struct lan78xx_net *dev = netdev_priv(netdev);
> +
> +	return dev->priv_flags;
> +}
> +
> +static int lan78xx_ethtool_set_priv_flags(struct net_device *netdev,
> u32 flags)
> +{
> +	struct lan78xx_net *dev = netdev_priv(netdev);
> +
> +	dev->priv_flags = flags;
Instead of assigning flags directly, it would be good to add checks and
assign something like this
if (flags & LAN78XX_ADAPTER_ETHTOOL_FLAG_OTP)
	dev->ethtool_flags |= LAN78XX_ADAPTER_ETHTOOL_FLAG_OTP;
else
	dev->ethtool_flags &= ~LAN78XX_ADAPTER_ETHTOOL_FLAG_OTP;

> +
> +	return 0;
> +}
> +
>  static void lan78xx_get_wol(struct net_device *netdev,
>  			    struct ethtool_wolinfo *wol)
>  {
> @@ -1905,6 +1938,8 @@ static const struct ethtool_ops
> lan78xx_ethtool_ops = {
>  	.get_eeprom	= lan78xx_ethtool_get_eeprom,
>  	.set_eeprom	= lan78xx_ethtool_set_eeprom,
>  	.get_ethtool_stats = lan78xx_get_stats,
> +	.get_priv_flags = lan78xx_ethtool_get_priv_flags,
> +	.set_priv_flags = lan78xx_ethtool_set_priv_flags,
>  	.get_sset_count = lan78xx_get_sset_count,
>  	.get_strings	= lan78xx_get_strings,
>  	.get_wol	= lan78xx_get_wol,