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