[PATCH net-next v2 0/9] support direct read from region

[PATCH net-next v2 0/9] support direct read from region

[Jacob Keller]

Changes since v1:
* Re-ordered patches at the beginning slightly, pulling min_t change and
  reporting of extended error messages to the start of the series.
* use NL_SET_ERR_MSG_ATTR for reporting invalid attributes
* Use kmalloc instead of kzalloc
* Cleanup spacing around data_size
* Fix the __always_unused positioning
* Update documentation for direct reads to clearly explain they are not
  atomic for larger reads.
* Add a patch to fix missing documentation for ice.rst
* Mention the direct read support in ice.rst documentation

A long time ago when initially implementing devlink regions in ice I
proposed the ability to allow reading from a region without taking a
snapshot [1]. I eventually dropped this work from the original series due to
size. Then I eventually lost track of submitting this follow up.

This can be useful when interacting with some region that has some
definitive "contents" from which snapshots are made. For example the ice
driver has regions representing the contents of the device flash.

If userspace wants to read the contents today, it must first take a snapshot
and then read from that snapshot. This makes sense if you want to read a
large portion of data or you want to be sure reads are consistently from the
same recording of the flash.

However if user space only wants to read a small chunk, it must first
generate a snapshot of the entire contents, perform a read from the
snapshot, and then delete the snapshot after reading.

For such a use case, a direct read from the region makes more sense. This
can be achieved by allowing the devlink region read command to work without
a snapshot. Instead the portion to be read can be forwarded directly to the
driver via a new .read callback.

This avoids the need to read the entire region contents into memory first
and avoids the software overhead of creating a snapshot and then deleting
it.

This series implements such behavior and hooks up the ice NVM and shadow RAM
regions to allow it.

[1] https://lore.kernel.org/netdev/20200130225913.1671982-1-jacob.e.keller@intel.com/

Cc: Jiri Pirko <jiri@nvidia.com>
Cc: Jakub Kicinski <kuba@kernel.org>


Jacob Keller (9):
  devlink: use min_t to calculate data_size
  devlink: report extended error message in region_read_dumpit
  devlink: find snapshot in devlink_nl_cmd_region_read_dumpit
  devlink: remove unnecessary parameter from chunk_fill function
  devlink: refactor region_read_snapshot_fill to use a callback function
  devlink: support directly reading from region memory
  ice: use same function to snapshot both NVM and Shadow RAM
  ice: document 'shadow-ram' devlink region
  ice: implement direct read for NVM and Shadow RAM regions

 .../networking/devlink/devlink-region.rst     |  11 ++
 Documentation/networking/devlink/ice.rst      |  13 +-
 drivers/net/ethernet/intel/ice/ice_devlink.c  | 112 +++++++++-------
 include/net/devlink.h                         |  16 +++
 net/core/devlink.c                            | 125 +++++++++++++-----
 5 files changed, 197 insertions(+), 80 deletions(-)


base-commit: 339e79dfb087075cbc27d3a902457574c4dac182
-- 
2.38.1.420.g319605f8f00e

[PATCH net-next v2 1/9] devlink: use min_t to calculate data_size

[Jacob Keller]

The calculation for the data_size in the devlink_nl_read_snapshot_fill
function uses an if statement that is better expressed using the min_t
macro.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since v1:
* Moved to 1/9 of the series
* No longer combined declaration of data_size with its assignment

 net/core/devlink.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index d93bc95cd7cb..a490b8850179 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6485,10 +6485,8 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
 		u32 data_size;
 		u8 *data;
 
-		if (end_offset - curr_offset < DEVLINK_REGION_READ_CHUNK_SIZE)
-			data_size = end_offset - curr_offset;
-		else
-			data_size = DEVLINK_REGION_READ_CHUNK_SIZE;
+		data_size = min_t(u32, end_offset - curr_offset,
+				  DEVLINK_REGION_READ_CHUNK_SIZE);
 
 		data = &snapshot->data[curr_offset];
 		err = devlink_nl_cmd_region_read_chunk_fill(skb, devlink,
-- 
2.38.1.420.g319605f8f00e

[PATCH net-next v2 2/9] devlink: report extended error message in region_read_dumpit

[Jacob Keller]

Report extended error details in the devlink_nl_cmd_region_read_dumpit
function, by using the extack structure from the netlink_callback.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since v1:
* Moved to 2/9 of the series
* No longer includes change to snapshot, as that is now handled in 3/9
* use local region_attr variable and NL_SET_ERR_MSG_ATTR

 net/core/devlink.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index a490b8850179..b5b317661f9a 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6507,10 +6507,10 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 {
 	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
 	u64 ret_offset, start_offset, end_offset = U64_MAX;
+	struct nlattr *chunks_attr, *region_attr;
 	struct nlattr **attrs = info->attrs;
 	struct devlink_port *port = NULL;
 	struct devlink_region *region;
-	struct nlattr *chunks_attr;
 	const char *region_name;
 	struct devlink *devlink;
 	unsigned int index;
@@ -6525,8 +6525,14 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 
 	devl_lock(devlink);
 
-	if (!attrs[DEVLINK_ATTR_REGION_NAME] ||
-	    !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
+	if (!attrs[DEVLINK_ATTR_REGION_NAME]) {
+		NL_SET_ERR_MSG(cb->extack, "No region name provided");
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
+	if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
+		NL_SET_ERR_MSG(cb->extack, "No snapshot id provided");
 		err = -EINVAL;
 		goto out_unlock;
 	}
@@ -6541,7 +6547,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		}
 	}
 
-	region_name = nla_data(attrs[DEVLINK_ATTR_REGION_NAME]);
+	region_attr = attrs[DEVLINK_ATTR_REGION_NAME];
+	region_name = nla_data(region_attr);
 
 	if (port)
 		region = devlink_port_region_get_by_name(port, region_name);
@@ -6549,6 +6556,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		region = devlink_region_get_by_name(devlink, region_name);
 
 	if (!region) {
+		NL_SET_ERR_MSG_ATTR(cb->extack, region_attr, "requested region does not exist");
 		err = -EINVAL;
 		goto out_unlock;
 	}
-- 
2.38.1.420.g319605f8f00e

[PATCH net-next v2 3/9] devlink: find snapshot in devlink_nl_cmd_region_read_dumpit

[Jacob Keller]

The snapshot pointer is obtained inside of the function
devlink_nl_region_read_snapshot_fill. Simplify this function by locating
the snapshot upfront in devlink_nl_cmd_region_read_dumpit instead. This
aligns with how other netlink attributes are handled, and allows us to
exit slightly earlier if an invalid snapshot ID is provided.

It also allows us to pass the snapshot pointer directly to the
devlink_nl_region_read_snapshot_fill, and remove the now unused attrs
parameter.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since v1
* Moved to 3/9 of series
* Use snapshot_attr and NL_SET_ERR_MSG_ATTR to report extended error

 net/core/devlink.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index b5b317661f9a..825c52a71df1 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6463,24 +6463,16 @@ static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
 
 static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
 						struct devlink *devlink,
-						struct devlink_region *region,
-						struct nlattr **attrs,
+						struct devlink_snapshot *snapshot,
 						u64 start_offset,
 						u64 end_offset,
 						u64 *new_offset)
 {
-	struct devlink_snapshot *snapshot;
 	u64 curr_offset = start_offset;
-	u32 snapshot_id;
 	int err = 0;
 
 	*new_offset = start_offset;
 
-	snapshot_id = nla_get_u32(attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]);
-	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
-	if (!snapshot)
-		return -EINVAL;
-
 	while (curr_offset < end_offset) {
 		u32 data_size;
 		u8 *data;
@@ -6506,14 +6498,16 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 					     struct netlink_callback *cb)
 {
 	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct nlattr *chunks_attr, *region_attr, *snapshot_attr;
 	u64 ret_offset, start_offset, end_offset = U64_MAX;
-	struct nlattr *chunks_attr, *region_attr;
 	struct nlattr **attrs = info->attrs;
 	struct devlink_port *port = NULL;
+	struct devlink_snapshot *snapshot;
 	struct devlink_region *region;
 	const char *region_name;
 	struct devlink *devlink;
 	unsigned int index;
+	u32 snapshot_id;
 	void *hdr;
 	int err;
 
@@ -6561,6 +6555,15 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		goto out_unlock;
 	}
 
+	snapshot_attr = attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];
+	snapshot_id = nla_get_u32(snapshot_attr);
+	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
+	if (!snapshot) {
+		NL_SET_ERR_MSG_ATTR(cb->extack, snapshot_attr, "requested snapshot does not exist");
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
 	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
 	    attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]) {
 		if (!start_offset)
@@ -6610,7 +6613,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 	}
 
 	err = devlink_nl_region_read_snapshot_fill(skb, devlink,
-						   region, attrs,
+						   snapshot,
 						   start_offset,
 						   end_offset, &ret_offset);
 
