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
