On Mon, 15 Mar 2021 19:12:39 +0200 Moshe Shemesh wrote:
From: Vladyslav Tarasiuk <redacted>
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 <redacted>
s/eeprom_data/eeprom/ everywhere?
Otherwise some of your identifiers are over 30 characters long.
quoted hunk ↗ jump to hunk
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?
+#define ETH_MODULE_EEPROM_PAGE_LEN 256
+#define ETH_MODULE_MAX_I2C_ADDRESS 0x7f
Not sure about these defines either.
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ 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?
Should we echo back offset in case driver wants to report re-aligned
data?
quoted hunk ↗ jump to hunk
+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?