[PATCH iwl-next v2 0/2] Refactor sending DDP + E830 support

[PATCH iwl-next v2 0/2] Refactor sending DDP + E830 support

[Przemek Kitszel]

This series refactors sending DDP segments in accordance to computing
"last" bit of AQ request (1st patch), then adds support for extended
format ("valid" + "last" bits in a new "flags" field) of DDP that was
changed to support Multi-Segment DDP packages needed by E830.

v1:
adjusted authorship, rebase, minor kdoc fix
https://lore.kernel.org/intel-wired-lan/20240911110926.25384-4-przemyslaw.kitszel@intel.com

Przemek Kitszel (2):
  ice: refactor "last" segment of DDP pkg
  ice: support optional flags in signature segment header

 drivers/net/ethernet/intel/ice/ice_ddp.h |   5 +-
 drivers/net/ethernet/intel/ice/ice_ddp.c | 292 ++++++++++++-----------
 2 files changed, 160 insertions(+), 137 deletions(-)


base-commit: f2589ad16e14b7102f1411e3385a2abf07076406
-- 
2.46.0

[PATCH iwl-next v2 1/2] ice: refactor "last" segment of DDP pkg

[Przemek Kitszel]

Add ice_ddp_send_hunk() that buffers "sent FW hunk" calls to AQ in order
to mark the "last" one in more elegant way. Next commit will add even
more complicated "sent FW" flow, so it's better to untangle a bit before.

Note that metadata buffers were not skipped for NOT-@indicate_last
segments, this is fixed now.

Minor:
 + use ice_is_buffer_metadata() instead of open coding it in
   ice_dwnld_cfg_bufs();
 + ice_dwnld_cfg_bufs_no_lock() + dependencies were moved up a bit to have
   better git-diff, as this function was rewritten (in terms of git-blame)

CC: Paul Greenwalt <paul.greenwalt@intel.com>
CC: Dan Nowlin <dan.nowlin@intel.com>
CC: Ahmed Zaki <ahmed.zaki@intel.com>
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
git: --inter-hunk-context=6

v2: fixed one kdoc warning
---
 drivers/net/ethernet/intel/ice/ice_ddp.c | 280 ++++++++++++-----------
 1 file changed, 145 insertions(+), 135 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
index 016fcab6ba34..a2bb8442f281 100644
--- a/drivers/net/ethernet/intel/ice/ice_ddp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
@@ -1210,6 +1210,127 @@ ice_aq_download_pkg(struct ice_hw *hw, struct ice_buf_hdr *pkg_buf,
 	return status;
 }
 