-- 
2.38.1.420.g319605f8f00e

[PATCH net-next v2 4/9] devlink: remove unnecessary parameter from chunk_fill function

[Jacob Keller]

The devlink parameter of the devlink_nl_cmd_region_read_chunk_fill
function is not used. Remove it, to simplify the function signature.

Once removed, it is also obvious that the devlink parameter is not
necessary for the devlink_nl_region_read_snapshot_fill either.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
No changes since v1.

 net/core/devlink.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 825c52a71df1..bd7af0600405 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6431,7 +6431,6 @@ devlink_nl_cmd_region_new(struct sk_buff *skb, struct genl_info *info)
 }
 
 static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
-						 struct devlink *devlink,
 						 u8 *chunk, u32 chunk_size,
 						 u64 addr)
 {
@@ -6462,7 +6461,6 @@ static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
 #define DEVLINK_REGION_READ_CHUNK_SIZE 256
 
 static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
-						struct devlink *devlink,
 						struct devlink_snapshot *snapshot,
 						u64 start_offset,
 						u64 end_offset,
@@ -6481,9 +6479,7 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
 				  DEVLINK_REGION_READ_CHUNK_SIZE);
 
 		data = &snapshot->data[curr_offset];
-		err = devlink_nl_cmd_region_read_chunk_fill(skb, devlink,
-							    data, data_size,
-							    curr_offset);
+		err = devlink_nl_cmd_region_read_chunk_fill(skb, data, data_size, curr_offset);
 		if (err)
 			break;
 
@@ -6612,9 +6608,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		goto nla_put_failure;
 	}
 
-	err = devlink_nl_region_read_snapshot_fill(skb, devlink,
-						   snapshot,
-						   start_offset,
+	err = devlink_nl_region_read_snapshot_fill(skb, snapshot, start_offset,
 						   end_offset, &ret_offset);
 
 	if (err && err != -EMSGSIZE)
-- 
2.38.1.420.g319605f8f00e

[PATCH net-next v2 5/9] devlink: refactor region_read_snapshot_fill to use a callback function

[Jacob Keller]

The devlink_nl_region_read_snapshot_fill is used to copy the contents of
a snapshot into a message for reporting to userspace via the
DEVLINK_CMG_REGION_READ netlink message.

A future change is going to add support for directly reading from
a region. Almost all of the logic for this new capability is identical.

To help reduce code duplication and make this logic more generic,
refactor the function to take a cb and cb_priv pointer for doing the
actual copy.

Add a devlink_region_snapshot_fill implementation that will simply copy
the relevant chunk of the region. This does require allocating some
storage for the chunk as opposed to simply passing the correct address
forward to the devlink_nl_cmg_region_read_chunk_fill function.

A future change to implement support for directly reading from a region
without a snapshot will provide a separate implementation that calls the
newly added devlink region operation.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since v1:
* Use kmalloc instead of kzalloc
* Don't combine data_size declaration and assignment
* Fix the always_unused placement for devlink_region_snapshot_fill

 net/core/devlink.c | 44 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index bd7af0600405..729e2162a4db 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6460,25 +6460,36 @@ static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
 
 #define DEVLINK_REGION_READ_CHUNK_SIZE 256
 
-static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
-						struct devlink_snapshot *snapshot,
-						u64 start_offset,
-						u64 end_offset,
-						u64 *new_offset)
+typedef int devlink_chunk_fill_t(void *cb_priv, u8 *chunk, u32 chunk_size,
+				 u64 curr_offset,
+				 struct netlink_ext_ack *extack);
+
+static int
+devlink_nl_region_read_fill(struct sk_buff *skb, devlink_chunk_fill_t *cb,
+			    void *cb_priv, u64 start_offset, u64 end_offset,
+			    u64 *new_offset, struct netlink_ext_ack *extack)
 {
 	u64 curr_offset = start_offset;
 	int err = 0;
+	u8 *data;
+
+	/* Allocate and re-use a single buffer */
+	data = kmalloc(DEVLINK_REGION_READ_CHUNK_SIZE, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
 
 	*new_offset = start_offset;
 
 	while (curr_offset < end_offset) {
 		u32 data_size;
-		u8 *data;
 
 		data_size = min_t(u32, end_offset - curr_offset,
 				  DEVLINK_REGION_READ_CHUNK_SIZE);
 
-		data = &snapshot->data[curr_offset];
+		err = cb(cb_priv, data, data_size, curr_offset, extack);
+		if (err)
+			break;
+
 		err = devlink_nl_cmd_region_read_chunk_fill(skb, data, data_size, curr_offset);
 		if (err)
 			break;
@@ -6487,9 +6498,23 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
 	}
 	*new_offset = curr_offset;
 
+	kfree(data);
+
 	return err;
 }
 
+static int
+devlink_region_snapshot_fill(void *cb_priv, u8 *chunk, u32 chunk_size,
+			     u64 curr_offset,
+			     struct netlink_ext_ack __always_unused *extack)
+{
+	struct devlink_snapshot *snapshot = cb_priv;
+
+	memcpy(chunk, &snapshot->data[curr_offset], chunk_size);
+
+	return 0;
+}
+
 static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 					     struct netlink_callback *cb)
 {
@@ -6608,8 +6633,9 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		goto nla_put_failure;
 	}
 
-	err = devlink_nl_region_read_snapshot_fill(skb, snapshot, start_offset,
-						   end_offset, &ret_offset);
+	err = devlink_nl_region_read_fill(skb, &devlink_region_snapshot_fill,
+					  snapshot, start_offset, end_offset,
+					  &ret_offset, cb->extack);
 
 	if (err && err != -EMSGSIZE)
 		goto nla_put_failure;
-- 
2.38.1.420.g319605f8f00e

[PATCH net-next v2 6/9] devlink: support directly reading from region memory

[Jacob Keller]

To read from a region, user space must currently request a new snapshot of
the region and then read from that snapshot. This can sometimes be overkill
if user space only reads a tiny portion. They first create the snapshot,
then request a read, then destroy the snapshot.

For regions which have a single underlying "contents", it makes sense to
allow supporting direct reading of the region data.

Extend the DEVLINK_CMD_REGION_READ to allow direct reading from a region if
supported. Instead of reporting a missing snapshot id as invalid, check if
the region supports direct read and if so switch to the direct access
method for reading the region data.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since v1:
* Update documentation to explain that direct reads are not atomic

 .../networking/devlink/devlink-region.rst     | 11 ++++
 include/net/devlink.h                         | 16 +++++
 net/core/devlink.c                            | 66 ++++++++++++++-----
 3 files changed, 76 insertions(+), 17 deletions(-)

diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
index f06dca9a1eb6..6525a9120a4e 100644
--- a/Documentation/networking/devlink/devlink-region.rst
+++ b/Documentation/networking/devlink/devlink-region.rst
@@ -31,6 +31,13 @@ in its ``devlink_region_ops`` structure. If snapshot id is not set in
 the ``DEVLINK_CMD_REGION_NEW`` request kernel will allocate one and send
 the snapshot information to user space.
 
+Regions may optionally allow directly reading from their contents without a
+snapshot. Direct read requests are not atomic. In particular a read request
+of size 256 bytes or larger will be split into multiple chunks. If atomic
+access is required, use a snapshot. A driver wishing to enable this for a
+region should implement the ``.read`` callback in the ``devlink_region_ops``
+structure.
+
 example usage
 -------------
 
@@ -65,6 +72,10 @@ example usage
     $ devlink region read pci/0000:00:05.0/fw-health snapshot 1 address 0 length 16
     0000000000000000 0014 95dc 0014 9514 0035 1670 0034 db30
 
+    # Read from the region without a snapshot
+    $ devlink region read pci/0000:00:05.0/fw-health address 16 length 16
+    0000000000000010 0000 0000 ffff ff04 0029 8c00 0028 8cc8
+
 As regions are likely very device or driver specific, no generic regions are
 defined. See the driver-specific documentation files for information on the
 specific regions a driver supports.
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 074a79b8933f..02528f736f65 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -650,6 +650,10 @@ struct devlink_info_req;
  *            the data variable must be updated to point to the snapshot data.
  *            The function will be called while the devlink instance lock is
  *            held.
+ * @read: callback to directly read a portion of the region. On success,
+ *        the data pointer will be updated with the contents of the
+ *        requested portion of the region. The function will be called
+ *        while the devlink instance lock is held.
  * @priv: Pointer to driver private data for the region operation
  */
 struct devlink_region_ops {
@@ -659,6 +663,10 @@ struct devlink_region_ops {
 			const struct devlink_region_ops *ops,
 			struct netlink_ext_ack *extack,
 			u8 **data);
+	int (*read)(struct devlink *devlink,
+		    const struct devlink_region_ops *ops,
+		    struct netlink_ext_ack *extack,
+		    u64 offset, u32 size, u8 *data);
 	void *priv;
 };
 
