On 3/15/2021 11:01 PM, Jakub Kicinski wrote:
On Mon, 15 Mar 2021 19:12:39 +0200 Moshe Shemesh wrote:
From: Vladyslav Tarasiuk <vladysl...@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 <vladysl...@nvidia.com>
s/eeprom_data/eeprom/ everywhere?
Otherwise some of your identifiers are over 30 characters long.
That can work part to one struct on the uapi file that can conflict with
previous old KAPI struct, but we can figure it too.
Will fix.
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index cde753bb2093..b3e92db3ad37 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.
+ * @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.
+ * @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;
+ __u8 page;
+ __u8 bank;
+ __u8 i2c_address;
+ __u8 *data;
+};
+
This structure does not have to be part of uAPI, right?
Right, will fix.
+#define ETH_MODULE_EEPROM_PAGE_LEN 256
+#define ETH_MODULE_MAX_I2C_ADDRESS 0x7f
Not sure about these defines either.
Probably.
+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 ret;
+
+ 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;
nit: new line?
Ack.
+ ret = ethnl_ops_begin(dev);
+ if (ret)
+ goto err_free;
+
+ ret = dev->ethtool_ops->get_module_eeprom_data_by_page(dev, &page_data,
+ info->extack);
+ if (ret < 0)
+ goto err_ops;
+
+ reply->length = ret;
+ reply->data = page_data.data;
+
+ ethnl_ops_complete(dev);
+ return 0;
+
+err_ops:
+ ethnl_ops_complete(dev);
+err_free:
+ kfree(page_data.data);
+ return ret;
+}
+
+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);
+ struct net_device *dev = req_info->dev;
+
+ 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_u8(tb[ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS]);
+ if (request->i2c_address > ETH_MODULE_MAX_I2C_ADDRESS)
+ return -EINVAL;
Max value should be set in the netlink policy.
Ack
+ request->offset = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
+ request->length = nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
+ if (tb[ETHTOOL_A_EEPROM_DATA_PAGE] &&
+ dev->ethtool_ops->get_module_eeprom_data_by_page &&
Why check dev->ethtool_ops->get_module_eeprom_data_by_page here?
Right, actually not needed here, thanks.
+ request->offset + request->length > ETH_MODULE_EEPROM_PAGE_LEN)
+ return -EINVAL;
+
+ if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
+ request->page = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
+ if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
+ request->bank = nla_get_u8(tb[ETHTOOL_A_EEPROM_DATA_BANK]);
+
+ 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(u8) * request->length); /* _EEPROM_DATA */
+}
Why report length? Is the netlink length of the DATA attribute not
sufficient?
Right, thanks.
Should we echo back offset in case driver wants to report re-aligned
data?
The driver knows the requested offset and data has to be from that offset.
+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_U8 },
+ [ETHTOOL_A_EEPROM_DATA_BANK] = { .type = NLA_U8 },
+ [ETHTOOL_A_EEPROM_DATA_I2C_ADDRESS] = { .type = NLA_U8 },
+ [ETHTOOL_A_EEPROM_DATA] = { .type = NLA_BINARY },
This is a policy for inputs, DATA should not be allowed on input, right?
Right, thanks.
