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@xxxxxxx>
Sent: Monday, October 28, 2024 3:19 PM
To: Ronnie Kunin - C21729 <Ronnie.Kunin@xxxxxxxxxxxxx>
Cc: Fabi.Benschuh@xxxxxx; Woojung Huh - C21699 <Woojung.Huh@xxxxxxxxxxxxx>; UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>; andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
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@xxxxxxxxxxxxx wrote:
> > From: Andrew Lunn <andrew@xxxxxxx>
> > 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