@@ -670,6 +678,10 @@ struct devlink_region_ops {
  *            the data variable must be updated to point to the snapshot data.
  *            The function will be called while the devlink instance lock is
  *            held.
+ * @read: callback to directly read a portion of the region. On success,
+ *        the data pointer will be updated with the contents of the
+ *        requested portion of the region. The function will be called
+ *        while the devlink instance lock is held.
  * @priv: Pointer to driver private data for the region operation
  */
 struct devlink_port_region_ops {
@@ -679,6 +691,10 @@ struct devlink_port_region_ops {
 			const struct devlink_port_region_ops *ops,
 			struct netlink_ext_ack *extack,
 			u8 **data);
+	int (*read)(struct devlink_port *port,
+		    const struct devlink_port_region_ops *ops,
+		    struct netlink_ext_ack *extack,
+		    u64 offset, u32 size, u8 *data);
 	void *priv;
 };
 
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 729e2162a4db..f249b5b57677 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6515,6 +6515,26 @@ devlink_region_snapshot_fill(void *cb_priv, u8 *chunk, u32 chunk_size,
 	return 0;
 }
 
+static int
+devlink_region_port_direct_fill(void *cb_priv, u8 *chunk, u32 chunk_size,
+				u64 curr_offset, struct netlink_ext_ack *extack)
+{
+	struct devlink_region *region = cb_priv;
+
+	return region->port_ops->read(region->port, region->port_ops, extack,
+				      curr_offset, chunk_size, chunk);
+}
+
+static int
+devlink_region_direct_fill(void *cb_priv, u8 *chunk, u32 chunk_size,
+			   u64 curr_offset, struct netlink_ext_ack *extack)
+{
+	struct devlink_region *region = cb_priv;
+
+	return region->ops->read(region->devlink, region->ops, extack,
+				 curr_offset, chunk_size, chunk);
+}
+
 static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 					     struct netlink_callback *cb)
 {
@@ -6523,12 +6543,12 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 	u64 ret_offset, start_offset, end_offset = U64_MAX;
 	struct nlattr **attrs = info->attrs;
 	struct devlink_port *port = NULL;
-	struct devlink_snapshot *snapshot;
+	devlink_chunk_fill_t *region_cb;
 	struct devlink_region *region;
 	const char *region_name;
 	struct devlink *devlink;
 	unsigned int index;
-	u32 snapshot_id;
+	void *region_cb_priv;
 	void *hdr;
 	int err;
 
@@ -6546,12 +6566,6 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		goto out_unlock;
 	}
 
-	if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
-		NL_SET_ERR_MSG(cb->extack, "No snapshot id provided");
-		err = -EINVAL;
-		goto out_unlock;
-	}
-
 	if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
 		index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
 
@@ -6577,12 +6591,30 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 	}
 
 	snapshot_attr = attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];
-	snapshot_id = nla_get_u32(snapshot_attr);
-	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
-	if (!snapshot) {
-		NL_SET_ERR_MSG_ATTR(cb->extack, snapshot_attr, "requested snapshot does not exist");
-		err = -EINVAL;
-		goto out_unlock;
+	if (!snapshot_attr) {
+		if (!region->ops->read) {
+			NL_SET_ERR_MSG(cb->extack, "requested region does not support direct read");
+			err = -EOPNOTSUPP;
+			goto out_unlock;
+		}
+		if (port)
+			region_cb = &devlink_region_port_direct_fill;
+		else
+			region_cb = &devlink_region_direct_fill;
+		region_cb_priv = region;
+	} else {
+		struct devlink_snapshot *snapshot;
+		u32 snapshot_id;
+
+		snapshot_id = nla_get_u32(snapshot_attr);
+		snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
+		if (!snapshot) {
+			NL_SET_ERR_MSG_ATTR(cb->extack, snapshot_attr, "requested snapshot does not exist");
+			err = -EINVAL;
+			goto out_unlock;
+		}
+		region_cb = &devlink_region_snapshot_fill;
+		region_cb_priv = snapshot;
 	}
 
 	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
@@ -6633,9 +6665,9 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		goto nla_put_failure;
 	}
 
-	err = devlink_nl_region_read_fill(skb, &devlink_region_snapshot_fill,
-					  snapshot, start_offset, end_offset,
-					  &ret_offset, cb->extack);
+	err = devlink_nl_region_read_fill(skb, region_cb, region_cb_priv,
+					  start_offset, end_offset, &ret_offset,
+					  cb->extack);
 
 	if (err && err != -EMSGSIZE)
 		goto nla_put_failure;
-- 
2.38.1.420.g319605f8f00e

[PATCH net-next v2 7/9] ice: use same function to snapshot both NVM and Shadow RAM

[Jacob Keller]

The ice driver supports a region for both the flat NVM contents as well as
the Shadow RAM which is a layer built on top of the flash during device
initialization.

These regions use an almost identical read function, except that the NVM
needs to set the direct flag when reading, while Shadow RAM needs to read
without the direct flag set. They each call ice_read_flat_nvm with the only
difference being whether to set the direct flash flag.

The NVM region read function also was fixed to read the NVM in blocks to
avoid a situation where the firmware reclaims the lock due to taking too
long.

Note that the region snapshot function takes the ops pointer so the
function can easily determine which region to read. Make use of this and
re-use the NVM snapshot function for both the NVM and Shadow RAM regions.
This makes Shadow RAM benefit from the same block approach as the NVM
region. It also reduces code in the ice driver.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
No changes since v1.

 drivers/net/ethernet/intel/ice/ice_devlink.c | 95 +++++---------------
 1 file changed, 23 insertions(+), 72 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 1d638216484d..9d3500291d2e 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -1596,21 +1596,22 @@ void ice_devlink_destroy_vf_port(struct ice_vf *vf)
 
 #define ICE_DEVLINK_READ_BLK_SIZE (1024 * 1024)
 
