> -----Original Message-----
> From: Kitszel, Przemyslaw <przemyslaw.kits...@intel.com>
> Sent: Wednesday, August 28, 2024 12:35 AM
> To: Keller, Jacob E <jacob.e.kel...@intel.com>
> Cc: Intel Wired LAN <intel-wired-...@lists.osuosl.org>; Vladimir Oltean
> <olte...@gmail.com>; Nguyen, Anthony L <anthony.l.ngu...@intel.com>
> Subject: Re: [PATCH iwl-next 13/13] ice: cleanup Rx queue context programming
> functions
>
> On 8/27/24 23:52, Jacob Keller wrote:
> > The ice_copy_rxq_ctx_to_hw() and ice_write_rxq_ctx() functions perform some
> > defensive checks which are typically frowned upon by kernel style
> > guidelines.
> >
> > In particular, they perform NULL checks on buffers which are never expected
> > to be NULL. Both functions are only called once and always have valid
> > buffers pointing to stack memory. These checks only serve to hide potential
> > programming error, as we will not produce the normal crash dump on a NULL
> > access.
> >
> > In addition, ice_copy_rxq_ctx_to_hw() cannot fail in another way, so could
> > be made void.
> >
> > Future support for VF Live Migration will need to introduce an inverse
> > function for reading Rx queue context from HW registers to unpack it, as
> > well as functions to pack and unpack Tx queue context from HW.
> >
> > Rather than copying these style issues into the new functions, lets first
> > cleanup the existing code.
> >
> > For the ice_copy_rxq_ctx_to_hw() function:
> >
> > * Remove the NULL parameter check.
> > * Move the Rx queue index check out of this function.
> > * Convert the function to a void return.
> > * Use a simple int variable instead of a u8 for the for loop index.
> > * Use a local variable and array syntax to access the rxq_ctx.
> > * Update the function description to better align with kernel doc style.
> >
> > For the ice_write_rxq_ctx() function:
> >
> > * Use the more common '= {}' syntax for initializing the context buffer.
> > * Move the Rx queue index check into this function.
> > * Update the function description with a Returns: to align with kernel doc
> > style.
> >
> > These changes align the existing write functions to current kernel
> > style, and will align with the style of the new functions added when we
> > implement live migration in a future series.
> >
>
> Reviewed-by: Przemek Kitszel <przemyslaw.kits...@intel.com>
>
> > Signed-off-by: Jacob Keller <jacob.e.kel...@intel.com>
> > ---
> > drivers/net/ethernet/intel/ice/ice_common.c | 42
> > ++++++++++++-----------------
> > 1 file changed, 17 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
> b/drivers/net/ethernet/intel/ice/ice_common.c
> > index 67273e4af7ff..3480a16d6452 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_common.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> > @@ -1357,34 +1357,22 @@ int ice_reset(struct ice_hw *hw, enum ice_reset_req
> req)
> > }
> >
> > /**
> > - * ice_copy_rxq_ctx_to_hw
> > + * ice_copy_rxq_ctx_to_hw - Copy packed Rx queue context to HW registers
> > * @hw: pointer to the hardware structure
> > - * @ice_rxq_ctx: pointer to the rxq context
>
> "ice" prefix for params, yeah :D
>
Yea, that seemed unnecessary!
> > + * @rxq_ctx: pointer to the packed Rx queue context
> > * @rxq_index: the index of the Rx queue
> > - *
> > - * Copies rxq context from dense structure to HW register space
> > */
> > -static int
> > -ice_copy_rxq_ctx_to_hw(struct ice_hw *hw, u8 *ice_rxq_ctx, u32 rxq_index)
> > +static void ice_copy_rxq_ctx_to_hw(struct ice_hw *hw, u8 *rxq_ctx,
> > + u32 rxq_index)
> > {
> > - u8 i;
> > -
> > - if (!ice_rxq_ctx)
> > - return -EINVAL;
> > -
> > - if (rxq_index > QRX_CTRL_MAX_INDEX)
> > - return -EINVAL;
> > -
> > /* Copy each dword separately to HW */
> > - for (i = 0; i < ICE_RXQ_CTX_SIZE_DWORDS; i++) {
> > - wr32(hw, QRX_CONTEXT(i, rxq_index),
> > - *((u32 *)(ice_rxq_ctx + (i * sizeof(u32)))));
> > + for (int i = 0; i < ICE_RXQ_CTX_SIZE_DWORDS; i++) {
> > + u32 ctx = ((u32 *)rxq_ctx)[i];
> >
> > - ice_debug(hw, ICE_DBG_QCTX, "qrxdata[%d]: %08X\n", i,
> > - *((u32 *)(ice_rxq_ctx + (i * sizeof(u32)))));
> > + wr32(hw, QRX_CONTEXT(i, rxq_index), ctx);
> > +
> > + ice_debug(hw, ICE_DBG_QCTX, "qrxdata[%d]: %08X\n", i, ctx);
> > }
> > -
> > - return 0;
> > }
> >
> > /**
> > @@ -1497,23 +1485,27 @@ const struct ice_ctx_ele ice_rlan_ctx_info[] = {
> > /**
> > * ice_write_rxq_ctx - Write Rx Queue context to hardware
> > * @hw: pointer to the hardware structure
> > - * @rlan_ctx: pointer to the rxq context
> > + * @rlan_ctx: pointer to the packed Rx queue context
> > * @rxq_index: the index of the Rx queue
> > *
> > * Pack the sparse Rx Queue context into dense hardware format and write
> > it
> > * into the HW register space.
> > + *
> > + * Returns: 0 on success, or -EINVAL if the Rx queue index is invalid.
>
> I remember some bot complaining on plural Returns:, but I have just
> checked the kernel-doc script and it is allowed :)
>
It is strictly allowed, but I will fix. I want to be consistent.
> > */
> > int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx,
> > u32 rxq_index)
> > {
> > - u8 ctx_buf[ICE_RXQ_CTX_SZ] = { 0 };
> > + u8 ctx_buf[ICE_RXQ_CTX_SZ] = {};
> >
> > - if (!rlan_ctx)
> > + if (rxq_index > QRX_CTRL_MAX_INDEX)
> > return -EINVAL;
> >
> > ice_pack_rxq_ctx(rlan_ctx, ctx_buf);
> >
> > - return ice_copy_rxq_ctx_to_hw(hw, ctx_buf, rxq_index);
> > + ice_copy_rxq_ctx_to_hw(hw, ctx_buf, rxq_index);
> > +
> > + return 0;
> > }
> >
> > /* LAN Tx Queue Context */
> >