+/**
+ * ice_is_buffer_metadata - determine if package buffer is a metadata buffer
+ * @buf: pointer to buffer header
+ * Return: whether given @buf is a metadata one.
+ */
+static bool ice_is_buffer_metadata(struct ice_buf_hdr *buf)
+{
+	return le32_to_cpu(buf->section_entry[0].type) & ICE_METADATA_BUF;
+}
+
+/**
+ * struct ice_ddp_send_ctx - sending context of current DDP segment
+ * @hw: pointer to the hardware struct
+ *
+ * Keeps current sending state (header, error) for the purpose of proper "last"
+ * bit settting in ice_aq_download_pkg(). Use via calls to ice_ddp_send_hunk().
+ */
+struct ice_ddp_send_ctx {
+	struct ice_hw *hw;
+/* private: only for ice_ddp_send_hunk() */
+	struct ice_buf_hdr *hdr;
+	int err;
+};
+
+/**
+ * ice_ddp_send_hunk - send one hunk of data to FW
+ * @ctx - current segment sending context
+ * @hunk - next hunk to send, size is always ICE_PKG_BUF_SIZE
+ *
+ * Send the next hunk of data to FW, retrying if needed.
+ *
+ * Notice: must be called once more with a NULL @hunk to finish up; such call
+ * will set up the "last" bit of an AQ request. After such call @ctx.hdr is
+ * cleared, @hw is still valid.
+ *
+ * Return: %ICE_DDP_PKG_SUCCESS if there were no problems; a sticky @err
+ *         otherwise.
+ */
+static enum ice_ddp_state ice_ddp_send_hunk(struct ice_ddp_send_ctx *ctx,
+					    struct ice_buf_hdr *hunk)
+{
+	struct ice_buf_hdr *prev_hunk = ctx->hdr;
+	struct ice_hw *hw = ctx->hw;
+	bool prev_was_last = !hunk;
+	enum ice_aq_err aq_err;
+	u32 offset, info;
+	int attempt, err;
+
+	if (ctx->err)
+		return ctx->err;
+
+	ctx->hdr = hunk;
+	if (!prev_hunk)
+		return ICE_DDP_PKG_SUCCESS; /* no problem so far */
+
+	for (attempt = 0; attempt < 5; attempt++) {
+		if (attempt)
+			msleep(20);
+
+		err = ice_aq_download_pkg(hw, prev_hunk, ICE_PKG_BUF_SIZE,
+					  prev_was_last, &offset, &info, NULL);
+
+		aq_err = hw->adminq.sq_last_status;
+		if (aq_err != ICE_AQ_RC_ENOSEC && aq_err != ICE_AQ_RC_EBADSIG)
+			break;
+	}
+
+	if (err) {
+		ice_debug(hw, ICE_DBG_PKG, "Pkg download failed: err %d off %d inf %d\n",
+			  err, offset, info);
+		ctx->err = ice_map_aq_err_to_ddp_state(aq_err);
+	} else if (attempt) {
+		dev_dbg(ice_hw_to_dev(hw),
+			"ice_aq_download_pkg number of retries: %d\n", attempt);
+	}
+
+	return ctx->err;
+}
+
+/**
+ * ice_dwnld_cfg_bufs_no_lock
+ * @ctx: context of the current buffers section to send
+ * @bufs: pointer to an array of buffers
+ * @start: buffer index of first buffer to download
+ * @count: the number of buffers to download
+ *
+ * Downloads package configuration buffers to the firmware. Metadata buffers
+ * are skipped, and the first metadata buffer found indicates that the rest
+ * of the buffers are all metadata buffers.
+ */
+static enum ice_ddp_state
+ice_dwnld_cfg_bufs_no_lock(struct ice_ddp_send_ctx *ctx, struct ice_buf *bufs,
+			   u32 start, u32 count)
+{
+	struct ice_buf_hdr *bh;
+	enum ice_ddp_state err;
+
+	if (!bufs || !count) {
+		ctx->err = ICE_DDP_PKG_ERR;
+		return ctx->err;
+	}
+
+	bufs += start;
+	bh = (struct ice_buf_hdr *)bufs;
+
+	for (int i = 0; i < count; i++, bufs++) {
+		bh = (struct ice_buf_hdr *)bufs;
+		/* Metadata buffers should not be sent to FW,
+		 * their presence means "we are done here".
+		 */
+		if (ice_is_buffer_metadata(bh))
+			break;
+
+		err = ice_ddp_send_hunk(ctx, bh);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 /**
  * ice_get_pkg_seg_by_idx
  * @pkg_hdr: pointer to the package header to be searched
@@ -1269,147 +1390,32 @@ ice_is_signing_seg_type_at_idx(struct ice_pkg_hdr *pkg_hdr, u32 idx,
 	return false;
 }
 
-/**
- * ice_is_buffer_metadata - determine if package buffer is a metadata buffer
- * @buf: pointer to buffer header
- */
-static bool ice_is_buffer_metadata(struct ice_buf_hdr *buf)
-{
-	if (le32_to_cpu(buf->section_entry[0].type) & ICE_METADATA_BUF)
-		return true;
-
-	return false;
-}
-
-/**
- * ice_is_last_download_buffer
- * @buf: pointer to current buffer header
- * @idx: index of the buffer in the current sequence
- * @count: the buffer count in the current sequence
- *
- * Note: this routine should only be called if the buffer is not the last buffer
- */
-static bool
-ice_is_last_download_buffer(struct ice_buf_hdr *buf, u32 idx, u32 count)
-{
-	struct ice_buf *next_buf;
-
-	if ((idx + 1) == count)
-		return true;
-
-	/* A set metadata flag in the next buffer will signal that the current
-	 * buffer will be the last buffer downloaded
-	 */
-	next_buf = ((struct ice_buf *)buf) + 1;
-
-	return ice_is_buffer_metadata((struct ice_buf_hdr *)next_buf);
-}
-
-/**
- * ice_dwnld_cfg_bufs_no_lock
- * @hw: pointer to the hardware structure
- * @bufs: pointer to an array of buffers
- * @start: buffer index of first buffer to download
- * @count: the number of buffers to download
- * @indicate_last: if true, then set last buffer flag on last buffer download
- *
- * Downloads package configuration buffers to the firmware. Metadata buffers
- * are skipped, and the first metadata buffer found indicates that the rest
- * of the buffers are all metadata buffers.
- */
-static enum ice_ddp_state
-ice_dwnld_cfg_bufs_no_lock(struct ice_hw *hw, struct ice_buf *bufs, u32 start,
-			   u32 count, bool indicate_last)
-{
-	enum ice_ddp_state state = ICE_DDP_PKG_SUCCESS;
-	struct ice_buf_hdr *bh;
-	enum ice_aq_err err;
-	u32 offset, info, i;
-
-	if (!bufs || !count)
-		return ICE_DDP_PKG_ERR;
-
-	/* If the first buffer's first section has its metadata bit set
-	 * then there are no buffers to be downloaded, and the operation is
-	 * considered a success.
-	 */
-	bh = (struct ice_buf_hdr *)(bufs + start);
-	if (le32_to_cpu(bh->section_entry[0].type) & ICE_METADATA_BUF)
-		return ICE_DDP_PKG_SUCCESS;
-
-	for (i = 0; i < count; i++) {
-		bool last = false;
-		int try_cnt = 0;
-		int status;
-
-		bh = (struct ice_buf_hdr *)(bufs + start + i);
-
-		if (indicate_last)
-			last = ice_is_last_download_buffer(bh, i, count);
-
-		while (1) {
-			status = ice_aq_download_pkg(hw, bh, ICE_PKG_BUF_SIZE,
-						     last, &offset, &info,
-						     NULL);
-			if (hw->adminq.sq_last_status != ICE_AQ_RC_ENOSEC &&
-			    hw->adminq.sq_last_status != ICE_AQ_RC_EBADSIG)
-				break;
-
-			try_cnt++;
-
-			if (try_cnt == 5)
-				break;
-
-			msleep(20);
-		}
-
-		if (try_cnt)
-			dev_dbg(ice_hw_to_dev(hw),
-				"ice_aq_download_pkg number of retries: %d\n",
-				try_cnt);
-
-		/* Save AQ status from download package */
-		if (status) {
-			ice_debug(hw, ICE_DBG_PKG, "Pkg download failed: err %d off %d inf %d\n",
-				  status, offset, info);
-			err = hw->adminq.sq_last_status;
-			state = ice_map_aq_err_to_ddp_state(err);
-			break;
-		}
-
-		if (last)
-			break;
-	}
-
-	return state;
-}
-
 /**
  * ice_download_pkg_sig_seg - download a signature segment
- * @hw: pointer to the hardware structure
+ * @ctx: context of the current buffers section to send
  * @seg: pointer to signature segment
  */
 static enum ice_ddp_state
-ice_download_pkg_sig_seg(struct ice_hw *hw, struct ice_sign_seg *seg)
+ice_download_pkg_sig_seg(struct ice_ddp_send_ctx *ctx, struct ice_sign_seg *seg)
 {
-	return  ice_dwnld_cfg_bufs_no_lock(hw, seg->buf_tbl.buf_array, 0,
-					   le32_to_cpu(seg->buf_tbl.buf_count),
-					   false);
+	return ice_dwnld_cfg_bufs_no_lock(ctx, seg->buf_tbl.buf_array, 0,
+					  le32_to_cpu(seg->buf_tbl.buf_count));
 }
 
 /**
  * ice_download_pkg_config_seg - download a config segment
- * @hw: pointer to the hardware structure
+ * @ctx: context of the current buffers section to send
  * @pkg_hdr: pointer to package header
  * @idx: segment index
  * @start: starting buffer
  * @count: buffer count
  *
  * Note: idx must reference a ICE segment
  */
 static enum ice_ddp_state
-ice_download_pkg_config_seg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr,
-			    u32 idx, u32 start, u32 count)
+ice_download_pkg_config_seg(struct ice_ddp_send_ctx *ctx,
+			    struct ice_pkg_hdr *pkg_hdr, u32 idx, u32 start,
+			    u32 count)
 {
 	struct ice_buf_table *bufs;
 	struct ice_seg *seg;
@@ -1425,21 +1431,20 @@ ice_download_pkg_config_seg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr,
 	if (start >= buf_count || start + count > buf_count)
 		return ICE_DDP_PKG_ERR;
 
-	return  ice_dwnld_cfg_bufs_no_lock(hw, bufs->buf_array, start, count,
-					   true);
+	return ice_dwnld_cfg_bufs_no_lock(ctx, bufs->buf_array, start, count);
 }
 
 /**
  * ice_dwnld_sign_and_cfg_segs - download a signing segment and config segment
- * @hw: pointer to the hardware structure
+ * @ctx: context of the current buffers section to send
  * @pkg_hdr: pointer to package header
  * @idx: segment index (must be a signature segment)
  *
  * Note: idx must reference a signature segment
  */
 static enum ice_ddp_state
-ice_dwnld_sign_and_cfg_segs(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr,
-			    u32 idx)
+ice_dwnld_sign_and_cfg_segs(struct ice_ddp_send_ctx *ctx,
+			    struct ice_pkg_hdr *pkg_hdr, u32 idx)
 {
 	enum ice_ddp_state state;
 	struct ice_sign_seg *seg;
@@ -1454,17 +1459,16 @@ ice_dwnld_sign_and_cfg_segs(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr,
 	}
 
 	count = le32_to_cpu(seg->signed_buf_count);
-	state = ice_download_pkg_sig_seg(hw, seg);
+	state = ice_download_pkg_sig_seg(ctx, seg);
 	if (state || !count)
 		goto exit;
 
 	conf_idx = le32_to_cpu(seg->signed_seg_idx);
 	start = le32_to_cpu(seg->signed_buf_start);
 
-	state = ice_download_pkg_config_seg(hw, pkg_hdr, conf_idx, start,
-					    count);
-
+	return ice_download_pkg_config_seg(ctx, pkg_hdr, conf_idx, start, count);
 exit:
+	ctx->err = state;
 	return state;
 }
 
@@ -1519,6 +1523,7 @@ ice_download_pkg_with_sig_seg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr)
 {
 	enum ice_aq_err aq_err = hw->adminq.sq_last_status;
 	enum ice_ddp_state state = ICE_DDP_PKG_ERR;
+	struct ice_ddp_send_ctx ctx = { .hw = hw };
 	int status;
 	u32 i;
 
@@ -1539,7 +1544,9 @@ ice_download_pkg_with_sig_seg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr)
 						    hw->pkg_sign_type))
 			continue;
 