+static const struct devlink_region_ops ice_nvm_region_ops;
+static const struct devlink_region_ops ice_sram_region_ops;
+
 /**
  * ice_devlink_nvm_snapshot - Capture a snapshot of the NVM flash contents
  * @devlink: the devlink instance
- * @ops: the devlink region being snapshotted
+ * @ops: the devlink region to snapshot
  * @extack: extended ACK response structure
  * @data: on exit points to snapshot data buffer
  *
- * This function is called in response to the DEVLINK_CMD_REGION_TRIGGER for
- * the nvm-flash devlink region. It captures a snapshot of the full NVM flash
- * contents, including both banks of flash. This snapshot can later be viewed
- * via the devlink-region interface.
+ * This function is called in response to a DEVLINK_CMD_REGION_NEW for either
+ * the nvm-flash or shadow-ram region.
  *
- * It captures the flash using the FLASH_ONLY bit set when reading via
- * firmware, so it does not read the current Shadow RAM contents. For that,
- * use the shadow-ram region.
+ * It captures a snapshot of the NVM or Shadow RAM flash contents. This
+ * snapshot can then later be viewed via the DEVLINK_CMD_REGION_READ netlink
+ * interface.
  *
  * @returns zero on success, and updates the data pointer. Returns a non-zero
  * error code on failure.
@@ -1622,17 +1623,27 @@ static int ice_devlink_nvm_snapshot(struct devlink *devlink,
 	struct ice_pf *pf = devlink_priv(devlink);
 	struct device *dev = ice_pf_to_dev(pf);
 	struct ice_hw *hw = &pf->hw;
+	bool read_shadow_ram;
 	u8 *nvm_data, *tmp, i;
 	u32 nvm_size, left;
 	s8 num_blks;
 	int status;
 
-	nvm_size = hw->flash.flash_size;
+	if (ops == &ice_nvm_region_ops) {
+		read_shadow_ram = false;
+		nvm_size = hw->flash.flash_size;
+	} else if (ops == &ice_sram_region_ops) {
+		read_shadow_ram = true;
+		nvm_size = hw->flash.sr_words * 2u;
+	} else {
+		NL_SET_ERR_MSG_MOD(extack, "Unexpected region in snapshot function");
+		return -EOPNOTSUPP;
+	}
+
 	nvm_data = vzalloc(nvm_size);
 	if (!nvm_data)
 		return -ENOMEM;
 
-
 	num_blks = DIV_ROUND_UP(nvm_size, ICE_DEVLINK_READ_BLK_SIZE);
 	tmp = nvm_data;
 	left = nvm_size;
@@ -1656,7 +1667,7 @@ static int ice_devlink_nvm_snapshot(struct devlink *devlink,
 		}
 
 		status = ice_read_flat_nvm(hw, i * ICE_DEVLINK_READ_BLK_SIZE,
-					   &read_sz, tmp, false);
+					   &read_sz, tmp, read_shadow_ram);
 		if (status) {
 			dev_dbg(dev, "ice_read_flat_nvm failed after reading %u bytes, err %d aq_err %d\n",
 				read_sz, status, hw->adminq.sq_last_status);
@@ -1676,66 +1687,6 @@ static int ice_devlink_nvm_snapshot(struct devlink *devlink,
 	return 0;
 }
 
-/**
- * ice_devlink_sram_snapshot - Capture a snapshot of the Shadow RAM contents
- * @devlink: the devlink instance
- * @ops: the devlink region being snapshotted
- * @extack: extended ACK response structure
- * @data: on exit points to snapshot data buffer
- *
- * This function is called in response to the DEVLINK_CMD_REGION_TRIGGER for
- * the shadow-ram devlink region. It captures a snapshot of the shadow ram
- * contents. This snapshot can later be viewed via the devlink-region
- * interface.
- *
- * @returns zero on success, and updates the data pointer. Returns a non-zero
- * error code on failure.
- */
-static int
-ice_devlink_sram_snapshot(struct devlink *devlink,
-			  const struct devlink_region_ops __always_unused *ops,
-			  struct netlink_ext_ack *extack, u8 **data)
-{
-	struct ice_pf *pf = devlink_priv(devlink);
-	struct device *dev = ice_pf_to_dev(pf);
-	struct ice_hw *hw = &pf->hw;
-	u8 *sram_data;
-	u32 sram_size;
-	int err;
-
-	sram_size = hw->flash.sr_words * 2u;
-	sram_data = vzalloc(sram_size);
-	if (!sram_data)
-		return -ENOMEM;
-
-	err = ice_acquire_nvm(hw, ICE_RES_READ);
-	if (err) {
-		dev_dbg(dev, "ice_acquire_nvm failed, err %d aq_err %d\n",
-			err, hw->adminq.sq_last_status);
-		NL_SET_ERR_MSG_MOD(extack, "Failed to acquire NVM semaphore");
-		vfree(sram_data);
-		return err;
-	}
-
-	/* Read from the Shadow RAM, rather than directly from NVM */
-	err = ice_read_flat_nvm(hw, 0, &sram_size, sram_data, true);
-	if (err) {
-		dev_dbg(dev, "ice_read_flat_nvm failed after reading %u bytes, err %d aq_err %d\n",
-			sram_size, err, hw->adminq.sq_last_status);
-		NL_SET_ERR_MSG_MOD(extack,
-				   "Failed to read Shadow RAM contents");
-		ice_release_nvm(hw);
-		vfree(sram_data);
-		return err;
-	}
-
-	ice_release_nvm(hw);
-
-	*data = sram_data;
-
-	return 0;
-}
-
 /**
  * ice_devlink_devcaps_snapshot - Capture snapshot of device capabilities
  * @devlink: the devlink instance
@@ -1789,7 +1740,7 @@ static const struct devlink_region_ops ice_nvm_region_ops = {
 static const struct devlink_region_ops ice_sram_region_ops = {
 	.name = "shadow-ram",
 	.destructor = vfree,
-	.snapshot = ice_devlink_sram_snapshot,
+	.snapshot = ice_devlink_nvm_snapshot,
 };
 
 static const struct devlink_region_ops ice_devcaps_region_ops = {
-- 
2.38.1.420.g319605f8f00e

[PATCH net-next v2 8/9] ice: document 'shadow-ram' devlink region

[Jacob Keller]

78ad87da9978 ("ice: devlink: add shadow-ram region to snapshot Shadow RAM")
added support for the 'shadow-ram' devlink region, but did not document it
in the ice devlink documentation. Fix this.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
New patch, not in v1.

 Documentation/networking/devlink/ice.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst
index 890062da7820..bcd4839fbf79 100644
--- a/Documentation/networking/devlink/ice.rst
+++ b/Documentation/networking/devlink/ice.rst
@@ -189,6 +189,11 @@ device data.
     * - ``nvm-flash``
       - The contents of the entire flash chip, sometimes referred to as
         the device's Non Volatile Memory.
+    * - ``shadow-ram``
+      - The contents of the Shadow RAM, which is loaded from the beginning
+        of the flash. Although the contents are primarily from the flash,
+        this area also contains data generated during device boot which is
+        not stored in flash.
     * - ``device-caps``
       - The contents of the device firmware's capabilities buffer. Useful to
         determine the current state and configuration of the device.
-- 
2.38.1.420.g319605f8f00e

[PATCH net-next v2 9/9] ice: implement direct read for NVM and Shadow RAM regions

[Jacob Keller]

Implement the .read handler for the NVM and Shadow RAM regions. This
enables user space to read a small chunk of the flash without needing the
overhead of creating a full snapshot.

Update the documentation for ice to detail which regions have direct read
support.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes since v1
* Update documentation for ice.rst

 Documentation/networking/devlink/ice.rst     |  8 ++-
 drivers/net/ethernet/intel/ice/ice_devlink.c | 69 ++++++++++++++++++++
 2 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst
index bcd4839fbf79..625efb3777d5 100644
--- a/Documentation/networking/devlink/ice.rst
+++ b/Documentation/networking/devlink/ice.rst
@@ -198,8 +198,12 @@ device data.
       - The contents of the device firmware's capabilities buffer. Useful to
         determine the current state and configuration of the device.
 
-Users can request an immediate capture of a snapshot via the
-``DEVLINK_CMD_REGION_NEW``
+Both the ``nvm-flash`` and ``shadow-ram`` regions can be accessed without a
+snapshot. The ``device-caps`` region requires a snapshot as the contents are
+sent by firmware and can't be split into separate reads.
+
+Users can request an immediate capture of a snapshot for all three regions
+via the ``DEVLINK_CMD_REGION_NEW`` command.
 
 .. code:: shell
 
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 9d3500291d2e..946d64e577c9 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -1687,6 +1687,73 @@ static int ice_devlink_nvm_snapshot(struct devlink *devlink,
 	return 0;
 }
 
+/**
+ * ice_devlink_nvm_read - Read a portion of NVM flash contents
+ * @devlink: the devlink instance
+ * @ops: the devlink region to snapshot
+ * @extack: extended ACK response structure
+ * @offset: the offset to start at
+ * @size: the amount to read
+ * @data: the data buffer to read into
+ *
+ * This function is called in response to DEVLINK_CMD_REGION_READ to directly
+ * read a section of the NVM contents.
+ *
+ * It reads from either the nvm-flash or shadow-ram region contents.
+ *
+ * @returns zero on success, and updates the data pointer. Returns a non-zero
+ * error code on failure.
+ */
+static int ice_devlink_nvm_read(struct devlink *devlink,
+				const struct devlink_region_ops *ops,
+				struct netlink_ext_ack *extack,
+				u64 offset, u32 size, u8 *data)
+{
+	struct ice_pf *pf = devlink_priv(devlink);
+	struct device *dev = ice_pf_to_dev(pf);
+	struct ice_hw *hw = &pf->hw;
+	bool read_shadow_ram;
+	u64 nvm_size;
+	int status;
+
+	if (ops == &ice_nvm_region_ops) {
+		read_shadow_ram = false;
+		nvm_size = hw->flash.flash_size;
+	} else if (ops == &ice_sram_region_ops) {
+		read_shadow_ram = true;
+		nvm_size = hw->flash.sr_words * 2u;
+	} else {
+		NL_SET_ERR_MSG_MOD(extack, "Unexpected region in snapshot function");
+		return -EOPNOTSUPP;
+	}
+
+	if (offset + size >= nvm_size) {
+		NL_SET_ERR_MSG_MOD(extack, "Cannot read beyond the region size");
+		return -ERANGE;
+	}
+
+	status = ice_acquire_nvm(hw, ICE_RES_READ);
+	if (status) {
+		dev_dbg(dev, "ice_acquire_nvm failed, err %d aq_err %d\n",
+			status, hw->adminq.sq_last_status);
+		NL_SET_ERR_MSG_MOD(extack, "Failed to acquire NVM semaphore");
+		return -EIO;
+	}
+
+	status = ice_read_flat_nvm(hw, (u32)offset, &size, data,
+				   read_shadow_ram);
+	if (status) {
+		dev_dbg(dev, "ice_read_flat_nvm failed after reading %u bytes, err %d aq_err %d\n",
+			size, status, hw->adminq.sq_last_status);
+		NL_SET_ERR_MSG_MOD(extack, "Failed to read NVM contents");
+		ice_release_nvm(hw);
+		return -EIO;
+	}
+	ice_release_nvm(hw);
+
+	return 0;
+}
+
 /**
  * ice_devlink_devcaps_snapshot - Capture snapshot of device capabilities
  * @devlink: the devlink instance
@@ -1735,12 +1802,14 @@ static const struct devlink_region_ops ice_nvm_region_ops = {
 	.name = "nvm-flash",
 	.destructor = vfree,
 	.snapshot = ice_devlink_nvm_snapshot,
+	.read = ice_devlink_nvm_read,
 };
 
 static const struct devlink_region_ops ice_sram_region_ops = {
 	.name = "shadow-ram",
 	.destructor = vfree,
 	.snapshot = ice_devlink_nvm_snapshot,
+	.read = ice_devlink_nvm_read,
 };
 
 static const struct devlink_region_ops ice_devcaps_region_ops = {
-- 
2.38.1.420.g319605f8f00e

Re: [PATCH net-next v2 0/9] support direct read from region

[Jakub Kicinski]

On Wed, 23 Nov 2022 12:38:25 -0800 Jacob Keller wrote:
> Changes since v1:
> * Re-ordered patches at the beginning slightly, pulling min_t change and
>   reporting of extended error messages to the start of the series.
> * use NL_SET_ERR_MSG_ATTR for reporting invalid attributes
> * Use kmalloc instead of kzalloc
> * Cleanup spacing around data_size
> * Fix the __always_unused positioning
> * Update documentation for direct reads to clearly explain they are not
>   atomic for larger reads.
> * Add a patch to fix missing documentation for ice.rst
> * Mention the direct read support in ice.rst documentation

Acked-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net-next v2 1/9] devlink: use min_t to calculate data_size

[Jiri Pirko]

Wed, Nov 23, 2022 at 09:38:26PM CET, jacob.e.keller@intel.com wrote:
>The calculation for the data_size in the devlink_nl_read_snapshot_fill
>function uses an if statement that is better expressed using the min_t
>macro.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH net-next v2 2/9] devlink: report extended error message in region_read_dumpit

[Jiri Pirko]

Wed, Nov 23, 2022 at 09:38:27PM CET, jacob.e.keller@intel.com wrote:
>Report extended error details in the devlink_nl_cmd_region_read_dumpit

Nit: It is customary to use "()"s when mentioning the function name in
patch description.
w

>function, by using the extack structure from the netlink_callback.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH net-next v2 2/9] devlink: report extended error message in region_read_dumpit

[Jiri Pirko]

Wed, Nov 23, 2022 at 09:38:27PM CET, jacob.e.keller@intel.com wrote:

[...]


>@@ -6525,8 +6525,14 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 
> 	devl_lock(devlink);
> 
>-	if (!attrs[DEVLINK_ATTR_REGION_NAME] ||
>-	    !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
>+	if (!attrs[DEVLINK_ATTR_REGION_NAME]) {
>+		NL_SET_ERR_MSG(cb->extack, "No region name provided");
>+		err = -EINVAL;
>+		goto out_unlock;
>+	}
>+
>+	if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
>+		NL_SET_ERR_MSG(cb->extack, "No snapshot id provided");
> 		err = -EINVAL;
> 		goto out_unlock;
> 	}
>@@ -6541,7 +6547,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 		}
> 	}
> 
>-	region_name = nla_data(attrs[DEVLINK_ATTR_REGION_NAME]);
>+	region_attr = attrs[DEVLINK_ATTR_REGION_NAME];
>+	region_name = nla_data(region_attr);
> 
> 	if (port)
> 		region = devlink_port_region_get_by_name(port, region_name);
>@@ -6549,6 +6556,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 		region = devlink_region_get_by_name(devlink, region_name);
> 
> 	if (!region) {
>+		NL_SET_ERR_MSG_ATTR(cb->extack, region_attr, "requested region does not exist");

Any reason why don't start the message with uppercase? It would be
consistent with the other 2 messages you just introduced.
Same goes to the message in the next patch (perhaps some others too)


> 		err = -EINVAL;
> 		goto out_unlock;
> 	}
>-- 
>2.38.1.420.g319605f8f00e
>

Re: [PATCH net-next v2 3/9] devlink: find snapshot in devlink_nl_cmd_region_read_dumpit

[Jiri Pirko]

Wed, Nov 23, 2022 at 09:38:28PM CET, jacob.e.keller@intel.com wrote:
>The snapshot pointer is obtained inside of the function
>devlink_nl_region_read_snapshot_fill. Simplify this function by locating
>the snapshot upfront in devlink_nl_cmd_region_read_dumpit instead. This
>aligns with how other netlink attributes are handled, and allows us to
>exit slightly earlier if an invalid snapshot ID is provided.
>
>It also allows us to pass the snapshot pointer directly to the
>devlink_nl_region_read_snapshot_fill, and remove the now unused attrs
>parameter.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

With the msg uppercase start nit below.
Reviewed-by: Jiri Pirko <jiri@nvidia.com>



>---
>Changes since v1
>* Moved to 3/9 of series
>* Use snapshot_attr and NL_SET_ERR_MSG_ATTR to report extended error
>
> net/core/devlink.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index b5b317661f9a..825c52a71df1 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -6463,24 +6463,16 @@ static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
> 
> static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
> 						struct devlink *devlink,
>-						struct devlink_region *region,
>-						struct nlattr **attrs,
>+						struct devlink_snapshot *snapshot,
> 						u64 start_offset,
> 						u64 end_offset,
> 						u64 *new_offset)
> {
>-	struct devlink_snapshot *snapshot;
> 	u64 curr_offset = start_offset;
>-	u32 snapshot_id;
> 	int err = 0;
> 
> 	*new_offset = start_offset;
> 
>-	snapshot_id = nla_get_u32(attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]);
>-	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
>-	if (!snapshot)
>-		return -EINVAL;
>-
> 	while (curr_offset < end_offset) {
> 		u32 data_size;
> 		u8 *data;
>@@ -6506,14 +6498,16 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 					     struct netlink_callback *cb)
> {
> 	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>+	struct nlattr *chunks_attr, *region_attr, *snapshot_attr;
> 	u64 ret_offset, start_offset, end_offset = U64_MAX;
>-	struct nlattr *chunks_attr, *region_attr;
> 	struct nlattr **attrs = info->attrs;
> 	struct devlink_port *port = NULL;
>+	struct devlink_snapshot *snapshot;
> 	struct devlink_region *region;
> 	const char *region_name;
> 	struct devlink *devlink;
> 	unsigned int index;
>+	u32 snapshot_id;
> 	void *hdr;
> 	int err;
> 
>@@ -6561,6 +6555,15 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 		goto out_unlock;
> 	}
> 
>+	snapshot_attr = attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];
>+	snapshot_id = nla_get_u32(snapshot_attr);
>+	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
>+	if (!snapshot) {
>+		NL_SET_ERR_MSG_ATTR(cb->extack, snapshot_attr, "requested snapshot does not exist");

Why not to start with "R"?


>+		err = -EINVAL;
>+		goto out_unlock;
>+	}
>+
> 	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
> 	    attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]) {
> 		if (!start_offset)
>@@ -6610,7 +6613,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 	}
> 
> 	err = devlink_nl_region_read_snapshot_fill(skb, devlink,
>-						   region, attrs,
>+						   snapshot,
> 						   start_offset,
> 						   end_offset, &ret_offset);
> 
>-- 
>2.38.1.420.g319605f8f00e
>

Re: [PATCH net-next v2 4/9] devlink: remove unnecessary parameter from chunk_fill function

[Jiri Pirko]

Wed, Nov 23, 2022 at 09:38:29PM CET, jacob.e.keller@intel.com wrote:
>The devlink parameter of the devlink_nl_cmd_region_read_chunk_fill
>function is not used. Remove it, to simplify the function signature.
>
>Once removed, it is also obvious that the devlink parameter is not
>necessary for the devlink_nl_region_read_snapshot_fill either.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH net-next v2 6/9] devlink: support directly reading from region memory

[Jiri Pirko]

Wed, Nov 23, 2022 at 09:38:31PM CET, jacob.e.keller@intel.com wrote:
>To read from a region, user space must currently request a new snapshot of
>the region and then read from that snapshot. This can sometimes be overkill
>if user space only reads a tiny portion. They first create the snapshot,
>then request a read, then destroy the snapshot.
>
>For regions which have a single underlying "contents", it makes sense to
>allow supporting direct reading of the region data.
>
>Extend the DEVLINK_CMD_REGION_READ to allow direct reading from a region if
>supported. Instead of reporting a missing snapshot id as invalid, check if
>the region supports direct read and if so switch to the direct access
>method for reading the region data.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>---
>Changes since v1:
>* Update documentation to explain that direct reads are not atomic
>
> .../networking/devlink/devlink-region.rst     | 11 ++++
> include/net/devlink.h                         | 16 +++++
> net/core/devlink.c                            | 66 ++++++++++++++-----
> 3 files changed, 76 insertions(+), 17 deletions(-)
>
>diff --git a/Documentation/networking/devlink/devlink-region.rst b/Documentation/networking/devlink/devlink-region.rst
>index f06dca9a1eb6..6525a9120a4e 100644
>--- a/Documentation/networking/devlink/devlink-region.rst
>+++ b/Documentation/networking/devlink/devlink-region.rst
>@@ -31,6 +31,13 @@ in its ``devlink_region_ops`` structure. If snapshot id is not set in
> the ``DEVLINK_CMD_REGION_NEW`` request kernel will allocate one and send
> the snapshot information to user space.
> 
>+Regions may optionally allow directly reading from their contents without a
>+snapshot. Direct read requests are not atomic. In particular a read request
>+of size 256 bytes or larger will be split into multiple chunks. If atomic
>+access is required, use a snapshot. A driver wishing to enable this for a
>+region should implement the ``.read`` callback in the ``devlink_region_ops``
>+structure.
>+
> example usage
> -------------
> 
>@@ -65,6 +72,10 @@ example usage
>     $ devlink region read pci/0000:00:05.0/fw-health snapshot 1 address 0 length 16
>     0000000000000000 0014 95dc 0014 9514 0035 1670 0034 db30
> 
>+    # Read from the region without a snapshot
>+    $ devlink region read pci/0000:00:05.0/fw-health address 16 length 16
>+    0000000000000010 0000 0000 ffff ff04 0029 8c00 0028 8cc8
>+
> As regions are likely very device or driver specific, no generic regions are
> defined. See the driver-specific documentation files for information on the
> specific regions a driver supports.
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 074a79b8933f..02528f736f65 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -650,6 +650,10 @@ struct devlink_info_req;
>  *            the data variable must be updated to point to the snapshot data.
>  *            The function will be called while the devlink instance lock is
>  *            held.
>+ * @read: callback to directly read a portion of the region. On success,
>+ *        the data pointer will be updated with the contents of the
>+ *        requested portion of the region. The function will be called
>+ *        while the devlink instance lock is held.
>  * @priv: Pointer to driver private data for the region operation
>  */
> struct devlink_region_ops {
>@@ -659,6 +663,10 @@ struct devlink_region_ops {
> 			const struct devlink_region_ops *ops,
> 			struct netlink_ext_ack *extack,
> 			u8 **data);
>+	int (*read)(struct devlink *devlink,
>+		    const struct devlink_region_ops *ops,
>+		    struct netlink_ext_ack *extack,
>+		    u64 offset, u32 size, u8 *data);
> 	void *priv;
> };
> 
>@@ -670,6 +678,10 @@ struct devlink_region_ops {
>  *            the data variable must be updated to point to the snapshot data.
>  *            The function will be called while the devlink instance lock is
>  *            held.
>+ * @read: callback to directly read a portion of the region. On success,
>+ *        the data pointer will be updated with the contents of the
>+ *        requested portion of the region. The function will be called
>+ *        while the devlink instance lock is held.
>  * @priv: Pointer to driver private data for the region operation
>  */
> struct devlink_port_region_ops {
>@@ -679,6 +691,10 @@ struct devlink_port_region_ops {
> 			const struct devlink_port_region_ops *ops,
> 			struct netlink_ext_ack *extack,
> 			u8 **data);
>+	int (*read)(struct devlink_port *port,
>+		    const struct devlink_port_region_ops *ops,
>+		    struct netlink_ext_ack *extack,
>+		    u64 offset, u32 size, u8 *data);
> 	void *priv;
> };
> 
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 729e2162a4db..f249b5b57677 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -6515,6 +6515,26 @@ devlink_region_snapshot_fill(void *cb_priv, u8 *chunk, u32 chunk_size,
> 	return 0;
> }
> 
>+static int
>+devlink_region_port_direct_fill(void *cb_priv, u8 *chunk, u32 chunk_size,
>+				u64 curr_offset, struct netlink_ext_ack *extack)
>+{
>+	struct devlink_region *region = cb_priv;
>+
>+	return region->port_ops->read(region->port, region->port_ops, extack,
>+				      curr_offset, chunk_size, chunk);
>+}
>+
>+static int
>+devlink_region_direct_fill(void *cb_priv, u8 *chunk, u32 chunk_size,
>+			   u64 curr_offset, struct netlink_ext_ack *extack)
>+{
>+	struct devlink_region *region = cb_priv;
>+
>+	return region->ops->read(region->devlink, region->ops, extack,
>+				 curr_offset, chunk_size, chunk);
>+}
>+
> static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 					     struct netlink_callback *cb)
> {
>@@ -6523,12 +6543,12 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 	u64 ret_offset, start_offset, end_offset = U64_MAX;
> 	struct nlattr **attrs = info->attrs;
> 	struct devlink_port *port = NULL;
>-	struct devlink_snapshot *snapshot;
>+	devlink_chunk_fill_t *region_cb;
> 	struct devlink_region *region;
> 	const char *region_name;
> 	struct devlink *devlink;
> 	unsigned int index;
>-	u32 snapshot_id;
>+	void *region_cb_priv;
> 	void *hdr;
> 	int err;
> 
>@@ -6546,12 +6566,6 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 		goto out_unlock;
> 	}
> 
>-	if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
>-		NL_SET_ERR_MSG(cb->extack, "No snapshot id provided");
>-		err = -EINVAL;
>-		goto out_unlock;
>-	}
>-
> 	if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
> 		index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
> 
>@@ -6577,12 +6591,30 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 	}
> 
> 	snapshot_attr = attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];
>-	snapshot_id = nla_get_u32(snapshot_attr);
>-	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
>-	if (!snapshot) {
>-		NL_SET_ERR_MSG_ATTR(cb->extack, snapshot_attr, "requested snapshot does not exist");
>-		err = -EINVAL;
>-		goto out_unlock;
>+	if (!snapshot_attr) {

Hmm. Being pedantic here, it changes userspace expectations:
current - message without snapshotid is errored out
new - message without snapshotid is happily processed

I can imagine some obscure userspace app depending on this behaviour,
in theory.

Safe would be to add new NLA_FLAG attr DEVLINK_ATTR_REGION_DIRECT or
something that would indicate userspace is interested in direct read.
Also, the userspace would know right away it this new functionality
is supported or not by the kernel.



>+		if (!region->ops->read) {
>+			NL_SET_ERR_MSG(cb->extack, "requested region does not support direct read");
>+			err = -EOPNOTSUPP;
>+			goto out_unlock;
>+		}
>+		if (port)
>+			region_cb = &devlink_region_port_direct_fill;
>+		else
>+			region_cb = &devlink_region_direct_fill;
>+		region_cb_priv = region;
>+	} else {
>+		struct devlink_snapshot *snapshot;
>+		u32 snapshot_id;
>+
>+		snapshot_id = nla_get_u32(snapshot_attr);
>+		snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
>+		if (!snapshot) {
>+			NL_SET_ERR_MSG_ATTR(cb->extack, snapshot_attr, "requested snapshot does not exist");
>+			err = -EINVAL;
>+			goto out_unlock;
>+		}
>+		region_cb = &devlink_region_snapshot_fill;
>+		region_cb_priv = snapshot;
> 	}
> 
> 	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
>@@ -6633,9 +6665,9 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 		goto nla_put_failure;
> 	}
> 
>-	err = devlink_nl_region_read_fill(skb, &devlink_region_snapshot_fill,
>-					  snapshot, start_offset, end_offset,
>-					  &ret_offset, cb->extack);
>+	err = devlink_nl_region_read_fill(skb, region_cb, region_cb_priv,
>+					  start_offset, end_offset, &ret_offset,
>+					  cb->extack);
> 
> 	if (err && err != -EMSGSIZE)
> 		goto nla_put_failure;
>-- 
>2.38.1.420.g319605f8f00e
>

