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