Re: [Intel-wired-lan] [PATCH iwl-next 12/13] ice: move prefetch enable to ice_setup_rx_ctx

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

On 8/27/24 23:52, Jacob Keller wrote:
> The ice_write_rxq_ctx() function is responsible for programming the Rx
> Queue context into hardware. It receives the configuration in unpacked form
> via the ice_rlan_ctx structure.
> 
> This function unconditionally modifies the context to set the prefetch
> enable bit. This was done by commit c31a5c25bb19 ("ice: Always set prefena
> when configuring an Rx queue"). Setting this bit makes sense, since
> prefetching descriptors is almost always the preferred behavior.
> 
> However, the ice_write_rxq_ctx() function is not the place that actually
> defines the queue context. We initialize the Rx Queue context in
> ice_setup_rx_ctx(). It is surprising to have the Rx queue context changed
> by a function who's responsibility is to program the given context to
> hardware.
> 
> Following the principle of least surprise, move the setting of the prefetch
> enable bit out of ice_write_rxq_ctx() and into the ice_setup_rx_ctx().
> 
> Fixes: c31a5c25bb19 ("ice: Always set prefena when configuring an Rx queue")

The code is fine, but I would drop fixes tag.
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_base.c   | 3 +++
>   drivers/net/ethernet/intel/ice/ice_common.c | 9 +++------
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> index 1881ce8105ca..3fe87a30c29e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_base.c
> +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> @@ -453,6 +453,9 @@ static int ice_setup_rx_ctx(struct ice_rx_ring *ring)
>   	/* Rx queue threshold in units of 64 */
>   	rlan_ctx.lrxqthresh = 1;
>   
> +	/* Enable descriptor prefetch */
> +	rlan_ctx.prefena = 1;
> +
>   	/* PF acts as uplink for switchdev; set flex descriptor with src_vsi
>   	 * metadata and flags to allow redirecting to PR netdev
>   	 */
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
> index 09a94c20e16d..67273e4af7ff 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -1495,14 +1495,13 @@ const struct ice_ctx_ele ice_rlan_ctx_info[] = {
>   };
>   
>   /**
> - * ice_write_rxq_ctx
> + * ice_write_rxq_ctx - Write Rx Queue context to hardware
>    * @hw: pointer to the hardware structure
>    * @rlan_ctx: pointer to the rxq context
>    * @rxq_index: the index of the Rx queue
>    *
> - * Converts rxq context from sparse to dense structure and then writes
> - * it to HW register space and enables the hardware to prefetch descriptors
> - * instead of only fetching them on demand
> + * Pack the sparse Rx Queue context into dense hardware format and write it
> + * into the HW register space.
>    */
>   int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx,
>   		      u32 rxq_index)
> @@ -1512,8 +1511,6 @@ int ice_write_rxq_ctx(struct ice_hw *hw, struct ice_rlan_ctx *rlan_ctx,
>   	if (!rlan_ctx)
>   		return -EINVAL;
>   
> -	rlan_ctx->prefena = 1;
> -
>   	ice_pack_rxq_ctx(rlan_ctx, ctx_buf);
>   
>   	return ice_copy_rxq_ctx_to_hw(hw, ctx_buf, rxq_index);
>