Re: [PATCH net-next v2 5/9] devlink: refactor region_read_snapshot_fill to use a callback function

[Jiri Pirko]

Wed, Nov 23, 2022 at 09:38:30PM CET, jacob.e.keller@intel.com wrote:
>The devlink_nl_region_read_snapshot_fill is used to copy the contents of
>a snapshot into a message for reporting to userspace via the
>DEVLINK_CMG_REGION_READ netlink message.
>
>A future change is going to add support for directly reading from
>a region. Almost all of the logic for this new capability is identical.
>
>To help reduce code duplication and make this logic more generic,
>refactor the function to take a cb and cb_priv pointer for doing the
>actual copy.
>
>Add a devlink_region_snapshot_fill implementation that will simply copy
>the relevant chunk of the region. This does require allocating some
>storage for the chunk as opposed to simply passing the correct address
>forward to the devlink_nl_cmg_region_read_chunk_fill function.
>
>A future change to implement support for directly reading from a region
>without a snapshot will provide a separate implementation that calls the
>newly added devlink region operation.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>---
>Changes since v1:
>* Use kmalloc instead of kzalloc
>* Don't combine data_size declaration and assignment
>* Fix the always_unused placement for devlink_region_snapshot_fill
>
> net/core/devlink.c | 44 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 35 insertions(+), 9 deletions(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index bd7af0600405..729e2162a4db 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -6460,25 +6460,36 @@ static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
> 
> #define DEVLINK_REGION_READ_CHUNK_SIZE 256
> 
>-static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
>-						struct devlink_snapshot *snapshot,
>-						u64 start_offset,
>-						u64 end_offset,
>-						u64 *new_offset)
>+typedef int devlink_chunk_fill_t(void *cb_priv, u8 *chunk, u32 chunk_size,
>+				 u64 curr_offset,
>+				 struct netlink_ext_ack *extack);
>+
>+static int
>+devlink_nl_region_read_fill(struct sk_buff *skb, devlink_chunk_fill_t *cb,
>+			    void *cb_priv, u64 start_offset, u64 end_offset,
>+			    u64 *new_offset, struct netlink_ext_ack *extack)
> {
> 	u64 curr_offset = start_offset;
> 	int err = 0;
>+	u8 *data;
>+
>+	/* Allocate and re-use a single buffer */
>+	data = kmalloc(DEVLINK_REGION_READ_CHUNK_SIZE, GFP_KERNEL);

Hmm, I tried to figure out how to do this without extra alloc and
memcpy, didn't find any nice solution :/

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Btw, do you plan to extend this for write as well. It might be valuable
for debugging purposes to have that. I recall we discussed it in past.



>+	if (!data)
>+		return -ENOMEM;
> 
> 	*new_offset = start_offset;
> 
> 	while (curr_offset < end_offset) {
> 		u32 data_size;
>-		u8 *data;
> 
> 		data_size = min_t(u32, end_offset - curr_offset,
> 				  DEVLINK_REGION_READ_CHUNK_SIZE);
> 
>-		data = &snapshot->data[curr_offset];
>+		err = cb(cb_priv, data, data_size, curr_offset, extack);
>+		if (err)
>+			break;
>+
> 		err = devlink_nl_cmd_region_read_chunk_fill(skb, data, data_size, curr_offset);
> 		if (err)
> 			break;
>@@ -6487,9 +6498,23 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
> 	}
> 	*new_offset = curr_offset;
> 
>+	kfree(data);
>+
> 	return err;
> }
> 
>+static int
>+devlink_region_snapshot_fill(void *cb_priv, u8 *chunk, u32 chunk_size,
>+			     u64 curr_offset,
>+			     struct netlink_ext_ack __always_unused *extack)
>+{
>+	struct devlink_snapshot *snapshot = cb_priv;
>+
>+	memcpy(chunk, &snapshot->data[curr_offset], chunk_size);
>+
>+	return 0;
>+}
>+
> static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 					     struct netlink_callback *cb)
> {
>@@ -6608,8 +6633,9 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 		goto nla_put_failure;
> 	}
> 
>-	err = devlink_nl_region_read_snapshot_fill(skb, snapshot, start_offset,
>-						   end_offset, &ret_offset);
>+	err = devlink_nl_region_read_fill(skb, &devlink_region_snapshot_fill,
>+					  snapshot, start_offset, end_offset,
>+					  &ret_offset, cb->extack);
> 
> 	if (err && err != -EMSGSIZE)
> 		goto nla_put_failure;
>-- 
>2.38.1.420.g319605f8f00e
>

