Add LAN78XX OTP_ACCESS flag support

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(-)

On Sat, Oct 26, 2024 at 01:05:46AM +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


Please can you tell us more about OTP vs EEPROM? Is the OTP internal
while the EEPROM is external? What is contained in each? How does the
device decide which to use when it finds it has both?

I'm just wondering if we even need a private flag, if the hardware
will use one or the other exclusively?

	Andrew

> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, October 28, 2024 8:38 AM
> 
> On Sat, Oct 26, 2024 at 01:05:46AM +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
> 
> Please can you tell us more about OTP vs EEPROM? Is the OTP internal while the EEPROM is external?
> What is contained in each? How does the device decide which to use when it finds it has both?
> 
>         Andrew

This is pretty much the same implementation that is already in place for the Linux driver of the LAN743x PCIe device.

OTP (one time programmable) is a configuration memory internal to the device. EEPROM is external.
The internal OTP memory always exists but it may not be programmed. The EEPROM is optional, and if present can also be programmed or not.
A signature byte at offset 0 indicates whether the OTP or EEPROM device is programmed. 
If present, EEPROM has higher priority.
More info @ Chapter 10 here;
https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/LAN7800-Data-Sheet-DS00001992H.pdf

> I'm just wondering if we even need a private flag, if the hardware will use one or the other exclusively?
>
Yes, both can be present/used, so we need the flag so we can tell the destination of a write or source of a read.

Ronnie

On Mon, Oct 28, 2024 at 03:02:44PM +0000, Ronnie.Kunin@microchip.com wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Monday, October 28, 2024 8:38 AM
> > 
> > On Sat, Oct 26, 2024 at 01:05:46AM +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
> > 
> > Please can you tell us more about OTP vs EEPROM? Is the OTP internal while the EEPROM is external?
> > What is contained in each? How does the device decide which to use when it finds it has both?
> > 
> >         Andrew
> 

> This is pretty much the same implementation that is already in place
> for the Linux driver of the LAN743x PCIe device.

That is good, it gives some degree of consistency. But i wounder if we
should go further. I doubt these are the only two devices which
support both EEPROM and OTP. It would be nicer to extend ethtool:

       ethtool -e|--eeprom-dump devname [raw on|off] [offset N] [length N] [otp] [eeprom]

There does not appear to be a netlink implementation of this ethtool
call. If we add one, we can add an additional optional attribute,
indicating OTP or EEPROM. We have done similar in the past. It
probably means within the kernel you replace struct ethtool_eeprom
with struct kernel_ethtool_eeprom which has the additional
parameter. The ioctl interface then never sees the new parameter,
which keeps with the kAPI. get_eeprom() and set_eeprom() probably have
all they need. get_eeprom_len() is more complex since it currently
only takes netdev. I think get_eeprom_len() needs its functionality
extended to indicate if the driver actually looks at the additional
parameter. We want the kAPI calls for get and set to failed with
-EOPNOTSUPP when otp or eeprom is not supported, which will initially
be 99% of the drivers. And we don't want to have to make proper code
changes to every driver. So maybe an additional parameter

	int	(*get_eeprom_len)(struct net_device *,
	                          struct kernel_eeprom_len *eeprom_len);

struct kernel_eeprom_len {
	int otp;
	int eeprom;
}

Have the core zero this. After the call, if they are still zero, they
are not supported.

I know this is a lot more work than your current patch, but it is a
better solution, should be well documented, easy to find and should
work for everybody, against a device private parameter which is not
obvious and unlikely to be consistent across drivers.

	Andrew

Thanks Andrew.

Adding Rakesh who has been working on this patch internally at Microchip (though for an earlier kernel version) and after completing that was going to be upstreaming it for net-next (Fabian beat him to it). 

Ronnie

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch> 
Sent: Monday, October 28, 2024 3:19 PM
To: Ronnie Kunin - C21729 <Ronnie.Kunin@microchip.com>
Cc: Fabi.Benschuh@fau.de; Woojung Huh - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver <UNGLinuxDriver@microchip.com>; andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-usb@vger.kernel.org
Subject: Re: [PATCH] Add LAN78XX OTP_ACCESS flag support

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Mon, Oct 28, 2024 at 03:02:44PM +0000, Ronnie.Kunin@microchip.com wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Monday, October 28, 2024 8:38 AM
> >
> > On Sat, Oct 26, 2024 at 01:05:46AM +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
> >
> > Please can you tell us more about OTP vs EEPROM? Is the OTP internal while the EEPROM is external?
> > What is contained in each? How does the device decide which to use when it finds it has both?
> >
> >         Andrew
>