-		state = ice_dwnld_sign_and_cfg_segs(hw, pkg_hdr, i);
+		ice_dwnld_sign_and_cfg_segs(&ctx, pkg_hdr, i);
+		/* finish up by sending last hunk with "last" flag set */
+		state = ice_ddp_send_hunk(&ctx, NULL);
 		if (state)
 			break;
 	}
@@ -1564,6 +1571,7 @@ ice_download_pkg_with_sig_seg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr)
 static enum ice_ddp_state
 ice_dwnld_cfg_bufs(struct ice_hw *hw, struct ice_buf *bufs, u32 count)
 {
+	struct ice_ddp_send_ctx ctx = { .hw = hw };
 	enum ice_ddp_state state;
 	struct ice_buf_hdr *bh;
 	int status;
@@ -1576,17 +1584,19 @@ ice_dwnld_cfg_bufs(struct ice_hw *hw, struct ice_buf *bufs, u32 count)
 	 * considered a success.
 	 */
 	bh = (struct ice_buf_hdr *)bufs;
-	if (le32_to_cpu(bh->section_entry[0].type) & ICE_METADATA_BUF)
+	if (ice_is_buffer_metadata(bh))
 		return ICE_DDP_PKG_SUCCESS;
 
 	status = ice_acquire_global_cfg_lock(hw, ICE_RES_WRITE);
 	if (status) {
 		if (status == -EALREADY)
 			return ICE_DDP_PKG_ALREADY_LOADED;
 		return ice_map_aq_err_to_ddp_state(hw->adminq.sq_last_status);
 	}
 
-	state = ice_dwnld_cfg_bufs_no_lock(hw, bufs, 0, count, true);
+	ice_dwnld_cfg_bufs_no_lock(&ctx, bufs, 0, count);
+	/* finish up by sending last hunk with "last" flag set */
+	state = ice_ddp_send_hunk(&ctx, NULL);
 	if (!state)
 		state = ice_post_dwnld_pkg_actions(hw);
 
-- 
2.46.0

RE: [Intel-wired-lan] [PATCH iwl-next v2 1/2] ice: refactor "last" segment of DDP pkg

[Pucha, HimasekharX Reddy]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Przemek Kitszel
> Sent: Thursday, October 3, 2024 5:41 AM
> To: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: Nowlin, Dan <dan.nowlin@intel.com>; Greenwalt, Paul <paul.greenwalt@intel.com>; netdev@vger.kernel.org; Zaki, Ahmed <ahmed.zaki@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Subject: [Intel-wired-lan] [PATCH iwl-next v2 1/2] ice: refactor "last" segment of DDP pkg
>
> Add ice_ddp_send_hunk() that buffers "sent FW hunk" calls to AQ in order to mark the "last" one in more elegant way. Next commit will add even more complicated "sent FW" flow, so it's better to untangle a bit before.
>
> Note that metadata buffers were not skipped for NOT-@indicate_last segments, this is fixed now.
>
> Minor:
>  + use ice_is_buffer_metadata() instead of open coding it in
>    ice_dwnld_cfg_bufs();
>  + ice_dwnld_cfg_bufs_no_lock() + dependencies were moved up a bit to have
>    better git-diff, as this function was rewritten (in terms of git-blame)
>
> CC: Paul Greenwalt <paul.greenwalt@intel.com>
> CC: Dan Nowlin <dan.nowlin@intel.com>
> CC: Ahmed Zaki <ahmed.zaki@intel.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
> git: --inter-hunk-context=6
>
> v2: fixed one kdoc warning
> ---
>  drivers/net/ethernet/intel/ice/ice_ddp.c | 280 ++++++++++++-----------
>  1 file changed, 145 insertions(+), 135 deletions(-)
>

Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)