RE: [PATCH net-next v2 1/9] devlink: use min_t to calculate data_size

[David Laight]

From: Jacob Keller
> Sent: 23 November 2022 20:38
> 
> The calculation for the data_size in the devlink_nl_read_snapshot_fill
> function uses an if statement that is better expressed using the min_t
> macro.

There ought to be a 'duck shoot' arranged for all uses of min_t().
I was testing a patch (I might submit next week) that relaxes the
checks in min() so that it doesn't error a lot of valid cases.
In particular a positive integer constant can always be cast to (int)
and the compare will DTRT.

I found things like min_t(u32, u32_length, u64_limit) where
you really don't want to mask the limit down.
There are also the min_t(u8, ...) and min_t(u16, ...).


...
> +		data_size = min_t(u32, end_offset - curr_offset,
> +				  DEVLINK_REGION_READ_CHUNK_SIZE);

Here I think both xxx_offset are u32 - so the CHUNK_SIZE
constant probably needs a U suffix.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH net-next v2 2/9] devlink: report extended error message in region_read_dumpit

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Thursday, November 24, 2022 12:47 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Jiri Pirko <jiri@nvidia.com>; Jakub Kicinski
> <kuba@kernel.org>
> Subject: Re: [PATCH net-next v2 2/9] devlink: report extended error message in
> region_read_dumpit
> 
> Wed, Nov 23, 2022 at 09:38:27PM CET, jacob.e.keller@intel.com wrote:
> 
> [...]
> 
> 
> >@@ -6525,8 +6525,14 @@ static int
> devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> >
> > 	devl_lock(devlink);
> >
> >-	if (!attrs[DEVLINK_ATTR_REGION_NAME] ||
> >-	    !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
> >+	if (!attrs[DEVLINK_ATTR_REGION_NAME]) {
> >+		NL_SET_ERR_MSG(cb->extack, "No region name provided");
> >+		err = -EINVAL;
> >+		goto out_unlock;
> >+	}
> >+
> >+	if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) {
> >+		NL_SET_ERR_MSG(cb->extack, "No snapshot id provided");
> > 		err = -EINVAL;
> > 		goto out_unlock;
> > 	}
> >@@ -6541,7 +6547,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct
> sk_buff *skb,
> > 		}
> > 	}
> >
> >-	region_name = nla_data(attrs[DEVLINK_ATTR_REGION_NAME]);
> >+	region_attr = attrs[DEVLINK_ATTR_REGION_NAME];
> >+	region_name = nla_data(region_attr);
> >
> > 	if (port)
> > 		region = devlink_port_region_get_by_name(port, region_name);
> >@@ -6549,6 +6556,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct
> sk_buff *skb,
> > 		region = devlink_region_get_by_name(devlink, region_name);
> >
> > 	if (!region) {
> >+		NL_SET_ERR_MSG_ATTR(cb->extack, region_attr, "requested
> region does not exist");
> 
> Any reason why don't start the message with uppercase? It would be
> consistent with the other 2 messages you just introduced.
> Same goes to the message in the next patch (perhaps some others too)
> 
> 

No particular reason. I'll fix these.

> > 		err = -EINVAL;
> > 		goto out_unlock;
> > 	}
> >--
> >2.38.1.420.g319605f8f00e
> >

RE: [PATCH net-next v2 5/9] devlink: refactor region_read_snapshot_fill to use a callback function

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Thursday, November 24, 2022 1:12 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev@vger.kernel.org; Jiri Pirko <jiri@nvidia.com>; Jakub Kicinski
> <kuba@kernel.org>
> Subject: Re: [PATCH net-next v2 5/9] devlink: refactor region_read_snapshot_fill
> to use a callback function
> 
> Wed, Nov 23, 2022 at 09:38:30PM CET, jacob.e.keller@intel.com wrote:
> >The devlink_nl_region_read_snapshot_fill is used to copy the contents of
> >a snapshot into a message for reporting to userspace via the
> >DEVLINK_CMG_REGION_READ netlink message.
> >
> >A future change is going to add support for directly reading from
> >a region. Almost all of the logic for this new capability is identical.
> >
> >To help reduce code duplication and make this logic more generic,
> >refactor the function to take a cb and cb_priv pointer for doing the
> >actual copy.
> >
> >Add a devlink_region_snapshot_fill implementation that will simply copy
> >the relevant chunk of the region. This does require allocating some
> >storage for the chunk as opposed to simply passing the correct address
> >forward to the devlink_nl_cmg_region_read_chunk_fill function.
> >
> >A future change to implement support for directly reading from a region
> >without a snapshot will provide a separate implementation that calls the
> >newly added devlink region operation.
> >
> >Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> >---
> >Changes since v1:
> >* Use kmalloc instead of kzalloc
> >* Don't combine data_size declaration and assignment
> >* Fix the always_unused placement for devlink_region_snapshot_fill
> >
> > net/core/devlink.c | 44 +++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 35 insertions(+), 9 deletions(-)
> >
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index bd7af0600405..729e2162a4db 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -6460,25 +6460,36 @@ static int
> devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
> >
> > #define DEVLINK_REGION_READ_CHUNK_SIZE 256
> >
> >-static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
> >-						struct devlink_snapshot
> *snapshot,
> >-						u64 start_offset,
> >-						u64 end_offset,
> >-						u64 *new_offset)
> >+typedef int devlink_chunk_fill_t(void *cb_priv, u8 *chunk, u32 chunk_size,
> >+				 u64 curr_offset,
> >+				 struct netlink_ext_ack *extack);
> >+
> >+static int
> >+devlink_nl_region_read_fill(struct sk_buff *skb, devlink_chunk_fill_t *cb,
> >+			    void *cb_priv, u64 start_offset, u64 end_offset,
> >+			    u64 *new_offset, struct netlink_ext_ack *extack)
> > {
> > 	u64 curr_offset = start_offset;
> > 	int err = 0;
> >+	u8 *data;
> >+
> >+	/* Allocate and re-use a single buffer */
> >+	data = kmalloc(DEVLINK_REGION_READ_CHUNK_SIZE, GFP_KERNEL);
> 
> Hmm, I tried to figure out how to do this without extra alloc and
> memcpy, didn't find any nice solution :/
> 

I also came up blank as well :( I can take another look when sending v3 with the other fixups.

> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> 
> Btw, do you plan to extend this for write as well. It might be valuable
> for debugging purposes to have that. I recall we discussed it in past.
> 

We could extend to support writing. I haven't had a strong need yet, and I worry about the potential for abuse of such an interface. We've already seen examples of people (ab)using such interfaces for unclear purposes...

Thanks,
Jake

Re: [PATCH net-next v2 1/9] devlink: use min_t to calculate data_size

[Jacob Keller]

On 11/24/2022 1:53 PM, David Laight wrote:
> From: Jacob Keller
>> Sent: 23 November 2022 20:38
>>
>> The calculation for the data_size in the devlink_nl_read_snapshot_fill
>> function uses an if statement that is better expressed using the min_t
>> macro.
> 
> There ought to be a 'duck shoot' arranged for all uses of min_t().
> I was testing a patch (I might submit next week) that relaxes the
> checks in min() so that it doesn't error a lot of valid cases.
> In particular a positive integer constant can always be cast to (int)
> and the compare will DTRT.
> 
> I found things like min_t(u32, u32_length, u64_limit) where
> you really don't want to mask the limit down.
> There are also the min_t(u8, ...) and min_t(u16, ...).
> 

Wouldn't that example just want to be min_t(u64, ...)?

> 
> ...
>> +		data_size = min_t(u32, end_offset - curr_offset,
>> +				  DEVLINK_REGION_READ_CHUNK_SIZE);
> 
> Here I think both xxx_offset are u32 - so the CHUNK_SIZE
> constant probably needs a U suffix.

Right. My understanding was that min_t would cast everything to a u32 
when doing such comparison, and we know that 
DEVLINK_REGION_READ_CHUNK_SIZE is < U32_MAX so this is ok?

Or am I misunderstanding?

> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

Re: [PATCH net-next v2 6/9] devlink: support directly reading from region memory

[Jacob Keller]

On 11/24/2022 1:05 AM, Jiri Pirko wrote:
> Wed, Nov 23, 2022 at 09:38:31PM CET, jacob.e.keller@intel.com wrote:
> 
> Hmm. Being pedantic here, it changes userspace expectations:
> current - message without snapshotid is errored out
> new - message without snapshotid is happily processed
> 

Yea. I guess I'm thinking more from an interactive application 
standpoint this wouldn't be a problem but from a scripted setup it might 
do something weird.

> I can imagine some obscure userspace app depending on this behaviour,
> in theory.

Hmm.

> 
> Safe would be to add new NLA_FLAG attr DEVLINK_ATTR_REGION_DIRECT or
> something that would indicate userspace is interested in direct read.
> Also, the userspace would know right away it this new functionality
> is supported or not by the kernel.
> 

The advantage of being able to see that such a feature exists is good.

I can rework this to add an attribute.

> 
> 
>> +		if (!region->ops->read) {
>> +			NL_SET_ERR_MSG(cb->extack, "requested region does not support direct read");
>> +			err = -EOPNOTSUPP;
>> +			goto out_unlock;
>> +		}
>> +		if (port)
>> +			region_cb = &devlink_region_port_direct_fill;
>> +		else
>> +			region_cb = &devlink_region_direct_fill;
>> +		region_cb_priv = region;
>> +	} else {
>> +		struct devlink_snapshot *snapshot;
>> +		u32 snapshot_id;
>> +
>> +		snapshot_id = nla_get_u32(snapshot_attr);
>> +		snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
>> +		if (!snapshot) {
>> +			NL_SET_ERR_MSG_ATTR(cb->extack, snapshot_attr, "requested snapshot does not exist");
>> +			err = -EINVAL;
>> +			goto out_unlock;
>> +		}
>> +		region_cb = &devlink_region_snapshot_fill;
>> +		region_cb_priv = snapshot;
>> 	}
>>
>> 	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
>> @@ -6633,9 +6665,9 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>> 		goto nla_put_failure;
>> 	}
>>
>> -	err = devlink_nl_region_read_fill(skb, &devlink_region_snapshot_fill,
>> -					  snapshot, start_offset, end_offset,
>> -					  &ret_offset, cb->extack);
>> +	err = devlink_nl_region_read_fill(skb, region_cb, region_cb_priv,
>> +					  start_offset, end_offset, &ret_offset,
>> +					  cb->extack);
>>
>> 	if (err && err != -EMSGSIZE)
>> 		goto nla_put_failure;
>> -- 
>> 2.38.1.420.g319605f8f00e
>>

