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

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

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