Re: [PATCH iwl-next v2 1/2] ice: refactor "last" segment of DDP pkg

[Simon Horman]

On Thu, Oct 03, 2024 at 02:10:31AM +0200, Przemek Kitszel wrote:
> Add ice_ddp_send_hunk() that buffers "sent FW hunk" calls to AQ in order
> to mark the "last" one in more elegant way. Next commit will add even
> more complicated "sent FW" flow, so it's better to untangle a bit before.
> 
> Note that metadata buffers were not skipped for NOT-@indicate_last
> segments, this is fixed now.
> 
> Minor:
>  + use ice_is_buffer_metadata() instead of open coding it in
>    ice_dwnld_cfg_bufs();
>  + ice_dwnld_cfg_bufs_no_lock() + dependencies were moved up a bit to have
>    better git-diff, as this function was rewritten (in terms of git-blame)
> 
> CC: Paul Greenwalt <paul.greenwalt@intel.com>
> CC: Dan Nowlin <dan.nowlin@intel.com>
> CC: Ahmed Zaki <ahmed.zaki@intel.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Hi Przemek,

Some minor feedback from my side.

> ---
> git: --inter-hunk-context=6
> 
> v2: fixed one kdoc warning
> ---
>  drivers/net/ethernet/intel/ice/ice_ddp.c | 280 ++++++++++++-----------
>  1 file changed, 145 insertions(+), 135 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
> index 016fcab6ba34..a2bb8442f281 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
> @@ -1210,6 +1210,127 @@ ice_aq_download_pkg(struct ice_hw *hw, struct ice_buf_hdr *pkg_buf,
>  	return status;
>  }
>  
> +/**
> + * ice_is_buffer_metadata - determine if package buffer is a metadata buffer
> + * @buf: pointer to buffer header
> + * Return: whether given @buf is a metadata one.
> + */
> +static bool ice_is_buffer_metadata(struct ice_buf_hdr *buf)
> +{
> +	return le32_to_cpu(buf->section_entry[0].type) & ICE_METADATA_BUF;

I see this is moving existing logic around.
And I see that this is a no-op on LE systems.
But it might be nicer to perform the byte-order conversion on the constant.

> +}
> +
> +/**
> + * struct ice_ddp_send_ctx - sending context of current DDP segment
> + * @hw: pointer to the hardware struct
> + *
> + * Keeps current sending state (header, error) for the purpose of proper "last"
> + * bit settting in ice_aq_download_pkg(). Use via calls to ice_ddp_send_hunk().

setting

> + */
> +struct ice_ddp_send_ctx {
> +	struct ice_hw *hw;
> +/* private: only for ice_ddp_send_hunk() */
> +	struct ice_buf_hdr *hdr;
> +	int err;
> +};
> +
> +/**
> + * ice_ddp_send_hunk - send one hunk of data to FW
> + * @ctx - current segment sending context
> + * @hunk - next hunk to send, size is always ICE_PKG_BUF_SIZE

Tooling seems to expect the following syntax.

 * @ctx: ...
 * @hunk: ...

> + *
> + * Send the next hunk of data to FW, retrying if needed.
> + *
> + * Notice: must be called once more with a NULL @hunk to finish up; such call
> + * will set up the "last" bit of an AQ request. After such call @ctx.hdr is
> + * cleared, @hw is still valid.
> + *
> + * Return: %ICE_DDP_PKG_SUCCESS if there were no problems; a sticky @err
> + *         otherwise.
> + */
> +static enum ice_ddp_state ice_ddp_send_hunk(struct ice_ddp_send_ctx *ctx,
> +					    struct ice_buf_hdr *hunk)