Re: [PATCH net-next v2 5/9] devlink: refactor region_read_snapshot_fill to use a callback function

[Jakub Kicinski]

On Mon, 28 Nov 2022 18:27:42 +0000 Keller, Jacob E wrote:
> > Hmm, I tried to figure out how to do this without extra alloc and
> > memcpy, didn't find any nice solution :/
> 
> I also came up blank as well :( I can take another look when sending v3 with the other fixups.

You can certainly rearrange things to nla_reserve() the space in the
message and pass to the driver a pointer to a buffer already in the
skb. But I don't think it's worth the code complexity.

RE: [PATCH net-next v2 5/9] devlink: refactor region_read_snapshot_fill to use a callback function

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, November 28, 2022 11:00 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org; Jiri Pirko
> <jiri@nvidia.com>
> Subject: Re: [PATCH net-next v2 5/9] devlink: refactor region_read_snapshot_fill
> to use a callback function
> 
> On Mon, 28 Nov 2022 18:27:42 +0000 Keller, Jacob E wrote:
> > > Hmm, I tried to figure out how to do this without extra alloc and
> > > memcpy, didn't find any nice solution :/
> >
> > I also came up blank as well :( I can take another look when sending v3 with the
> other fixups.
> 
> You can certainly rearrange things to nla_reserve() the space in the
> message and pass to the driver a pointer to a buffer already in the
> skb. But I don't think it's worth the code complexity.

Fair enough. I'll leave that as-is for v3.

Thanks,
Jake

RE: [PATCH net-next v2 1/9] devlink: use min_t to calculate data_size

[David Laight]

From: Jacob Keller
> Sent: 28 November 2022 18:31
> 
> On 11/24/2022 1:53 PM, David Laight wrote:
> > From: Jacob Keller
> >> Sent: 23 November 2022 20:38
> >>
> >> The calculation for the data_size in the devlink_nl_read_snapshot_fill
> >> function uses an if statement that is better expressed using the min_t
> >> macro.
> >
> > There ought to be a 'duck shoot' arranged for all uses of min_t().
> > I was testing a patch (I might submit next week) that relaxes the
> > checks in min() so that it doesn't error a lot of valid cases.
> > In particular a positive integer constant can always be cast to (int)
> > and the compare will DTRT.
> >
> > I found things like min_t(u32, u32_length, u64_limit) where
> > you really don't want to mask the limit down.
> > There are also the min_t(u8, ...) and min_t(u16, ...).
> >
> 
> Wouldn't that example just want to be min_t(u64, ...)?

That is what is would need to be.
But the compiler can work it out and get it right.

> > ...
> >> +		data_size = min_t(u32, end_offset - curr_offset,
> >> +				  DEVLINK_REGION_READ_CHUNK_SIZE);
> >
> > Here I think both xxx_offset are u32 - so the CHUNK_SIZE
> > constant probably needs a U suffix.
> 
> Right. My understanding was that min_t would cast everything to a u32
> when doing such comparison, and we know that
> DEVLINK_REGION_READ_CHUNK_SIZE is < U32_MAX so this is ok?
> 
> Or am I misunderstanding?

The code isn't wrong, except that errors from min() are really
an indication that the types mismatch, not that you should add
loads of casts.
You wouldn't think:
	x = (int)a + (int)b;
was anything normal, but that is what min_t() does.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)