| Message ID | 20240122084530.32451-2-danieller@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add ability to flash modules' firmware | expand |
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.
> > /** > > - * 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!
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
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
> > 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.
> > 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 --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,