| Message ID | 1614884228-8542-1-git-send-email-moshe@nvidia.com |
|---|---|
| Headers | show |
| Series | ethtool: Extend module EEPROM dump API | expand |
On Thu, Mar 04, 2021 at 10:57AM-0800, Moshe Shemesh wrote: > Ethtool supports module EEPROM dumps via the `ethtool -m <dev>` > command. > But in current state its functionality is limited - offset and length parameters, > which are used to specify a linear desired region of EEPROM data to dump, is > not enough, considering emergence of complex module EEPROM layouts > such as CMIS 4.0. > Moreover, CMIS 4.0 extends the amount of pages that may be accessible by > introducing another parameter for page addressing - banks. This is nice work, addressing the banks problem (though there are no devices with bank switching yet?) I suggest this change increase the maximum size of EEPROM to the maximum the architecture allows. That's 256 pages (128 bytes) plus the lower page for a total of 257*256 bytes. SFP devices can access another 256 bytes since they use two i2c addresses but only one of them is paged. The size increase is necessary for bank support since banked pages are all above the current 640 byte limit. Note that the SFF-* specs do not specify what is in pages above page 3 (except CMIS), but they DO specify that those pages are available for proprietary uses by module vendors. I will call out these changes in the following patches. Ethtool also supports module 'change-eeprom', a write function mirroring the dump function. That path needs to be implemented too. There are some very interesting proprietary tricks that some modules can do by writing the right magic to the right registers, some of which are on pages in the 0xF0 range. > > Besides, currently module EEPROM is represented as a chunk of > concatenated pages, where lower 128 bytes of all pages, except page 00h, > are omitted. Offset and length are used to address parts of this fake linear > memory. But in practice drivers, which implement > get_module_info() and get_module_eeprom() ethtool ops still calculate > page number and set I2C address on their own. > > This series tackles these issues by adding ethtool op, which allows to pass > page number, bank number and I2C address in addition to offset and length > parameters to the driver, adds corresponding netlink infrastructure and > implements the new interface in mlx5 driver. > > This allows to extend userspace 'ethtool -m' CLI by adding new parameters - > page, bank and i2c. New command line format: > ethtool -m <dev> [hex on|off] [raw on|off] [offset N] [length N] [page N] > [bank N] [i2c N] > > The consequence of this series is a possibility to dump arbitrary EEPROM > page at a time, in contrast to dumps of concatenated pages. Therefore, > offset and length change their semantics and may be used only to specify a > part of data within a page, which size is currently limited to 256 bytes. Just to be clear, if you define a page to be 256 bytes, and only specify offset within a page, then offset 0-127 is the same for every page on the device, and useful offsets for each page start at 128. This can be confusing, but I think it is the right approach. > > As for backwards compatibility with get_module_info() and > get_module_eeprom() pair, the series addresses it as well by implementing > a fallback mechanism. As mentioned earlier, drivers derive a page number > from 'global' offset, so this can be done vice versa without their involvement > thanks to standardization. If kernel netlink handler of 'ethtool -m' command > detects that new ethtool op is not supported by the driver, it calculates > offset from given page number and page offset and calls old ndos, if they are > available. > > Change log: > v1 -> v2: > - Limited i2c_address values by 127 > - Added page bound check for offset and length > - Added defines for these two points > - Added extack to ndo parameters > - Moved ethnl_ops_begin(dev) and set error path accordingly > > > > Vladyslav Tarasiuk (5): > ethtool: Allow network drivers to dump arbitrary EEPROM data > net/mlx5: Refactor module EEPROM query > net/mlx5: Implement get_module_eeprom_data_by_page() > net/mlx5: Add support for DSFP module EEPROM dumps > ethtool: Add fallback to get_module_eeprom from netlink command > > .../ethernet/mellanox/mlx5/core/en_ethtool.c | 42 +++ > .../net/ethernet/mellanox/mlx5/core/port.c | 101 ++++++-- > include/linux/ethtool.h | 7 +- > include/linux/mlx5/port.h | 12 + > include/uapi/linux/ethtool.h | 26 ++ > include/uapi/linux/ethtool_netlink.h | 19 ++ > net/ethtool/Makefile | 2 +- > net/ethtool/eeprom.c | 239 ++++++++++++++++++ > net/ethtool/netlink.c | 10 + > net/ethtool/netlink.h | 2 + > 10 files changed, 430 insertions(+), 30 deletions(-) create mode 100644 > net/ethtool/eeprom.c > > -- > 2.18.2 Don Bollinger
On Thu, Mar 04, 2021 at 10:57AM-0800, Moshe Shemesh wrote: > From: Vladyslav Tarasiuk <vladyslavt@nvidia.com> > > Define get_module_eeprom_data_by_page() ethtool callback and > implement netlink infrastructure. > > get_module_eeprom_data_by_page() allows network drivers to dump a > part of module's EEPROM specified by page and bank numbers along with > offset and length. It is effectively a netlink replacement for > get_module_info() and get_module_eeprom() pair, which is needed due to > emergence of complex non-linear EEPROM layouts. > > Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com> > --- > include/linux/ethtool.h | 7 +- > include/uapi/linux/ethtool.h | 26 +++++ > include/uapi/linux/ethtool_netlink.h | 19 ++++ > net/ethtool/Makefile | 2 +- > net/ethtool/eeprom.c | 157 +++++++++++++++++++++++++++ > net/ethtool/netlink.c | 10 ++ > net/ethtool/netlink.h | 2 + > 7 files changed, 221 insertions(+), 2 deletions(-) create mode 100644 > net/ethtool/eeprom.c > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index > ec4cd3921c67..2f65aae5f492 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -81,6 +81,7 @@ enum { > #define ETH_RSS_HASH_NO_CHANGE 0 > > struct net_device; > +struct netlink_ext_ack; > > /* Some generic methods drivers may use in their ethtool_ops */ > u32 ethtool_op_get_link(struct net_device *dev); @@ -410,6 +411,8 @@ > struct ethtool_pause_stats { > * @get_ethtool_phy_stats: Return extended statistics about the PHY > device. > * This is only useful if the device maintains PHY statistics and > * cannot use the standard PHY library helpers. > + * @get_module_eeprom_data_by_page: Get a region of plug-in module > EEPROM data > + * from specified page. Returns a negative error code or zero. > * > * All operations are optional (i.e. the function pointer may be set > * to %NULL) and callers must take this into account. Callers must @@ -515,6 > +518,9 @@ struct ethtool_ops { > const struct ethtool_tunable *, void *); > int (*set_phy_tunable)(struct net_device *, > const struct ethtool_tunable *, const void > *); > + int (*get_module_eeprom_data_by_page)(struct net_device > *dev, > + const struct > ethtool_eeprom_data *page, > + struct netlink_ext_ack > *extack); > }; > > int ethtool_check_ops(const struct ethtool_ops *ops); @@ -538,7 +544,6 > @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev, > const struct ethtool_link_ksettings *cmd, > u32 *dev_speed, u8 *dev_duplex); > > -struct netlink_ext_ack; > struct phy_device; > struct phy_tdr_config; > > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index > cde753bb2093..2459571fc1d1 100644 > --- a/include/uapi/linux/ethtool.h > +++ b/include/uapi/linux/ethtool.h > @@ -340,6 +340,28 @@ struct ethtool_eeprom { > __u8 data[0]; > }; > > +/** > + * struct ethtool_eeprom_data - EEPROM dump from specified page > + * @offset: Offset within the specified EEPROM page to begin read, in > bytes. Note here that bytes at offset 0-127 are the same for every page of the module, only bytes at offset 128 and higher are actually paged. > + * @length: Number of bytes to read. > + * @page: Page number to read from. > + * @bank: Page bank number to read from, if applicable by EEPROM spec. > + * @i2c_address: I2C address of a page. Value less than 0x7f expected. > Most > + * EEPROMs use 0x50 or 0x51. The standards are all very clear, the only legal values are 0x50 and 0x51. It isn't 'expected', it is required. I suggest that 0xA0 and 0xA2 also be silently accepted, and translated to 0x50 and 0x51 respectively. Some of the specs use A0/A2 instead of 0x50/0x51. They actually mean the same thing. > + * @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_eeprom_data { > + __u32 offset; > + __u32 length; > + __u32 page; > + __u32 bank; > + __u32 i2c_address; > + __u8 *data; > +}; > + > /** > * struct ethtool_eee - Energy Efficient Ethernet information > * @cmd: ETHTOOL_{G,S}EEE > @@ -1865,6 +1887,10 @@ static inline int ethtool_validate_duplex(__u8 > duplex) > #define ETH_MODULE_SFF_8636_MAX_LEN 640 > #define ETH_MODULE_SFF_8436_MAX_LEN 640 > > +#define ETH_MODULE_EEPROM_MAX_LEN 640 Please don't add this MAX_LEN constant. Even better, remove the two above it as well. The proper value for all 3 of these MAX_LEN items is the architectural limit imposed by the 8 bit page register plus the constant lower page (hence 257*128 bytes). The 8436 and 8636 specs do not actually limit these devices to 640 bytes (3 pages). There is no MAX_LEN listed for SFF_8472. If there is one, it should actually be 259 * 128 bytes (to account for 256 more bytes on the unpaged 0x50 i2c address). Nor is there one for CMIS. The maximum architected length for CMIS is (257*128) + (127 * 16 * 128). That's the QSFP max length plus 127 more banks of 16 pages. > +#define ETH_MODULE_EEPROM_PAGE_LEN 256 > +#define ETH_MODULE_MAX_I2C_ADDRESS 0x7f Actually there are only two legal values for the i2c address (0x50, 0x51). Rather than defining a MAX address, consider defining the legal values, or... is it used at all? Leave it out? > + > /* Reset flags */ > /* The reset() operation must clear the flags for the components which > * were actually reset. On successful return, the flags indicate the diff --git > a/include/uapi/linux/ethtool_netlink.h > b/include/uapi/linux/ethtool_netlink.h > index a286635ac9b8..60dd848d0b54 100644 > --- a/include/uapi/linux/ethtool_netlink.h > +++ b/include/uapi/linux/ethtool_netlink.h > @@ -42,6 +42,7 @@ enum { > ETHTOOL_MSG_CABLE_TEST_ACT, > ETHTOOL_MSG_CABLE_TEST_TDR_ACT, > ETHTOOL_MSG_TUNNEL_INFO_GET, > + ETHTOOL_MSG_EEPROM_DATA_GET, > > /* add new constants above here */ > __ETHTOOL_MSG_USER_CNT, > @@ -80,6 +81,7 @@ enum { > ETHTOOL_MSG_CABLE_TEST_NTF, > ETHTOOL_MSG_CABLE_TEST_TDR_NTF, > ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY, > + ETHTOOL_MSG_EEPROM_DATA_GET_REPLY, > > /* add new constants above here */ > __ETHTOOL_MSG_KERNEL_CNT, > @@ -629,6 +631,23 @@ enum { > ETHTOOL_A_TUNNEL_INFO_MAX = > (__ETHTOOL_A_TUNNEL_INFO_CNT - 1) }; > > +/* MODULE EEPROM DATA */ > + > +enum { > + ETHTOOL_A_EEPROM_DATA_UNSPEC, > + ETHTOOL_A_EEPROM_DATA_HEADER, > + > + ETHTOOL_A_EEPROM_DATA_OFFSET, > + ETHTOOL_A_EEPROM_DATA_LENGTH, > + ETHTOOL_A_EEPROM_DATA_PAGE, > + ETHTOOL_A_EEPROM_DATA_BANK, > + ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS, > + ETHTOOL_A_EEPROM_DATA, > + > + __ETHTOOL_A_EEPROM_DATA_CNT, > + ETHTOOL_A_EEPROM_DATA_MAX = > (__ETHTOOL_A_EEPROM_DATA_CNT - 1) }; > + > /* generic netlink info */ > #define ETHTOOL_GENL_NAME "ethtool" > #define ETHTOOL_GENL_VERSION 1 > diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile index > 7a849ff22dad..d604346bc074 100644 > --- a/net/ethtool/Makefile > +++ b/net/ethtool/Makefile > @@ -7,4 +7,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o > ethtool_nl-y := netlink.o bitset.o strset.o linkinfo.o linkmodes.o \ > linkstate.o debug.o wol.o features.o privflags.o rings.o \ > channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \ > - tunnels.o > + tunnels.o eeprom.o > diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c new file mode > 100644 index 000000000000..2618a55b9a40 > --- /dev/null > +++ b/net/ethtool/eeprom.c > @@ -0,0 +1,157 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +#include <linux/ethtool.h> > +#include "netlink.h" > +#include "common.h" > + > +struct eeprom_data_req_info { > + struct ethnl_req_info base; > + u32 offset; > + u32 length; > + u32 page; > + u32 bank; > + u32 i2c_address; > +}; > + > +struct eeprom_data_reply_data { > + struct ethnl_reply_data base; > + u32 length; > + u32 i2c_address; > + u8 *data; > +}; > + > +#define EEPROM_DATA_REQINFO(__req_base) \ > + container_of(__req_base, struct eeprom_data_req_info, base) > + > +#define EEPROM_DATA_REPDATA(__reply_base) \ > + container_of(__reply_base, struct eeprom_data_reply_data, base) > + > +static int eeprom_data_prepare_data(const struct ethnl_req_info > *req_base, > + struct ethnl_reply_data *reply_base, > + struct genl_info *info) > +{ > + struct eeprom_data_reply_data *reply = > EEPROM_DATA_REPDATA(reply_base); > + struct eeprom_data_req_info *request = > EEPROM_DATA_REQINFO(req_base); > + struct ethtool_eeprom_data page_data = {0}; > + struct net_device *dev = reply_base->dev; > + int err; > + > + if (!dev->ethtool_ops->get_module_eeprom_data_by_page) > + return -EOPNOTSUPP; > + > + page_data.offset = request->offset; > + page_data.length = request->length; > + page_data.i2c_address = request->i2c_address; > + page_data.page = request->page; > + page_data.bank = request->bank; > + page_data.data = kmalloc(page_data.length, GFP_KERNEL); > + if (!page_data.data) > + return -ENOMEM; > + err = ethnl_ops_begin(dev); > + if (err) > + goto err_free; > + > + err = dev->ethtool_ops->get_module_eeprom_data_by_page(dev, > &page_data, > + info->extack); > + if (err) > + goto err_ops; > + > + reply->length = page_data.length; > + reply->i2c_address = page_data.i2c_address; > + reply->data = page_data.data; > + > + ethnl_ops_complete(dev); The two error paths below kfree(page_data.data). Does someone else free this memory when there is no error? > + return 0; > + > +err_ops: > + ethnl_ops_complete(dev); > +err_free: > + kfree(page_data.data); > + return err; > +} > + > +static int eeprom_data_parse_request(struct ethnl_req_info *req_info, > struct nlattr **tb, > + struct netlink_ext_ack *extack) { > + struct eeprom_data_req_info *request = > EEPROM_DATA_REQINFO(req_info); > + > + if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] || > + !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] || > + !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]) > + return -EINVAL; > + > + request->i2c_address = > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]); > + if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS) > + return -EINVAL; I would be much more restrictive, with one flexibility... if (request->i2c_address == 0xA0) request->i2c_address = 0x50; if (request->i2c_address == 0xA2) request->i2c_address = 0x51; if (request->i2c_address < 0x50) || (request->i2c_address > 0x51) return -EINVAL; > + > + request->offset = > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]); > + request->length = > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]); > + if (request->length > ETH_MODULE_EEPROM_MAX_LEN) > + return -EINVAL; This is really problematic as there are MANY different max values, within the specs, for the various EEPROMs being generically supported here. I would leave it to the drivers to handle out-of-range requests. If you really want to check, you need to know which spec the module supports, whether it supports pages, and whether it supports banks. I have not found a register that actually reports the number of supported pages that an eeprom supports. The specs should have included that :-(. > + if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] && > + request->offset + request->length > > ETH_MODULE_EEPROM_PAGE_LEN) > + return -EINVAL; Why does this stanza depend on DATA_PAGE? In this new data structure, no requests can cross the 256 byte page boundary. I suggest, rather then -EINVAL, you should reduce the length to reach the end of the page: if (request->offset + request->length) > ETH_MODULE_EEPROM_PAGE_LEN) request->length = ETH_MODULE_EEPROM_PAGE_LEN - request->offset. Note that this matches the choice you made to truncate rather than error out in fallback_set_parms(). > + > + if (tb[ETHTOOL_A_EEPROM_DATA_PAGE]) > + request->page = > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_PAGE]); > + if (tb[ETHTOOL_A_EEPROM_DATA_BANK]) > + request->bank = > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_BANK]); Other checks: Page and bank have to be between 0 and 255 (inclusive), they go into an 8 bit register in the eeprom. Offset and length can't be negative. > + > + return 0; > +} > + > +static int eeprom_data_reply_size(const struct ethnl_req_info *req_base, > + const struct ethnl_reply_data *reply_base) { > + const struct eeprom_data_req_info *request = > +EEPROM_DATA_REQINFO(req_base); > + > + return nla_total_size(sizeof(u32)) + /* _EEPROM_DATA_LENGTH */ > + nla_total_size(sizeof(u32)) + /* _EEPROM_DATA_I2C_ADDRESS > */ > + nla_total_size(sizeof(u8) * request->length); /* _EEPROM_DATA > +*/ } > + > +static int eeprom_data_fill_reply(struct sk_buff *skb, > + const struct ethnl_req_info *req_base, > + const struct ethnl_reply_data *reply_base) { > + struct eeprom_data_reply_data *reply = > +EEPROM_DATA_REPDATA(reply_base); > + > + if (nla_put_u32(skb, ETHTOOL_A_EEPROM_DATA_LENGTH, reply- > >length) || > + nla_put_u32(skb, ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS, > reply->i2c_address) || > + nla_put(skb, ETHTOOL_A_EEPROM_DATA, reply->length, reply- > >data)) > + return -EMSGSIZE; > + > + return 0; > +} > + > +static void eeprom_data_cleanup_data(struct ethnl_reply_data > +*reply_base) { > + struct eeprom_data_reply_data *reply = > +EEPROM_DATA_REPDATA(reply_base); > + > + kfree(reply->data); > +} > + > +const struct ethnl_request_ops ethnl_eeprom_data_request_ops = { > + .request_cmd = ETHTOOL_MSG_EEPROM_DATA_GET, > + .reply_cmd = > ETHTOOL_MSG_EEPROM_DATA_GET_REPLY, > + .hdr_attr = ETHTOOL_A_EEPROM_DATA_HEADER, > + .req_info_size = sizeof(struct eeprom_data_req_info), > + .reply_data_size = sizeof(struct eeprom_data_reply_data), > + > + .parse_request = eeprom_data_parse_request, > + .prepare_data = eeprom_data_prepare_data, > + .reply_size = eeprom_data_reply_size, > + .fill_reply = eeprom_data_fill_reply, > + .cleanup_data = eeprom_data_cleanup_data, > +}; > + > +const struct nla_policy ethnl_eeprom_data_get_policy[] = { > + [ETHTOOL_A_EEPROM_DATA_HEADER] = > NLA_POLICY_NESTED(ethnl_header_policy), > + [ETHTOOL_A_EEPROM_DATA_OFFSET] = { .type = > NLA_U32 }, > + [ETHTOOL_A_EEPROM_DATA_LENGTH] = { .type = > NLA_U32 }, > + [ETHTOOL_A_EEPROM_DATA_PAGE] = { .type = NLA_U32 }, > + [ETHTOOL_A_EEPROM_DATA_BANK] = { .type = NLA_U32 }, > + [ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS] = { .type = NLA_U32 }, > + [ETHTOOL_A_EEPROM_DATA] = { .type = > NLA_BINARY }, > +}; > + > diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index > 50d3c8896f91..ff2528bee192 100644 > --- a/net/ethtool/netlink.c > +++ b/net/ethtool/netlink.c > @@ -245,6 +245,7 @@ > ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = { > [ETHTOOL_MSG_PAUSE_GET] = > ðnl_pause_request_ops, > [ETHTOOL_MSG_EEE_GET] = ðnl_eee_request_ops, > [ETHTOOL_MSG_TSINFO_GET] = ðnl_tsinfo_request_ops, > + [ETHTOOL_MSG_EEPROM_DATA_GET] = > ðnl_eeprom_data_request_ops, > }; > > static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback > *cb) @@ -912,6 +913,15 @@ static const struct genl_ops ethtool_genl_ops[] > = { > .policy = ethnl_tunnel_info_get_policy, > .maxattr = ARRAY_SIZE(ethnl_tunnel_info_get_policy) - 1, > }, > + { > + .cmd = ETHTOOL_MSG_EEPROM_DATA_GET, > + .doit = ethnl_default_doit, > + .start = ethnl_default_start, > + .dumpit = ethnl_default_dumpit, > + .done = ethnl_default_done, > + .policy = ethnl_eeprom_data_get_policy, > + .maxattr = ARRAY_SIZE(ethnl_eeprom_data_get_policy) - 1, > + }, > }; > > static const struct genl_multicast_group ethtool_nl_mcgrps[] = { diff --git > a/net/ethtool/netlink.h b/net/ethtool/netlink.h index > 6eabd58d81bf..60954c7b4dfe 100644 > --- a/net/ethtool/netlink.h > +++ b/net/ethtool/netlink.h > @@ -344,6 +344,7 @@ extern const struct ethnl_request_ops > ethnl_coalesce_request_ops; extern const struct ethnl_request_ops > ethnl_pause_request_ops; extern const struct ethnl_request_ops > ethnl_eee_request_ops; extern const struct ethnl_request_ops > ethnl_tsinfo_request_ops; > +extern const struct ethnl_request_ops ethnl_eeprom_data_request_ops; > > extern const struct nla_policy > ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1]; extern const struct > nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1]; @@ > -375,6 +376,7 @@ extern const struct nla_policy > ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_HEADER + extern const struct > nla_policy ethnl_cable_test_act_policy[ETHTOOL_A_CABLE_TEST_HEADER + > 1]; extern const struct nla_policy > ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_CFG + 1]; > extern const struct nla_policy > ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_HEADER + 1]; > +extern const struct nla_policy > +ethnl_eeprom_data_get_policy[ETHTOOL_A_EEPROM_DATA + 1]; > > int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info); int > ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info); > -- > 2.18.2
> > + * @length: Number of bytes to read. > > + * @page: Page number to read from. > > + * @bank: Page bank number to read from, if applicable by EEPROM spec. > > + * @i2c_address: I2C address of a page. Value less than 0x7f expected. > > Most > > + * EEPROMs use 0x50 or 0x51. > > The standards are all very clear Our experience so far is that manufactures of SFP modules like to ignore the standard. And none of the standards seem to cover copper modules, which have additional registers at some other page. Admittedly, they cannot be mapped as pages, you need some proprietary protocol to map MDIO onto I2C. But i would not be surprised to find some SFP that maps the FLASH of the microcontroller onto an address, which we might be able to read out using this API. So i suggested we keep it generic, allowing access to these proprietary registers at other addresses. And if there is nothing there, you probably get a 1/2 page of 0xff. > I suggest that 0xA0 and 0xA2 also be silently accepted, and > translated to 0x50 and 0x51 respectively. No, i don't like having two different values mean the same thing. It just leads to confusion. And userspace is going to be confused when it asks for 0xA0 but the reply says it is for 0x50. The Linux I2C subsystem does not magically map 8bit addresses in 7bit addresses. We should follow what the Linux I2C subsystem does. > > + > > + request->offset = > > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]); > > + request->length = > > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]); > > + if (request->length > ETH_MODULE_EEPROM_MAX_LEN) > > + return -EINVAL; > > This is really problematic as there are MANY different max values, within > the specs I agree. We should only be returning one 1/2 page as a maximum. So it should be limited to 128 bytes. And offset+length should not go beyond the end of a 1/2 page. > > + if (tb[ETHTOOL_A_EEPROM_DATA_PAGE]) > > + request->page = > > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_PAGE]); > > + if (tb[ETHTOOL_A_EEPROM_DATA_BANK]) > > + request->bank = > > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_BANK]); > > Other checks: > > Page and bank have to be between 0 and 255 (inclusive), they > go into an 8 bit register in the eeprom. Yes, a u8 would be a better type here. Andrew
> +/* MODULE EEPROM DATA */ > + > +enum { > + ETHTOOL_A_EEPROM_DATA_UNSPEC, > + ETHTOOL_A_EEPROM_DATA_HEADER, > + > + ETHTOOL_A_EEPROM_DATA_OFFSET, > + ETHTOOL_A_EEPROM_DATA_LENGTH, > + ETHTOOL_A_EEPROM_DATA_PAGE, > + ETHTOOL_A_EEPROM_DATA_BANK, > + ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS, > + ETHTOOL_A_EEPROM_DATA, If you look at all the other such enums in ethtool_netlink, you will see a comment indicating the type. Please add them here as well. Please also update Documentation/networking/ethtool-netlink.rst. Andrew
On 3/5/2021 2:50 AM, Don Bollinger wrote: > > On Thu, Mar 04, 2021 at 10:57AM-0800, Moshe Shemesh wrote: >> From: Vladyslav Tarasiuk <vladyslavt@nvidia.com> >> >> Define get_module_eeprom_data_by_page() ethtool callback and >> implement netlink infrastructure. >> >> get_module_eeprom_data_by_page() allows network drivers to dump a >> part of module's EEPROM specified by page and bank numbers along with >> offset and length. It is effectively a netlink replacement for >> get_module_info() and get_module_eeprom() pair, which is needed due to >> emergence of complex non-linear EEPROM layouts. >> >> Signed-off-by: Vladyslav Tarasiuk <vladyslavt@nvidia.com> >> --- >> include/linux/ethtool.h | 7 +- >> include/uapi/linux/ethtool.h | 26 +++++ >> include/uapi/linux/ethtool_netlink.h | 19 ++++ >> net/ethtool/Makefile | 2 +- >> net/ethtool/eeprom.c | 157 +++++++++++++++++++++++++++ >> net/ethtool/netlink.c | 10 ++ >> net/ethtool/netlink.h | 2 + >> 7 files changed, 221 insertions(+), 2 deletions(-) create mode 100644 >> net/ethtool/eeprom.c >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index >> ec4cd3921c67..2f65aae5f492 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -81,6 +81,7 @@ enum { >> #define ETH_RSS_HASH_NO_CHANGE 0 >> >> struct net_device; >> +struct netlink_ext_ack; >> >> /* Some generic methods drivers may use in their ethtool_ops */ >> u32 ethtool_op_get_link(struct net_device *dev); @@ -410,6 +411,8 @@ >> struct ethtool_pause_stats { >> * @get_ethtool_phy_stats: Return extended statistics about the PHY >> device. >> * This is only useful if the device maintains PHY statistics and >> * cannot use the standard PHY library helpers. >> + * @get_module_eeprom_data_by_page: Get a region of plug-in module >> EEPROM data >> + * from specified page. Returns a negative error code or zero. >> * >> * All operations are optional (i.e. the function pointer may be set >> * to %NULL) and callers must take this into account. Callers must @@ > -515,6 >> +518,9 @@ struct ethtool_ops { >> const struct ethtool_tunable *, void *); >> int (*set_phy_tunable)(struct net_device *, >> const struct ethtool_tunable *, const > void >> *); >> + int (*get_module_eeprom_data_by_page)(struct net_device >> *dev, >> + const struct >> ethtool_eeprom_data *page, >> + struct netlink_ext_ack >> *extack); >> }; >> >> int ethtool_check_ops(const struct ethtool_ops *ops); @@ -538,7 +544,6 >> @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev, >> const struct ethtool_link_ksettings > *cmd, >> u32 *dev_speed, u8 *dev_duplex); >> >> -struct netlink_ext_ack; >> struct phy_device; >> struct phy_tdr_config; >> >> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h > index >> cde753bb2093..2459571fc1d1 100644 >> --- a/include/uapi/linux/ethtool.h >> +++ b/include/uapi/linux/ethtool.h >> @@ -340,6 +340,28 @@ struct ethtool_eeprom { >> __u8 data[0]; >> }; >> >> +/** >> + * struct ethtool_eeprom_data - EEPROM dump from specified page >> + * @offset: Offset within the specified EEPROM page to begin read, in >> bytes. > Note here that bytes at offset 0-127 are the same for every page of the > module, only bytes at offset 128 and higher are actually paged. Right, but still having the offset relative to 128 will be confusing. >> + * @length: Number of bytes to read. >> + * @page: Page number to read from. >> + * @bank: Page bank number to read from, if applicable by EEPROM spec. >> + * @i2c_address: I2C address of a page. Value less than 0x7f expected. >> Most >> + * EEPROMs use 0x50 or 0x51. > The standards are all very clear, the only legal values are 0x50 and 0x51. > It isn't 'expected', it is required. I suggest that 0xA0 and 0xA2 also be > silently accepted, and translated to 0x50 and 0x51 respectively. Some > of the specs use A0/A2 instead of 0x50/0x51. They actually mean the > same thing. > >> + * @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_eeprom_data { >> + __u32 offset; >> + __u32 length; >> + __u32 page; >> + __u32 bank; >> + __u32 i2c_address; >> + __u8 *data; >> +}; >> + >> /** >> * struct ethtool_eee - Energy Efficient Ethernet information >> * @cmd: ETHTOOL_{G,S}EEE >> @@ -1865,6 +1887,10 @@ static inline int ethtool_validate_duplex(__u8 >> duplex) >> #define ETH_MODULE_SFF_8636_MAX_LEN 640 >> #define ETH_MODULE_SFF_8436_MAX_LEN 640 >> >> +#define ETH_MODULE_EEPROM_MAX_LEN 640 > Please don't add this MAX_LEN constant. Even better, remove > the two above it as well. These constants are relevant to the ioctl function and used by the drivers. For netlink new KAPI I will remove the new MAX_LEN per your explanation, thanks. > The proper value for all 3 of these MAX_LEN items is the > architectural limit imposed by the 8 bit page register plus the constant > lower page (hence 257*128 bytes). The 8436 and 8636 > specs do not actually limit these devices to 640 bytes (3 pages). > > There is no MAX_LEN listed for SFF_8472. If there is one, it should > actually be 259 * 128 bytes (to account for 256 more bytes on the > unpaged 0x50 i2c address). > > Nor is there one for CMIS. The maximum > architected length for CMIS is (257*128) + (127 * 16 * 128). That's > the QSFP max length plus 127 more banks of 16 pages. > >> +#define ETH_MODULE_EEPROM_PAGE_LEN 256 >> +#define ETH_MODULE_MAX_I2C_ADDRESS 0x7f > Actually there are only two legal values for the i2c address (0x50, 0x51). > Rather than defining a MAX address, consider defining the legal values, > or... is it used at all? Leave it out? As Andrew commented there might be usage of other i2c adresses, I will keep it. >> + >> /* Reset flags */ >> /* The reset() operation must clear the flags for the components which >> * were actually reset. On successful return, the flags indicate the > diff --git >> a/include/uapi/linux/ethtool_netlink.h >> b/include/uapi/linux/ethtool_netlink.h >> index a286635ac9b8..60dd848d0b54 100644 >> --- a/include/uapi/linux/ethtool_netlink.h >> +++ b/include/uapi/linux/ethtool_netlink.h >> @@ -42,6 +42,7 @@ enum { >> ETHTOOL_MSG_CABLE_TEST_ACT, >> ETHTOOL_MSG_CABLE_TEST_TDR_ACT, >> ETHTOOL_MSG_TUNNEL_INFO_GET, >> + ETHTOOL_MSG_EEPROM_DATA_GET, >> >> /* add new constants above here */ >> __ETHTOOL_MSG_USER_CNT, >> @@ -80,6 +81,7 @@ enum { >> ETHTOOL_MSG_CABLE_TEST_NTF, >> ETHTOOL_MSG_CABLE_TEST_TDR_NTF, >> ETHTOOL_MSG_TUNNEL_INFO_GET_REPLY, >> + ETHTOOL_MSG_EEPROM_DATA_GET_REPLY, >> >> /* add new constants above here */ >> __ETHTOOL_MSG_KERNEL_CNT, >> @@ -629,6 +631,23 @@ enum { >> ETHTOOL_A_TUNNEL_INFO_MAX = >> (__ETHTOOL_A_TUNNEL_INFO_CNT - 1) }; >> >> +/* MODULE EEPROM DATA */ >> + >> +enum { >> + ETHTOOL_A_EEPROM_DATA_UNSPEC, >> + ETHTOOL_A_EEPROM_DATA_HEADER, >> + >> + ETHTOOL_A_EEPROM_DATA_OFFSET, >> + ETHTOOL_A_EEPROM_DATA_LENGTH, >> + ETHTOOL_A_EEPROM_DATA_PAGE, >> + ETHTOOL_A_EEPROM_DATA_BANK, >> + ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS, >> + ETHTOOL_A_EEPROM_DATA, >> + >> + __ETHTOOL_A_EEPROM_DATA_CNT, >> + ETHTOOL_A_EEPROM_DATA_MAX = >> (__ETHTOOL_A_EEPROM_DATA_CNT - 1) }; >> + >> /* generic netlink info */ >> #define ETHTOOL_GENL_NAME "ethtool" >> #define ETHTOOL_GENL_VERSION 1 >> diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile index >> 7a849ff22dad..d604346bc074 100644 >> --- a/net/ethtool/Makefile >> +++ b/net/ethtool/Makefile >> @@ -7,4 +7,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o >> ethtool_nl-y := netlink.o bitset.o strset.o linkinfo.o linkmodes.o \ >> linkstate.o debug.o wol.o features.o privflags.o rings.o > \ >> channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o > \ >> - tunnels.o >> + tunnels.o eeprom.o >> diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c new file mode >> 100644 index 000000000000..2618a55b9a40 >> --- /dev/null >> +++ b/net/ethtool/eeprom.c >> @@ -0,0 +1,157 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> + >> +#include <linux/ethtool.h> >> +#include "netlink.h" >> +#include "common.h" >> + >> +struct eeprom_data_req_info { >> + struct ethnl_req_info base; >> + u32 offset; >> + u32 length; >> + u32 page; >> + u32 bank; >> + u32 i2c_address; >> +}; >> + >> +struct eeprom_data_reply_data { >> + struct ethnl_reply_data base; >> + u32 length; >> + u32 i2c_address; >> + u8 *data; >> +}; >> + >> +#define EEPROM_DATA_REQINFO(__req_base) \ >> + container_of(__req_base, struct eeprom_data_req_info, base) >> + >> +#define EEPROM_DATA_REPDATA(__reply_base) \ >> + container_of(__reply_base, struct eeprom_data_reply_data, base) >> + >> +static int eeprom_data_prepare_data(const struct ethnl_req_info >> *req_base, >> + struct ethnl_reply_data *reply_base, >> + struct genl_info *info) >> +{ >> + struct eeprom_data_reply_data *reply = >> EEPROM_DATA_REPDATA(reply_base); >> + struct eeprom_data_req_info *request = >> EEPROM_DATA_REQINFO(req_base); >> + struct ethtool_eeprom_data page_data = {0}; >> + struct net_device *dev = reply_base->dev; >> + int err; >> + >> + if (!dev->ethtool_ops->get_module_eeprom_data_by_page) >> + return -EOPNOTSUPP; >> + >> + page_data.offset = request->offset; >> + page_data.length = request->length; >> + page_data.i2c_address = request->i2c_address; >> + page_data.page = request->page; >> + page_data.bank = request->bank; >> + page_data.data = kmalloc(page_data.length, GFP_KERNEL); >> + if (!page_data.data) >> + return -ENOMEM; >> + err = ethnl_ops_begin(dev); >> + if (err) >> + goto err_free; >> + >> + err = dev->ethtool_ops->get_module_eeprom_data_by_page(dev, >> &page_data, >> + > info->extack); >> + if (err) >> + goto err_ops; >> + >> + reply->length = page_data.length; >> + reply->i2c_address = page_data.i2c_address; >> + reply->data = page_data.data; >> + >> + ethnl_ops_complete(dev); > The two error paths below kfree(page_data.data). Does someone else > free this memory when there is no error? eeprom_data_cleanup_data() >> + return 0; >> + >> +err_ops: >> + ethnl_ops_complete(dev); >> +err_free: >> + kfree(page_data.data); >> + return err; >> +} >> + >> +static int eeprom_data_parse_request(struct ethnl_req_info *req_info, >> struct nlattr **tb, >> + struct netlink_ext_ack *extack) { >> + struct eeprom_data_req_info *request = >> EEPROM_DATA_REQINFO(req_info); >> + >> + if (!tb[ETHTOOL_A_EEPROM_DATA_OFFSET] || >> + !tb[ETHTOOL_A_EEPROM_DATA_LENGTH] || >> + !tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]) >> + return -EINVAL; >> + >> + request->i2c_address = >> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]); >> + if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS) >> + return -EINVAL; > I would be much more restrictive, with one flexibility... > > if (request->i2c_address == 0xA0) request->i2c_address = 0x50; > if (request->i2c_address == 0xA2) request->i2c_address = 0x51; > if (request->i2c_address < 0x50) || (request->i2c_address > 0x51) > return -EINVAL; > >> + >> + request->offset = >> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]); >> + request->length = >> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]); >> + if (request->length > ETH_MODULE_EEPROM_MAX_LEN) >> + return -EINVAL; > This is really problematic as there are MANY different max values, within > the specs, for the various EEPROMs being generically supported here. > I would leave it to the drivers to handle out-of-range requests. If you > really want to check, you need to know which spec the module supports, > whether it supports pages, and whether it supports banks. I have not > found a register that actually reports the number of supported pages > that an eeprom supports. The specs should have included that :-(. Will remove this check per your explanation, thanks. >> + if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] && >> + request->offset + request->length > >> ETH_MODULE_EEPROM_PAGE_LEN) >> + return -EINVAL; > Why does this stanza depend on DATA_PAGE? In this new data > structure, no requests can cross the 256 byte page boundary. > > I suggest, rather then -EINVAL, you should reduce the length to reach > the end of the page: > > if (request->offset + request->length) > ETH_MODULE_EEPROM_PAGE_LEN) > request->length = ETH_MODULE_EEPROM_PAGE_LEN - > request->offset. Backward compatibility, users use the command without page and bank as it was till now expect to get data that can cross page as it worked till now, I can't break it. > Note that this matches the choice you made to truncate rather than > error out in fallback_set_parms(). We truncate in fallback_set_params() to eeprom_len. length = modinfo->eeprom_len - offset; >> + >> + if (tb[ETHTOOL_A_EEPROM_DATA_PAGE]) >> + request->page = >> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_PAGE]); >> + if (tb[ETHTOOL_A_EEPROM_DATA_BANK]) >> + request->bank = >> nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_BANK]); > Other checks: > > Page and bank have to be between 0 and 255 (inclusive), they > go into an 8 bit register in the eeprom. Will add, thanks. > Offset and length can't be negative. Already are u32. >> + >> + return 0; >> +} >> + >> +static int eeprom_data_reply_size(const struct ethnl_req_info *req_base, >> + const struct ethnl_reply_data *reply_base) > { >> + const struct eeprom_data_req_info *request = >> +EEPROM_DATA_REQINFO(req_base); >> + >> + return nla_total_size(sizeof(u32)) + /* _EEPROM_DATA_LENGTH */ >> + nla_total_size(sizeof(u32)) + /* _EEPROM_DATA_I2C_ADDRESS >> */ >> + nla_total_size(sizeof(u8) * request->length); /* _EEPROM_DATA >> +*/ } >> + >> +static int eeprom_data_fill_reply(struct sk_buff *skb, >> + const struct ethnl_req_info *req_base, >> + const struct ethnl_reply_data *reply_base) > { >> + struct eeprom_data_reply_data *reply = >> +EEPROM_DATA_REPDATA(reply_base); >> + >> + if (nla_put_u32(skb, ETHTOOL_A_EEPROM_DATA_LENGTH, reply- >>> length) || >> + nla_put_u32(skb, ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS, >> reply->i2c_address) || >> + nla_put(skb, ETHTOOL_A_EEPROM_DATA, reply->length, reply- >>> data)) >> + return -EMSGSIZE; >> + >> + return 0; >> +} >> + >> +static void eeprom_data_cleanup_data(struct ethnl_reply_data >> +*reply_base) { >> + struct eeprom_data_reply_data *reply = >> +EEPROM_DATA_REPDATA(reply_base); >> + >> + kfree(reply->data); >> +} >> + >> +const struct ethnl_request_ops ethnl_eeprom_data_request_ops = { >> + .request_cmd = ETHTOOL_MSG_EEPROM_DATA_GET, >> + .reply_cmd = >> ETHTOOL_MSG_EEPROM_DATA_GET_REPLY, >> + .hdr_attr = ETHTOOL_A_EEPROM_DATA_HEADER, >> + .req_info_size = sizeof(struct eeprom_data_req_info), >> + .reply_data_size = sizeof(struct eeprom_data_reply_data), >> + >> + .parse_request = eeprom_data_parse_request, >> + .prepare_data = eeprom_data_prepare_data, >> + .reply_size = eeprom_data_reply_size, >> + .fill_reply = eeprom_data_fill_reply, >> + .cleanup_data = eeprom_data_cleanup_data, >> +}; >> + >> +const struct nla_policy ethnl_eeprom_data_get_policy[] = { >> + [ETHTOOL_A_EEPROM_DATA_HEADER] = >> NLA_POLICY_NESTED(ethnl_header_policy), >> + [ETHTOOL_A_EEPROM_DATA_OFFSET] = { .type = >> NLA_U32 }, >> + [ETHTOOL_A_EEPROM_DATA_LENGTH] = { .type = >> NLA_U32 }, >> + [ETHTOOL_A_EEPROM_DATA_PAGE] = { .type = NLA_U32 }, >> + [ETHTOOL_A_EEPROM_DATA_BANK] = { .type = NLA_U32 }, >> + [ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS] = { .type = NLA_U32 }, >> + [ETHTOOL_A_EEPROM_DATA] = { .type = >> NLA_BINARY }, >> +}; >> + >> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index >> 50d3c8896f91..ff2528bee192 100644 >> --- a/net/ethtool/netlink.c >> +++ b/net/ethtool/netlink.c >> @@ -245,6 +245,7 @@ >> ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = { >> [ETHTOOL_MSG_PAUSE_GET] = >> ðnl_pause_request_ops, >> [ETHTOOL_MSG_EEE_GET] = ðnl_eee_request_ops, >> [ETHTOOL_MSG_TSINFO_GET] = ðnl_tsinfo_request_ops, >> + [ETHTOOL_MSG_EEPROM_DATA_GET] = >> ðnl_eeprom_data_request_ops, >> }; >> >> static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback >> *cb) @@ -912,6 +913,15 @@ static const struct genl_ops ethtool_genl_ops[] >> = { >> .policy = ethnl_tunnel_info_get_policy, >> .maxattr = ARRAY_SIZE(ethnl_tunnel_info_get_policy) - 1, >> }, >> + { >> + .cmd = ETHTOOL_MSG_EEPROM_DATA_GET, >> + .doit = ethnl_default_doit, >> + .start = ethnl_default_start, >> + .dumpit = ethnl_default_dumpit, >> + .done = ethnl_default_done, >> + .policy = ethnl_eeprom_data_get_policy, >> + .maxattr = ARRAY_SIZE(ethnl_eeprom_data_get_policy) - 1, >> + }, >> }; >> >> static const struct genl_multicast_group ethtool_nl_mcgrps[] = { diff > --git >> a/net/ethtool/netlink.h b/net/ethtool/netlink.h index >> 6eabd58d81bf..60954c7b4dfe 100644 >> --- a/net/ethtool/netlink.h >> +++ b/net/ethtool/netlink.h >> @@ -344,6 +344,7 @@ extern const struct ethnl_request_ops >> ethnl_coalesce_request_ops; extern const struct ethnl_request_ops >> ethnl_pause_request_ops; extern const struct ethnl_request_ops >> ethnl_eee_request_ops; extern const struct ethnl_request_ops >> ethnl_tsinfo_request_ops; >> +extern const struct ethnl_request_ops ethnl_eeprom_data_request_ops; >> >> extern const struct nla_policy >> ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1]; extern const struct >> nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1]; @@ >> -375,6 +376,7 @@ extern const struct nla_policy >> ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_HEADER + extern const struct >> nla_policy ethnl_cable_test_act_policy[ETHTOOL_A_CABLE_TEST_HEADER + >> 1]; extern const struct nla_policy >> ethnl_cable_test_tdr_act_policy[ETHTOOL_A_CABLE_TEST_TDR_CFG + 1]; >> extern const struct nla_policy >> ethnl_tunnel_info_get_policy[ETHTOOL_A_TUNNEL_INFO_HEADER + 1]; >> +extern const struct nla_policy >> +ethnl_eeprom_data_get_policy[ETHTOOL_A_EEPROM_DATA + 1]; >> >> int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info); int >> ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info); >> -- >> 2.18.2
On 3/5/2021 3:58 AM, Andrew Lunn wrote: > >> +/* MODULE EEPROM DATA */ >> + >> +enum { >> + ETHTOOL_A_EEPROM_DATA_UNSPEC, >> + ETHTOOL_A_EEPROM_DATA_HEADER, >> + >> + ETHTOOL_A_EEPROM_DATA_OFFSET, >> + ETHTOOL_A_EEPROM_DATA_LENGTH, >> + ETHTOOL_A_EEPROM_DATA_PAGE, >> + ETHTOOL_A_EEPROM_DATA_BANK, >> + ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS, >> + ETHTOOL_A_EEPROM_DATA, > If you look at all the other such enums in ethtool_netlink, you will > see a comment indicating the type. Please add them here as well. > > Please also update Documentation/networking/ethtool-netlink.rst. Sure, will add. > Andrew