| Message ID | 20210623075925.2610908-1-idosch@idosch.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | ethtool: Add ability to write to transceiver module EEPROMs | expand |
On Wed, Jun 23, 2021 at 10:59:21AM +0300, Ido Schimmel wrote: > From: Ido Schimmel <idosch@nvidia.com> > > This patchset adds write support to transceiver module EEPROMs by > extending the ethtool netlink API. > > Motivation > ========== > > The kernel can currently dump the contents of module EEPROMs to user > space via the ethtool legacy ioctl API or the new netlink API. These > dumps can then be parsed by ethtool(8) according to the specification > that defines the memory map of the EEPROM. For example, SFF-8636 [1] for > QSFP and CMIS [2] for QSFP-DD. > > In addition to read-only elements, these specifications also define > writeable elements that can be used to control the behavior of the > module. For example, controlling whether the module is put in low or > high power mode to limit its power consumption. Hi Ido So power control is part of the specification? All CMIS devices should implement it the same. > The CMIS specification even defines a message exchange mechanism (CDB, > Command Data Block) on top of the module's memory map. This allows the > host to send various commands to the module. For example, to update its > firmware. > This approach allows the kernel to remain ignorant of the various > standards and avoids the need to constantly update the kernel to support > new registers / commands. More importantly, it allows advanced > functionality such as firmware update to be implemented once in user > space and shared across all the drivers that support read and write > access to module EEPROMs. I fear we are opening the door for user space binary blob drivers, which do much more than just firmware upgrade. You seems to say that power control is part of the CMIS standard. So we don't need userspace involved for that. We can implement a library which any MAC driver can share. I also wonder if it is safe to perform firmware upgrade from user space? I've not looked at the code yet, but i assume you only allow write when the interface is down? Otherwise isn't there a danger you can brick the SFP if the MAC accesses it at the same time as when an upgrade is happening? Do you have other concrete use cases for write from user space? In general, we don't allow uncontrolled access to hardware from user space. We add specific, well documented API calls, and expect kernel drivers to implement them. I don't see why SFPs should be different. Standardised parts we can implement in common code. None standard parts we need device/vendor specific code. Which just means we need drivers following the usual linux conventions, some sort of bus driver which reads the vendor/device ID from the EEPROM and probes a driver for the specific SFP. Andrew
On Wed, Jun 23, 2021 at 08:44:57PM +0200, Andrew Lunn wrote: > On Wed, Jun 23, 2021 at 10:59:21AM +0300, Ido Schimmel wrote: > > From: Ido Schimmel <idosch@nvidia.com> > > > > This patchset adds write support to transceiver module EEPROMs by > > extending the ethtool netlink API. > > > > Motivation > > ========== > > > > The kernel can currently dump the contents of module EEPROMs to user > > space via the ethtool legacy ioctl API or the new netlink API. These > > dumps can then be parsed by ethtool(8) according to the specification > > that defines the memory map of the EEPROM. For example, SFF-8636 [1] for > > QSFP and CMIS [2] for QSFP-DD. > > > > In addition to read-only elements, these specifications also define > > writeable elements that can be used to control the behavior of the > > module. For example, controlling whether the module is put in low or > > high power mode to limit its power consumption. > > Hi Ido Hi Andrew, Thanks for the feedback. > > So power control is part of the specification? Yes. See "6.3.2.4 Module Power Mode" in CMIS. > All CMIS devices should implement it the same. The implementation inside the module will probably differ between different vendors, but the interface towards the host (Linux) will be the same as it is standardized by CMIS. > > > The CMIS specification even defines a message exchange mechanism (CDB, > > Command Data Block) on top of the module's memory map. This allows the > > host to send various commands to the module. For example, to update its > > firmware. > > > This approach allows the kernel to remain ignorant of the various > > standards and avoids the need to constantly update the kernel to support > > new registers / commands. More importantly, it allows advanced > > functionality such as firmware update to be implemented once in user > > space and shared across all the drivers that support read and write > > access to module EEPROMs. > > I fear we are opening the door for user space binary blob drivers, No need for multiple drivers. The whole point of these standards is that a single implementation on the host will work across different modules from different vendors. I expect that ethtool(8) will have an implementation which other projects can then use as a reference. Similar to how iproute2 is used as a reference by developers of various interface managers / routing daemons. > which do much more than just firmware upgrade. CMIS indeed allows for much more than firmware update. But again, it is all standardized to ensure a single implementation on the host will suffice. > You seems to say that power control is part of the CMIS standard. So > we don't need userspace involved for that. We can implement a library > which any MAC driver can share. I fail to understand this logic. I would understand pushing functionality into the kernel in order to create an abstraction for user space over different hardware interfaces from different vendors. This is not the case here. Nothing is vendor specific. Not to the host vendor nor to the module vendor. Pushing all this functionality into the kernel will basically mean creating an abstraction over an abstraction. Practically, it means bloating the kernel with several thousands LoC (maybe more) and a lot of new user APIs which we will never be able to remove. TBH, I cannot explain to myself what we stand to gain from that. > > I also wonder if it is safe to perform firmware upgrade from user > space? I have yet to write the relevant code in ethtool(8), but it should be safe. See more below. > I've not looked at the code yet, but i assume you only allow > write when the interface is down? No. Taking it down will mean needless packet loss for the entire time of the firmware download process. After the download process, you need to activate the new image from one of the banks (A/B) via CDB "Run Image" command, but even that can be hit less. See "HitlessRestart" bit which can be queried using "Firmware Management Features" command: " 0: CMD Run Image causes a reset. Traffic is affected. 1: CMD Run Image may reset, but module will do its best to maintain traffic and management states. Data path functions are not reset. " Can be implemented in ethtool(8) using commands such as these: # ethtool --module-fw-query DEVNAME # ethtool --module-fw-dl DEVNAME FILENAME # ethtool --module-fw-run DEVNAME Just a quick example. Yet to design / implement this functionality. Wanted to discuss the kernel interface before starting to pile user space functionality on top. > Otherwise isn't there a danger you can brick the SFP if the MAC > accesses it at the same time as when an upgrade is happening? No. You download the firmware image to a bank, verify it was downloaded correctly and then activate the new image (without affecting traffic, potentially). > > Do you have other concrete use cases for write from user space? The first priority on my list is related to SFF-8636 (QSFP). Specifically, ability to monitor signal to noise ratio and laser temperature. This is done via High Page 20h that can monitor up to 24 parameters. Since the number of parameters that can be monitored is much larger than 24, you need to instruct the module which parameters to monitor. This is done by writing to the "Param Configuration" table on this page. See "6.7.2.5 Parameter Configuration Registers". > > In general, we don't allow uncontrolled access to hardware from user > space. We add specific, well documented API calls, and expect kernel > drivers to implement them. I don't see why SFPs should be > different. Standardised parts we can implement in common code. None > standard parts we need device/vendor specific code. Which just means > we need drivers following the usual linux conventions, some sort of > bus driver which reads the vendor/device ID from the EEPROM and probes > a driver for the specific SFP. I think I touched on all these points above. The symmetric GET interface was implemented following your good advice [1] from a year ago in order to avoid two parsers, in user space and kernel. Creating a netlink API for every little writeable element in these standards will not only mean that the kernel will need to be intimately familiar with these memory maps, but also that it will need to be constantly patched with new user APIs which we will never be able to remove. Given the above is entirely avoidable with a single portable implementation in ethtool(8), I do not think we should go with a kernel implementation. [1] https://lore.kernel.org/netdev/20200630002159.GA597495@lunn.ch/ > > Andrew > > >
> I fail to understand this logic. I would understand pushing > functionality into the kernel in order to create an abstraction for user > space over different hardware interfaces from different vendors. This is > not the case here. Nothing is vendor specific. Not to the host vendor > nor to the module vendor. Hi Ido My worry is, we are opening up an ideal vector for user space drivers for SFPs. And worse still, closed source user space drivers. We have had great success with switchdev, over a hundred supported switches, partially because we have always pushed back against kAPIs which allow user space driver, closed binary blobs etc. We have the choice here. We can add a write method to the kAPI, add open source code to Ethtool using that API, and just accept people are going to abuse the API for all sorts of horrible things in user space. Or we can add more restrictive kAPIs, put more code in the kernel, and probably limit user space doing horrible things. Maybe as a side effect, SFP vendors contribute some open source code, rather than binary blobs? I tend to disagree about adding kAPIs which allow write. But i would like to hear other peoples opinions on this. Andrew
On Thu, Jun 24, 2021 at 10:27:13PM +0200, Andrew Lunn wrote: > > I fail to understand this logic. I would understand pushing > > functionality into the kernel in order to create an abstraction for user > > space over different hardware interfaces from different vendors. This is > > not the case here. Nothing is vendor specific. Not to the host vendor > > nor to the module vendor. > > Hi Ido Hi Andrew, > > My worry is, we are opening up an ideal vector for user space drivers > for SFPs. And worse still, closed source user space drivers. We have > had great success with switchdev, over a hundred supported switches, > partially because we have always pushed back against kAPIs which allow > user space driver, closed binary blobs etc. I don't think it's a correct comparison. Switch ASICs don't have a standardized interface towards the host. It is therefore essential that the kernel will abstract these differences to user space. > > We have the choice here. We can add a write method to the kAPI, add > open source code to Ethtool using that API, and just accept people are > going to abuse the API for all sorts of horrible things in user space. > Or we can add more restrictive kAPIs, put more code in the kernel, and > probably limit user space doing horrible things. Maybe as a side > effect, SFP vendors contribute some open source code, rather than > binary blobs? I didn't see any code or binary blobs from SFP vendors and I'm not sure how they can provide these either. Their goal is - I believe - to sell as much modules as possible to what the standard calls "systems manufactures" / "system integrators". Therefore, they cannot make any assumptions about the I2C connectivity (whether to the ASIC or the CPU), the operating system running on the host and the user interface (ioctl / netlink etc). Given all these moving parts, I don't see how they can provide any tooling. It is in their best interest to simply follow the standard and make the tooling a problem of the "systems manufactures" / "system integrators". In fact, the user who requested this functionality claims: "the cable vendors don't develop the tools to burn the FW since the vendors claim that the CMIS is supported". The user also confirmed that another provider "is able to burn the FW for the cables from different vendors". > > I tend to disagree about adding kAPIs which allow write. But i would > like to hear other peoples opinions on this. > > Andrew >
On Sun, Jun 27, 2021 at 01:33:13PM +0300, Ido Schimmel wrote: > On Thu, Jun 24, 2021 at 10:27:13PM +0200, Andrew Lunn wrote: > > > I fail to understand this logic. I would understand pushing > > > functionality into the kernel in order to create an abstraction for user > > > space over different hardware interfaces from different vendors. This is > > > not the case here. Nothing is vendor specific. Not to the host vendor > > > nor to the module vendor. > > > > Hi Ido > > Hi Andrew, > > > > > My worry is, we are opening up an ideal vector for user space drivers > > for SFPs. And worse still, closed source user space drivers. We have > > had great success with switchdev, over a hundred supported switches, > > partially because we have always pushed back against kAPIs which allow > > user space driver, closed binary blobs etc. > > I don't think it's a correct comparison. Switch ASICs don't have a > standardized interface towards the host. It is therefore essential that > the kernel will abstract these differences to user space. > > > > > We have the choice here. We can add a write method to the kAPI, add > > open source code to Ethtool using that API, and just accept people are > > going to abuse the API for all sorts of horrible things in user space. > > Or we can add more restrictive kAPIs, put more code in the kernel, and > > probably limit user space doing horrible things. Maybe as a side > > effect, SFP vendors contribute some open source code, rather than > > binary blobs? > > I didn't see any code or binary blobs from SFP vendors and I'm not sure > how they can provide these either. Their goal is - I believe - to sell > as much modules as possible to what the standard calls "systems > manufactures" / "system integrators". Therefore, they cannot make any > assumptions about the I2C connectivity (whether to the ASIC or the CPU), > the operating system running on the host and the user interface (ioctl / > netlink etc). > > Given all these moving parts, I don't see how they can provide any > tooling. It is in their best interest to simply follow the standard and > make the tooling a problem of the "systems manufactures" / "system > integrators". In fact, the user who requested this functionality claims: > "the cable vendors don't develop the tools to burn the FW since the > vendors claim that the CMIS is supported". The user also confirmed that > another provider "is able to burn the FW for the cables from different > vendors". Hi Ido This API is not just about CMIS, it covers any I2C connected SFP device. I'm more involved in the lower end, 1G, 2.5G and 10G. Devices in this category seem to be very bad a following the standards. GPON is especially bad, and GPON manufactures don't seem to care their devices don't follow the standard, they assume the Customer Premises Equipment is going to run software to work around whatever issues their specific GPON has, maybe they provide driver code? The API you are adding would be ideal for putting that driver in user space, as a binary blob. That is going to make it harder for us to open up the many millions of CPE used in FTTH. And there are people attempting to do that. If devices following CMIS really are closely following the standard that is great. We should provide tooling to do firmware upgrade. But at the same time, we don't want to aid those who go against the standards and do their own thing. And it sounds like in the CMIS world, we might have the power to encourage vendors to follow CMIS, "Look, firmware upgrade just works for the competitors devices, why should i use your device when it does not work?" I just want to make sure we are considering the full range of devices this new API will cover. From little ARM systems with 1G copper and FTTH fibre ports through to big TOR systems with large number of 100G ports. If CMIS is well support by vendors, putting the code into the kernel, as a loadable module, might be the better solution for the whole range of devices the kernel needs to support. Andrew
On Sun, Jun 27, 2021 at 05:12:26PM +0200, Andrew Lunn wrote: > This API is not just about CMIS, it covers any I2C connected SFP > device. I'm more involved in the lower end, 1G, 2.5G and 10G. Devices > in this category seem to be very bad a following the standards. GPON > is especially bad, and GPON manufactures don't seem to care their > devices don't follow the standard, they assume the Customer Premises > Equipment is going to run software to work around whatever issues > their specific GPON has, maybe they provide driver code? The API you > are adding would be ideal for putting that driver in user space, as a > binary blob. That is going to make it harder for us to open up the > many millions of CPE used in FTTH. And there are people attempting to > do that. > > If devices following CMIS really are closely following the standard > that is great. We should provide tooling to do firmware upgrade. But > at the same time, we don't want to aid those who go against the > standards and do their own thing. And it sounds like in the CMIS > world, we might have the power to encourage vendors to follow CMIS, > "Look, firmware upgrade just works for the competitors devices, why > should i use your device when it does not work?" > > I just want to make sure we are considering the full range of devices > this new API will cover. From little ARM systems with 1G copper and > FTTH fibre ports through to big TOR systems with large number of 100G > ports. If CMIS is well support by vendors, putting the code into the > kernel, as a loadable module, might be the better solution for the > whole range of devices the kernel needs to support. If the goal is to limit what user space can do, then putting all the code in the kernel and adding an ever-growing number of restrictive user APIs is not the only way. Even with the proposed approach, the kernel sits in the middle between the module and user space. As such, it can maintain an "allow list" that only allows access to modules with a specific memory map (CMIS and SFF-8636 for now) and only to a subset of the pages which are standardized by the specifications.
> Even with the proposed approach, the kernel sits in the middle between > the module and user space. As such, it can maintain an "allow list" that > only allows access to modules with a specific memory map (CMIS and > SFF-8636 for now) and only to a subset of the pages which are > standardized by the specifications. Hi Ido This seems like a reasonable compromise. But i would go further. Limit it to just what is needed for firmware upgrade. Andrew
On Thu, 24 Jun 2021 22:38:27 +0300 Ido Schimmel wrote: > > I also wonder if it is safe to perform firmware upgrade from user > > space? > > I have yet to write the relevant code in ethtool(8), but it should be > safe. See more below. What's unclear to me is what difference does it make whether the code is in kernel or in ethtool.git. If modules follow the standard the code churn will be low. And we have to type the code up anyway it seems it doesn't matter where it lies. Where it does matter is if one wants to reuse existing user space built on top of raw read/write interfaces. IOW it shouldn't matter if we put the code in the kernel or user space from effort perspective, but the latter is more risky. You mention some limited sensor resources, isn't this exactly the resource management the kernel is supposed to perform?
On Tuesday 29 June 2021 15:47:33 Andrew Lunn wrote: > > Even with the proposed approach, the kernel sits in the middle between > > the module and user space. As such, it can maintain an "allow list" that > > only allows access to modules with a specific memory map (CMIS and > > SFF-8636 for now) and only to a subset of the pages which are > > standardized by the specifications. > > Hi Ido > > This seems like a reasonable compromise. But i would go further. Limit > it to just what is needed for firmware upgrade. > > Andrew Hello! If this is just because for CMIS firmware upgrade, what about rather providing kernel driver for CMIS firmware upgrade?
On Sunday 27 June 2021 13:33:13 Ido Schimmel wrote: > On Thu, Jun 24, 2021 at 10:27:13PM +0200, Andrew Lunn wrote: > > We have the choice here. We can add a write method to the kAPI, add > > open source code to Ethtool using that API, and just accept people are > > going to abuse the API for all sorts of horrible things in user space. > > Or we can add more restrictive kAPIs, put more code in the kernel, and > > probably limit user space doing horrible things. Maybe as a side > > effect, SFP vendors contribute some open source code, rather than > > binary blobs? > > I didn't see any code or binary blobs from SFP vendors and I'm not sure > how they can provide these either. Their goal is - I believe - to sell > as much modules as possible to what the standard calls "systems > manufactures" / "system integrators". Therefore, they cannot make any > assumptions about the I2C connectivity (whether to the ASIC or the CPU), > the operating system running on the host and the user interface (ioctl / > netlink etc). Hello! This is really happening in GPON world. Most GPON SFP modules are working only in few devices with SFP cages. And it is either because GPON SFP module vendor provided to vendor of device with SFP cage how to "hack", initialize and use that GPON module properly or because we have figured out how particular GPON modules violate SFF standards and added special quirks per SFP module or per chipset in SFP module. And the other thing which is happening. If vendor of GPON SFP module is the same as vendor of device with SFP cage then it is doing everything to ensure that only its GPON SFP modules would work in its devices. Or to ensure that its OLT station (opposite end of GPON client SFP module) would link only with its branded GPON SFP modules. Classic vendor lockin. If ISP is using OLT station from vendor A then A wants that you cannot use GPON SFP modules from vendor B on that network. You said that vendor goal is to sell as much modules as possible. Seem that this is truth and vendors are doing it by above vendor lockin strategy only. So at the end I really do not like raw RW access to SFP EEPROM. This just opens a new door for vendor lockin and vendor blob strategy. And the last thing is that rewriting EEPROM on arbitrary SFP module may lead to total damage of SFP. Specially on SFPs with "computer inside" where parts of (critical) memory is shadowed in SFP EEPROM space.
From: Ido Schimmel <idosch@nvidia.com> This patchset adds write support to transceiver module EEPROMs by extending the ethtool netlink API. Motivation ========== The kernel can currently dump the contents of module EEPROMs to user space via the ethtool legacy ioctl API or the new netlink API. These dumps can then be parsed by ethtool(8) according to the specification that defines the memory map of the EEPROM. For example, SFF-8636 [1] for QSFP and CMIS [2] for QSFP-DD. In addition to read-only elements, these specifications also define writeable elements that can be used to control the behavior of the module. For example, controlling whether the module is put in low or high power mode to limit its power consumption. The CMIS specification even defines a message exchange mechanism (CDB, Command Data Block) on top of the module's memory map. This allows the host to send various commands to the module. For example, to update its firmware. Implementation ============== The legacy ioctl API to dump module EEPROMs required drivers to parse the contents of the EEPROM in order to understand how many bytes can be read and dumped to user space. This meant that drivers had to be updated to support new standards. See [3], for example. To overcome this limitation, a new netlink-based API to dump module EEPROMs was merged in kernel 5.13 [4]. With the new API, the kernel is merely responsible for fetching EEPROM pages. User space then parses the information, determines if more pages are available and instructs the kernel to fetch them as well. Write support for module EEPROMs employs the same approach. User space instructs the kernel which bytes (page/offset/bank/length) to change and to which values. This approach allows the kernel to remain ignorant of the various standards and avoids the need to constantly update the kernel to support new registers / commands. More importantly, it allows advanced functionality such as firmware update to be implemented once in user space and shared across all the drivers that support read and write access to module EEPROMs. The above is achieved by adding a new command to the generic ethtool netlink family ('ETHTOOL_MSG_MODULE_EEPROM_SET') which shares the same attributes with the get command ('ETHTOOL_MSG_MODULE_EEPROM_GET'). See Documentation/networking/ethtool-netlink.rst in patch #3 for detailed description of the proposed netlink API. Note that the new command shares the same restrictions with the existing get command. This means, for example, that no more than 128 bytes can be written at once and that cross-page write is forbidden. However, some CMIS compliant modules might support "Auto Paging" which allows hosts to "write data in large chunks, without the overhead of explicitly programming Page changes" [2]. At this time, I cannot evaluate the benefits of "Auto Paging" as I do not have modules that support the feature, nor a host that can write more than 48 bytes at once. If the current restrictions prove to be a bottleneck, they can be relaxed in the future. ethtool(8) support ================== The corresponding user space patches extend ethtool(8) with the ability to change the value of a single byte in the module EEPROM. Example: # ethtool -M swp11 offset 0x80 page 3 bank 0 i2c 0x50 value 0x44 This is in accordance with the '-E' option which allows changing the value of a single byte in the EEPROM of the network device. The current command line interface is not user-friendly and also impractical for functionality that requires many reads and writes such as firmware update. Therefore, the plan is to extend ethtool(8) over time with commonly requested functionality on top of the netlink API. Testing ======= Tested by writing to page 3 (User EEPROM) of a QSFP-DD module: # ethtool -m swp11 offset 0x80 length 3 page 3 bank 0 i2c 0x50 Offset Values ------ ------ 0x0080: 00 00 00 # ethtool -M swp11 offset 0x80 page 3 bank 0 i2c 0x50 value 0x44 # ethtool -M swp11 offset 0x81 page 3 bank 0 i2c 0x50 value 0x41 # ethtool -M swp11 offset 0x82 page 3 bank 0 i2c 0x50 value 0x44 # ethtool -m swp11 offset 0x80 length 3 page 3 bank 0 i2c 0x50 Offset Values ------ ------ 0x0080: 44 41 44 Patchset overview ================= Patches #1-#2 refactor the ethtool module EEPROM code to allow sharing attribute validation between read and write. Patch #3 adds the actual module EEPROM write implementation. Patch #4 adds mlxsw support. [1] https://members.snia.org/document/dl/26418 [2] http://www.qsfp-dd.com/wp-content/uploads/2021/05/CMIS5p0.pdf [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6af496adcbb8d4656b90a85401eeceb88d520c0d [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7dc85b599ae17fb705ffae1b7321ace4b3056aeb Ido Schimmel (4): ethtool: Extract module EEPROM attributes before validation ethtool: Split module EEPROM attributes validation to a function ethtool: Add ability to write to transceiver module EEPROM mlxsw: core: Add support for module EEPROM write by page Documentation/networking/ethtool-netlink.rst | 47 +++++ .../net/ethernet/mellanox/mlxsw/core_env.c | 44 ++++ .../net/ethernet/mellanox/mlxsw/core_env.h | 5 + drivers/net/ethernet/mellanox/mlxsw/minimal.c | 13 ++ .../mellanox/mlxsw/spectrum_ethtool.c | 14 ++ include/linux/ethtool.h | 21 +- include/uapi/linux/ethtool_netlink.h | 2 + net/ethtool/eeprom.c | 192 +++++++++++++++--- net/ethtool/netlink.c | 7 + net/ethtool/netlink.h | 2 + 10 files changed, 316 insertions(+), 31 deletions(-)