diff mbox series

[RFC,net-next,1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM

Message ID 20240122084530.32451-2-danieller@nvidia.com
State New
Headers
Series Add ability to flash modules' firmware |

Commit Message

Danielle Ratson Jan. 22, 2024, 8:45 a.m. UTC 
  From: Ido Schimmel <idosch@nvidia.com>

Ethtool can already retrieve information from a transceiver module
EEPROM by invoking the ethtool_ops::get_module_eeprom_by_page operation.
Add a corresponding operation that allows ethtool to write to a
transceiver module EEPROM.

The purpose of this operation is not to enable arbitrary read / write
access, but to allow the kernel to write to specific addresses as part
of transceiver module firmware flashing. In the future, more
functionality can be implemented on top of these read / write
operations.

Adjust the comments of the 'ethtool_module_eeprom' structure as it is
no longer used only for read access.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Petr Machata <petrm@nvidia.com>
---
 include/linux/ethtool.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Russell King (Oracle) Jan. 23, 2024, 5:03 p.m. UTC | #1 
On Mon, Jan 22, 2024 at 10:45:22AM +0200, Danielle Ratson wrote:
>  /**
> - * struct ethtool_module_eeprom - EEPROM dump from specified page
> - * @offset: Offset within the specified EEPROM page to begin read, in bytes.
> - * @length: Number of bytes to read.
> - * @page: Page number to read from.
> - * @bank: Page bank number to read from, if applicable by EEPROM spec.
> + * struct ethtool_module_eeprom - plug-in module EEPROM read / write parameters
> + * @offset: Offset within the specified page, in bytes.
> + * @length: Number of bytes to read / write.
> + * @page: Page number.
> + * @bank: Bank number, if supported by EEPROM spec.

I suppose I should have reviewed the addition of this (I can't recall
whether I got the original or not.)

If one looks at SFF-8472, then the first 128 bytes of the EEPROM at
0x50 (0xA0 on the wire) are not paged. Whereas bytes 128..255 are the
paged bytes. Therefore, "offset within the specified page" can sensibly
be interpreted as referring to the EEPROM at 0x50, at an offset of
128 + offset.

Meanwhile, the actual implementation doesn't do that - the offset is
the offset from the beginning of the EEPROM, and offsets >= 128 access
the paged area.

What this means is that the parameter description here is basically
wrong, both before and after your change.

This really ought to be fixed so that we describe things correctly
rather than misleading people who read documentation. Otherwise, it's
a recipe for broken implementations... and it's also completely
pointless documenting it if the documentation is wrong.
Danielle Ratson Jan. 24, 2024, 1:10 p.m. UTC | #2 
> >  /**
> > - * struct ethtool_module_eeprom - EEPROM dump from specified page
> > - * @offset: Offset within the specified EEPROM page to begin read, in
> bytes.
> > - * @length: Number of bytes to read.
> > - * @page: Page number to read from.
> > - * @bank: Page bank number to read from, if applicable by EEPROM spec.
> > + * struct ethtool_module_eeprom - plug-in module EEPROM read / write
> > + parameters
> > + * @offset: Offset within the specified page, in bytes.
> > + * @length: Number of bytes to read / write.
> > + * @page: Page number.
> > + * @bank: Bank number, if supported by EEPROM spec.
> 
> I suppose I should have reviewed the addition of this (I can't recall whether I
> got the original or not.)
> 
> If one looks at SFF-8472, then the first 128 bytes of the EEPROM at
> 0x50 (0xA0 on the wire) are not paged. Whereas bytes 128..255 are the paged
> bytes. Therefore, "offset within the specified page" can sensibly be
> interpreted as referring to the EEPROM at 0x50, at an offset of
> 128 + offset.
> 
> Meanwhile, the actual implementation doesn't do that - the offset is the
> offset from the beginning of the EEPROM, and offsets >= 128 access the
> paged area.
> 
> What this means is that the parameter description here is basically wrong,
> both before and after your change.
> 
> This really ought to be fixed so that we describe things correctly rather than
> misleading people who read documentation. Otherwise, it's a recipe for
> broken implementations... and it's also completely pointless documenting it if
> the documentation is wrong.
> 

Will rephrase.

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Andrew Lunn Jan. 25, 2024, 8:26 p.m. UTC | #3 
On Mon, Jan 22, 2024 at 10:45:22AM +0200, Danielle Ratson wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> Ethtool can already retrieve information from a transceiver module
> EEPROM by invoking the ethtool_ops::get_module_eeprom_by_page operation.
> Add a corresponding operation that allows ethtool to write to a
> transceiver module EEPROM.
> 
> The purpose of this operation is not to enable arbitrary read / write
> access, but to allow the kernel to write to specific addresses as part
> of transceiver module firmware flashing. In the future, more
> functionality can be implemented on top of these read / write
> operations.

My memory is dim, but i thought we decided that since the algorithm to
program these modules is defined in the standard, all we need to do is
pass the firmware blob, and have an in kernel implementation of the
algorithm. There is no need to have an arbitrary write blob to module,
which might, or might not be abused in the future.

Also, is the module functional while its firmware is being upgraded?
Do we need to enforce the link is down?

   Andrew
Andrew Lunn Jan. 25, 2024, 8:45 p.m. UTC | #4 
On Thu, Jan 25, 2024 at 09:26:16PM +0100, Andrew Lunn wrote:
> On Mon, Jan 22, 2024 at 10:45:22AM +0200, Danielle Ratson wrote:
> > From: Ido Schimmel <idosch@nvidia.com>
> > 
> > Ethtool can already retrieve information from a transceiver module
> > EEPROM by invoking the ethtool_ops::get_module_eeprom_by_page operation.
> > Add a corresponding operation that allows ethtool to write to a
> > transceiver module EEPROM.
> > 
> > The purpose of this operation is not to enable arbitrary read / write
> > access, but to allow the kernel to write to specific addresses as part
> > of transceiver module firmware flashing. In the future, more
> > functionality can be implemented on top of these read / write
> > operations.
> 
> My memory is dim, but i thought we decided that since the algorithm to
> program these modules is defined in the standard, all we need to do is
> pass the firmware blob, and have an in kernel implementation of the
> algorithm. There is no need to have an arbitrary write blob to module,
> which might, or might not be abused in the future.

O.K, back after reading more of the patches.

If i'm understanding the code correctly, this is never exposed to
userspace? Its purely an in kernel API? It would be good to make that
clear in the commit message, and document that in the ethtool ops
structure.

Thanks
      Andrew
Danielle Ratson Jan. 30, 2024, 7:43 a.m. UTC | #5 
> > On Mon, Jan 22, 2024 at 10:45:22AM +0200, Danielle Ratson wrote:
> > > From: Ido Schimmel <idosch@nvidia.com>
> > >
> > > Ethtool can already retrieve information from a transceiver module
> > > EEPROM by invoking the ethtool_ops::get_module_eeprom_by_page
> operation.
> > > Add a corresponding operation that allows ethtool to write to a
> > > transceiver module EEPROM.
> > >
> > > The purpose of this operation is not to enable arbitrary read /
> > > write access, but to allow the kernel to write to specific addresses
> > > as part of transceiver module firmware flashing. In the future, more
> > > functionality can be implemented on top of these read / write
> > > operations.
> >
> > My memory is dim, but i thought we decided that since the algorithm to
> > program these modules is defined in the standard, all we need to do is
> > pass the firmware blob, and have an in kernel implementation of the
> > algorithm. There is no need to have an arbitrary write blob to module,
> > which might, or might not be abused in the future.
> 
> O.K, back after reading more of the patches.
> 
> If i'm understanding the code correctly, this is never exposed to userspace? Its
> purely an in kernel API? It would be good to make that clear in the commit
> message, and document that in the ethtool ops structure.
> 
> Thanks
>       Andrew

Hi Andrew,

Yes, that is correct.
Will add a clarification.

Thanks.
Danielle Ratson Jan. 31, 2024, 11:51 a.m. UTC | #6 
> > From: Ido Schimmel <idosch@nvidia.com>
> >
> > Ethtool can already retrieve information from a transceiver module
> > EEPROM by invoking the ethtool_ops::get_module_eeprom_by_page
> operation.
> > Add a corresponding operation that allows ethtool to write to a
> > transceiver module EEPROM.
> >
> > The purpose of this operation is not to enable arbitrary read / write
> > access, but to allow the kernel to write to specific addresses as part
> > of transceiver module firmware flashing. In the future, more
> > functionality can be implemented on top of these read / write
> > operations.
> 
> My memory is dim, but i thought we decided that since the algorithm to
> program these modules is defined in the standard, all we need to do is pass
> the firmware blob, and have an in kernel implementation of the algorithm.
> There is no need to have an arbitrary write blob to module, which might, or
> might not be abused in the future.
> 
> Also, is the module functional while its firmware is being upgraded?
> Do we need to enforce the link is down?
> 
>    Andrew

This is part of the reasons why we kept a flag for module_fw_flash_in_progress. 
I think it should be down since the module is doing some sort of reset during the flashing process (after the Run Firmware Image).
So in order to avoid packet loss, this should be considered.

Ill consider the relevant scenarios for vetoing in the actual version.

Thanks.
diff mbox series

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 325e0778e937..bb253d90352b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -474,17 +474,14 @@  struct ethtool_rmon_stats {
 #define ETH_MODULE_MAX_I2C_ADDRESS	0x7f
 
 /**
- * struct ethtool_module_eeprom - EEPROM dump from specified page
- * @offset: Offset within the specified EEPROM page to begin read, in bytes.
- * @length: Number of bytes to read.
- * @page: Page number to read from.
- * @bank: Page bank number to read from, if applicable by EEPROM spec.
+ * struct ethtool_module_eeprom - plug-in module EEPROM read / write parameters
+ * @offset: Offset within the specified page, in bytes.
+ * @length: Number of bytes to read / write.
+ * @page: Page number.
+ * @bank: Bank number, if supported by EEPROM spec.
  * @i2c_address: I2C address of a page. Value less than 0x7f expected. Most
  *	EEPROMs use 0x50 or 0x51.
  * @data: Pointer to buffer with EEPROM data of @length size.
- *
- * This can be used to manage pages during EEPROM dump in ethtool and pass
- * required information to the driver.
  */
 struct ethtool_module_eeprom {
 	u32	offset;
@@ -789,6 +786,8 @@  struct ethtool_rxfh_param {
  * @get_module_eeprom_by_page: Get a region of plug-in module EEPROM data from
  *	specified page. Returns a negative error code or the amount of bytes
  *	read.
+ * @set_module_eeprom_by_page: Write to a region of plug-in module EEPROM.
+ *	Returns a negative error code or zero.
  * @get_eth_phy_stats: Query some of the IEEE 802.3 PHY statistics.
  * @get_eth_mac_stats: Query some of the IEEE 802.3 MAC statistics.
  * @get_eth_ctrl_stats: Query some of the IEEE 802.3 MAC Ctrl statistics.
@@ -921,6 +920,9 @@  struct ethtool_ops {
 	int	(*get_module_eeprom_by_page)(struct net_device *dev,
 					     const struct ethtool_module_eeprom *page,
 					     struct netlink_ext_ack *extack);
+	int	(*set_module_eeprom_by_page)(struct net_device *dev,
+					     const struct ethtool_module_eeprom *page,
+					     struct netlink_ext_ack *extack);
 	void	(*get_eth_phy_stats)(struct net_device *dev,
 				     struct ethtool_eth_phy_stats *phy_stats);
 	void	(*get_eth_mac_stats)(struct net_device *dev,