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

[Simon Horman <horms@kernel.org>]

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.

...