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

[Simon Horman <horms@kernel.org>]

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;
>  }

...