> This is pretty much the same implementation that is already in place 
> for the Linux driver of the LAN743x PCIe device.

That is good, it gives some degree of consistency. But i wounder if we should go further. I doubt these are the only two devices which support both EEPROM and OTP. It would be nicer to extend ethtool:

       ethtool -e|--eeprom-dump devname [raw on|off] [offset N] [length N] [otp] [eeprom]

There does not appear to be a netlink implementation of this ethtool call. If we add one, we can add an additional optional attribute, indicating OTP or EEPROM. We have done similar in the past. It probably means within the kernel you replace struct ethtool_eeprom with struct kernel_ethtool_eeprom which has the additional parameter. The ioctl interface then never sees the new parameter, which keeps with the kAPI. get_eeprom() and set_eeprom() probably have all they need. get_eeprom_len() is more complex since it currently only takes netdev. I think get_eeprom_len() needs its functionality extended to indicate if the driver actually looks at the additional parameter. We want the kAPI calls for get and set to failed with -EOPNOTSUPP when otp or eeprom is not supported, which will initially be 99% of the drivers. And we don't want to have to make proper code changes to every driver. So maybe an additional parameter

        int     (*get_eeprom_len)(struct net_device *,
                                  struct kernel_eeprom_len *eeprom_len);

struct kernel_eeprom_len {
        int otp;
        int eeprom;
}

Have the core zero this. After the call, if they are still zero, they are not supported.

I know this is a lot more work than your current patch, but it is a better solution, should be well documented, easy to find and should work for everybody, against a device private parameter which is not obvious and unlikely to be consistent across drivers.

        Andrew

On Mon, 28 Oct 2024 20:19:04 +0100 Andrew Lunn wrote:
> > This is pretty much the same implementation that is already in place
> > for the Linux driver of the LAN743x PCIe device.  
> 
> That is good, it gives some degree of consistency. But i wounder if we
> should go further. I doubt these are the only two devices which
> support both EEPROM and OTP. It would be nicer to extend ethtool:
> 
>        ethtool -e|--eeprom-dump devname [raw on|off] [offset N] [length N] [otp] [eeprom]

After a cursory look at the conversation I wonder if it wouldn't 
be easier to register devlink regions for eeprom and otp?

On Tue, Oct 29, 2024 at 10:43:13AM -0700, Jakub Kicinski wrote:
> On Mon, 28 Oct 2024 20:19:04 +0100 Andrew Lunn wrote:
> > > This is pretty much the same implementation that is already in place
> > > for the Linux driver of the LAN743x PCIe device.  
> > 
> > That is good, it gives some degree of consistency. But i wounder if we
> > should go further. I doubt these are the only two devices which
> > support both EEPROM and OTP. It would be nicer to extend ethtool:
> > 
> >        ethtool -e|--eeprom-dump devname [raw on|off] [offset N] [length N] [otp] [eeprom]
> 
> After a cursory look at the conversation I wonder if it wouldn't 
> be easier to register devlink regions for eeprom and otp?

Hi Jakub

devlink regions don't allow write. ethtool does.

	Andrew

On Tue, 29 Oct 2024 22:14:12 +0100 Andrew Lunn wrote:
> > > That is good, it gives some degree of consistency. But i wounder if we
> > > should go further. I doubt these are the only two devices which
> > > support both EEPROM and OTP. It would be nicer to extend ethtool:
> > > 
> > >        ethtool -e|--eeprom-dump devname [raw on|off] [offset N] [length N] [otp] [eeprom]  
> > 
> > After a cursory look at the conversation I wonder if it wouldn't 
> > be easier to register devlink regions for eeprom and otp?  
> 
> devlink regions don't allow write. ethtool does.

Sorry I missed the write part.
I see you already asked the "why" but I don't think the answer 
is entirely to the point. We need to know more - netdev focuses on
production use cases. Burning an OTP seems like a manufacturing action.

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,