...

> +/**
> + * ice_dwnld_cfg_bufs_no_lock
> + * @ctx: context of the current buffers section to send
> + * @bufs: pointer to an array of buffers
> + * @start: buffer index of first buffer to download
> + * @count: the number of buffers to download
> + *
> + * Downloads package configuration buffers to the firmware. Metadata buffers
> + * are skipped, and the first metadata buffer found indicates that the rest
> + * of the buffers are all metadata buffers.
> + */
> +static enum ice_ddp_state
> +ice_dwnld_cfg_bufs_no_lock(struct ice_ddp_send_ctx *ctx, struct ice_buf *bufs,
> +			   u32 start, u32 count)
> +{
> +	struct ice_buf_hdr *bh;
> +	enum ice_ddp_state err;
> +
> +	if (!bufs || !count) {
> +		ctx->err = ICE_DDP_PKG_ERR;
> +		return ctx->err;
> +	}
> +
> +	bufs += start;
> +	bh = (struct ice_buf_hdr *)bufs;

Again I see that, to some extent, this is moving existing logic around.
But as bh is set in each loop iteration does it also need to be set here?

> +
> +	for (int i = 0; i < count; i++, bufs++) {
> +		bh = (struct ice_buf_hdr *)bufs;
> +		/* Metadata buffers should not be sent to FW,
> +		 * their presence means "we are done here".
> +		 */
> +		if (ice_is_buffer_metadata(bh))
> +			break;
> +
> +		err = ice_ddp_send_hunk(ctx, bh);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * ice_get_pkg_seg_by_idx
>   * @pkg_hdr: pointer to the package header to be searched

...

> @@ -1454,17 +1459,16 @@ ice_dwnld_sign_and_cfg_segs(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr,
>  	}
>  
>  	count = le32_to_cpu(seg->signed_buf_count);
> -	state = ice_download_pkg_sig_seg(hw, seg);
> +	state = ice_download_pkg_sig_seg(ctx, seg);
>  	if (state || !count)
>  		goto exit;
>  
>  	conf_idx = le32_to_cpu(seg->signed_seg_idx);
>  	start = le32_to_cpu(seg->signed_buf_start);
>  
> -	state = ice_download_pkg_config_seg(hw, pkg_hdr, conf_idx, start,
> -					    count);
> -
> +	return ice_download_pkg_config_seg(ctx, pkg_hdr, conf_idx, start, count);

This changes the conditions under which this function sets
ctx->err, which is then changed again by the following patch.
Is that intentional?

>  exit:
> +	ctx->err = state;
>  	return state;
>  }

...

Re: [PATCH iwl-next v2 1/2] ice: refactor "last" segment of DDP pkg

[Przemek Kitszel]

On 10/17/24 12:06, Simon Horman wrote:
> On Thu, Oct 03, 2024 at 02:10:31AM +0200, Przemek Kitszel wrote:
>> Add ice_ddp_send_hunk() that buffers "sent FW hunk" calls to AQ in order
>> to mark the "last" one in more elegant way. Next commit will add even
>> more complicated "sent FW" flow, so it's better to untangle a bit before.
>>
>> Note that metadata buffers were not skipped for NOT-@indicate_last
>> segments, this is fixed now.
>>
>> Minor:
>>   + use ice_is_buffer_metadata() instead of open coding it in
>>     ice_dwnld_cfg_bufs();
>>   + ice_dwnld_cfg_bufs_no_lock() + dependencies were moved up a bit to have
>>     better git-diff, as this function was rewritten (in terms of git-blame)
>>
>> CC: Paul Greenwalt <paul.greenwalt@intel.com>
>> CC: Dan Nowlin <dan.nowlin@intel.com>
>> CC: Ahmed Zaki <ahmed.zaki@intel.com>
>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 
> Hi Przemek,
> 
> Some minor feedback from my side.

Thank you for reaching out!

>> +static bool ice_is_buffer_metadata(struct ice_buf_hdr *buf)
>> +{
>> +	return le32_to_cpu(buf->section_entry[0].type) & ICE_METADATA_BUF;
> 
> I see this is moving existing logic around.
> And I see that this is a no-op on LE systems.
> But it might be nicer to perform the byte-order conversion on the constant.

As far as I remember, for this driver we always do have binary-arith
constants (flags, masks, etc) in CPU-order, so do as I did.

I could imagine keeping all such constants in HW-order, and such
approach could potentially set the boundary for byte-order conversions
to be better expressed/illustrated.

For new drivers, I will still think more about unit-test-abilty instead,
and those will be easiest with as much constants expressed in CPU-order.

No strong opinion here anyway, and I think we agree that it's most
important to be consistent within the driver/component. I manually
sampled that for ice, but I don't have a proof.

>> +/**
>> + * struct ice_ddp_send_ctx - sending context of current DDP segment
>> + * @hw: pointer to the hardware struct
>> + *
>> + * Keeps current sending state (header, error) for the purpose of proper "last"
>> + * bit settting in ice_aq_download_pkg(). Use via calls to ice_ddp_send_hunk().
> 
> setting
> 
>> + */
>> +struct ice_ddp_send_ctx {
>> +	struct ice_hw *hw;
>> +/* private: only for ice_ddp_send_hunk() */
>> +	struct ice_buf_hdr *hdr;
>> +	int err;
>> +};
>> +
>> +/**
>> + * ice_ddp_send_hunk - send one hunk of data to FW
>> + * @ctx - current segment sending context
>> + * @hunk - next hunk to send, size is always ICE_PKG_BUF_SIZE
> 
> Tooling seems to expect the following syntax.
> 
>   * @ctx: ...
>   * @hunk: ...

oops, sorry!

>> +static enum ice_ddp_state
>> +ice_dwnld_cfg_bufs_no_lock(struct ice_ddp_send_ctx *ctx, struct ice_buf *bufs,
>> +			   u32 start, u32 count)
>> +{
>> +	struct ice_buf_hdr *bh;
>> +	enum ice_ddp_state err;
>> +
>> +	if (!bufs || !count) {
>> +		ctx->err = ICE_DDP_PKG_ERR;
>> +		return ctx->err;
>> +	}
>> +
>> +	bufs += start;
>> +	bh = (struct ice_buf_hdr *)bufs;
> 
> Again I see that, to some extent, this is moving existing logic around.
> But as bh is set in each loop iteration does it also need to be set here?

good point
ice_is_buffer_metadata() check was prior to the loop, now it is inside,
I will remove the redundant assignment.


>> @@ -1454,17 +1459,16 @@ ice_dwnld_sign_and_cfg_segs(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr,
>>   	}
>>   
>>   	count = le32_to_cpu(seg->signed_buf_count);
>> -	state = ice_download_pkg_sig_seg(hw, seg);
>> +	state = ice_download_pkg_sig_seg(ctx, seg);
>>   	if (state || !count)
>>   		goto exit;
>>   
>>   	conf_idx = le32_to_cpu(seg->signed_seg_idx);
>>   	start = le32_to_cpu(seg->signed_buf_start);
>>   
>> -	state = ice_download_pkg_config_seg(hw, pkg_hdr, conf_idx, start,
>> -					    count);
>> -
>> +	return ice_download_pkg_config_seg(ctx, pkg_hdr, conf_idx, start, count);
> 
> This changes the conditions under which this function sets
> ctx->err, which is then changed again by the following patch.
> Is that intentional?
> 
>>   exit:
>> +	ctx->err = state;

This line is unusual as it changes ctx->err from ctx user code.
ctx itself updates @err only on new error, it uses "retained error"
style of API (that I'm clearly a fan of ;))

Next commit replaces the last (successful) write (via ctx) of ddp,
and error return from new path would result in
"ctx->err = ctx->err" update. Not clear, not intentional, not harmful.
I will update code to leave less space for confusion.

>>   	return state;
>>   }
> 
> ...
> 

Re: [PATCH iwl-next v2 1/2] ice: refactor "last" segment of DDP pkg

[Simon Horman]

On Fri, Oct 18, 2024 at 02:06:27PM +0200, Przemek Kitszel wrote:
> On 10/17/24 12:06, Simon Horman wrote:
> > On Thu, Oct 03, 2024 at 02:10:31AM +0200, Przemek Kitszel wrote:
> > > Add ice_ddp_send_hunk() that buffers "sent FW hunk" calls to AQ in order
> > > to mark the "last" one in more elegant way. Next commit will add even
> > > more complicated "sent FW" flow, so it's better to untangle a bit before.
> > > 
> > > Note that metadata buffers were not skipped for NOT-@indicate_last
> > > segments, this is fixed now.
> > > 
> > > Minor:
> > >   + use ice_is_buffer_metadata() instead of open coding it in
> > >     ice_dwnld_cfg_bufs();
> > >   + ice_dwnld_cfg_bufs_no_lock() + dependencies were moved up a bit to have
> > >     better git-diff, as this function was rewritten (in terms of git-blame)
> > > 
> > > CC: Paul Greenwalt <paul.greenwalt@intel.com>
> > > CC: Dan Nowlin <dan.nowlin@intel.com>
> > > CC: Ahmed Zaki <ahmed.zaki@intel.com>
> > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > 
> > Hi Przemek,
> > 
> > Some minor feedback from my side.
> 
> Thank you for reaching out!
> 
> > > +static bool ice_is_buffer_metadata(struct ice_buf_hdr *buf)
> > > +{
> > > +	return le32_to_cpu(buf->section_entry[0].type) & ICE_METADATA_BUF;
> > 
> > I see this is moving existing logic around.
> > And I see that this is a no-op on LE systems.
> > But it might be nicer to perform the byte-order conversion on the constant.
> 
> As far as I remember, for this driver we always do have binary-arith
> constants (flags, masks, etc) in CPU-order, so do as I did.
> 
> I could imagine keeping all such constants in HW-order, and such
> approach could potentially set the boundary for byte-order conversions
> to be better expressed/illustrated.
> 
> For new drivers, I will still think more about unit-test-abilty instead,
> and those will be easiest with as much constants expressed in CPU-order.
> 
> No strong opinion here anyway, and I think we agree that it's most
> important to be consistent within the driver/component. I manually
> sampled that for ice, but I don't have a proof.

Yes, we agree. And I also have no strong opinion on this.
So lets leave things as you have them.

...

> > > @@ -1454,17 +1459,16 @@ ice_dwnld_sign_and_cfg_segs(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr,
> > >   	}
> > >   	count = le32_to_cpu(seg->signed_buf_count);
> > > -	state = ice_download_pkg_sig_seg(hw, seg);
> > > +	state = ice_download_pkg_sig_seg(ctx, seg);
> > >   	if (state || !count)
> > >   		goto exit;
> > >   	conf_idx = le32_to_cpu(seg->signed_seg_idx);
> > >   	start = le32_to_cpu(seg->signed_buf_start);
> > > -	state = ice_download_pkg_config_seg(hw, pkg_hdr, conf_idx, start,
> > > -					    count);
> > > -
> > > +	return ice_download_pkg_config_seg(ctx, pkg_hdr, conf_idx, start, count);
> > 
> > This changes the conditions under which this function sets
> > ctx->err, which is then changed again by the following patch.
> > Is that intentional?
> > 
> > >   exit:
> > > +	ctx->err = state;
> 
> This line is unusual as it changes ctx->err from ctx user code.
> ctx itself updates @err only on new error, it uses "retained error"
> style of API (that I'm clearly a fan of ;))
> 
> Next commit replaces the last (successful) write (via ctx) of ddp,
> and error return from new path would result in
> "ctx->err = ctx->err" update. Not clear, not intentional, not harmful.
> I will update code to leave less space for confusion.

Thanks for the clarification, much appreciated.

...

[PATCH iwl-next v2 2/2] ice: support optional flags in signature segment header

[Przemek Kitszel]

An optional flag field has been added to the signature segment header.
The field contains two flags, a "valid" bit, and a "last segment" bit
that indicates whether the segment is the last segment that will be
sent to firmware.

If the flag field's valid bit is NOT set, then as was done before,
assume that this is the last segment being downloaded.

However, if the flag field's valid bit IS set, then use the last segment
flag to determine if this segment is the last segment to download.

Co-developed-by: Dan Nowlin <dan.nowlin@intel.com>
Signed-off-by: Dan Nowlin <dan.nowlin@intel.com>
Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
v2: co/- authorship change
---
 drivers/net/ethernet/intel/ice/ice_ddp.h |  5 ++++-
 drivers/net/ethernet/intel/ice/ice_ddp.c | 24 +++++++++++++++++-------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.h b/drivers/net/ethernet/intel/ice/ice_ddp.h
index 79551da2a4b0..8a2d57fc5dae 100644
--- a/drivers/net/ethernet/intel/ice/ice_ddp.h
+++ b/drivers/net/ethernet/intel/ice/ice_ddp.h
@@ -181,7 +181,10 @@ struct ice_sign_seg {
 	__le32 signed_seg_idx;
 	__le32 signed_buf_start;
 	__le32 signed_buf_count;
-#define ICE_SIGN_SEG_RESERVED_COUNT	44
+#define ICE_SIGN_SEG_FLAGS_VALID	0x80000000
+#define ICE_SIGN_SEG_FLAGS_LAST		0x00000001
+	__le32 flags;
+#define ICE_SIGN_SEG_RESERVED_COUNT	40
 	u8 reserved[ICE_SIGN_SEG_RESERVED_COUNT];
 	struct ice_buf_table buf_tbl;
 };
diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
index a2bb8442f281..859009940af0 100644
--- a/drivers/net/ethernet/intel/ice/ice_ddp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
@@ -1434,6 +1434,12 @@ ice_download_pkg_config_seg(struct ice_ddp_send_ctx *ctx,
 	return ice_dwnld_cfg_bufs_no_lock(ctx, bufs->buf_array, start, count);
 }
 
+static bool ice_is_last_sign_seg(u32 flags)
+{
+	return !(flags & ICE_SIGN_SEG_FLAGS_VALID) /* behavior prior to valid */
+	       || (flags & ICE_SIGN_SEG_FLAGS_LAST);
+}
+
 /**
  * ice_dwnld_sign_and_cfg_segs - download a signing segment and config segment
  * @ctx: context of the current buffers section to send
@@ -1446,11 +1452,9 @@ static enum ice_ddp_state
 ice_dwnld_sign_and_cfg_segs(struct ice_ddp_send_ctx *ctx,
 			    struct ice_pkg_hdr *pkg_hdr, u32 idx)
 {
+	u32 conf_idx, start, count, flags;
 	enum ice_ddp_state state;
 	struct ice_sign_seg *seg;
-	u32 conf_idx;
-	u32 start;
-	u32 count;
 
 	seg = (struct ice_sign_seg *)ice_get_pkg_seg_by_idx(pkg_hdr, idx);
 	if (!seg) {
@@ -1466,7 +1470,15 @@ ice_dwnld_sign_and_cfg_segs(struct ice_ddp_send_ctx *ctx,
 	conf_idx = le32_to_cpu(seg->signed_seg_idx);
 	start = le32_to_cpu(seg->signed_buf_start);
 
-	return ice_download_pkg_config_seg(ctx, pkg_hdr, conf_idx, start, count);
+	state = ice_download_pkg_config_seg(ctx, pkg_hdr, conf_idx, start,
+					    count);
+
+	/* finish up by sending last hunk with "last" flag set if requested by
+	 * DDP content */
+	flags = le32_to_cpu(seg->flags);
+	if (ice_is_last_sign_seg(flags))
+		state = ice_ddp_send_hunk(ctx, NULL);
+
 exit:
 	ctx->err = state;
 	return state;
@@ -1544,9 +1556,7 @@ ice_download_pkg_with_sig_seg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr)
 						    hw->pkg_sign_type))
 			continue;
 
