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.greenw...@intel.com>
> CC: Dan Nowlin <dan.now...@intel.com>
> CC: Ahmed Zaki <ahmed.z...@intel.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkow...@linux.intel.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kits...@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;
> }
...