-		ice_dwnld_sign_and_cfg_segs(&ctx, pkg_hdr, i);
-		/* finish up by sending last hunk with "last" flag set */
-		state = ice_ddp_send_hunk(&ctx, NULL);
+		state = ice_dwnld_sign_and_cfg_segs(&ctx, pkg_hdr, i);
 		if (state)
 			break;
 	}
-- 
2.46.0

RE: [Intel-wired-lan] [PATCH iwl-next v2 2/2] ice: support optional flags in signature segment header

[Pucha, HimasekharX Reddy]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Przemek Kitszel
> Sent: Thursday, October 3, 2024 5:41 AM
> To: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: Nowlin, Dan <dan.nowlin@intel.com>; Greenwalt, Paul <paul.greenwalt@intel.com>; netdev@vger.kernel.org; Zaki, Ahmed <ahmed.zaki@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Subject: [Intel-wired-lan] [PATCH iwl-next v2 2/2] ice: support optional flags in signature segment header
>
> An optional flag field has been added to the signature segment header.
> The field contains two flags, a "valid" bit, and a "last segment" bit that indicates whether the segment is the last segment that will be sent to firmware.
>
> If the flag field's valid bit is NOT set, then as was done before, assume that this is the last segment being downloaded.
>
> However, if the flag field's valid bit IS set, then use the last segment flag to determine if this segment is the last segment to download.
>
> Co-developed-by: Dan Nowlin <dan.nowlin@intel.com>
> Signed-off-by: Dan Nowlin <dan.nowlin@intel.com>
> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
> v2: co/- authorship change
> ---
>  drivers/net/ethernet/intel/ice/ice_ddp.h |  5 ++++-  drivers/net/ethernet/intel/ice/ice_ddp.c | 24 +++++++++++++++++-------
>  2 files changed, 21 insertions(+), 8 deletions(-)
>

Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)