Re: [Intel-wired-lan] [PATCH iwl-next] ice: clean up __ice_aq_get_set_rss_lut()

From: Alexander Lobakin <aleksander.lobakin@intel.com>
To: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Cc: netdev@vger.kernel.org,
	Jesse Brandeburg <jesse.brandeburg@intel.com>,
	Tony Nguyen <anthony.l.nguyen@intel.com>,
	intel-wired-lan@lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH iwl-next] ice: clean up __ice_aq_get_set_rss_lut()
Date: Tue, 6 Jun 2023 15:31:08 +0200	[thread overview]
Message-ID: <1bd64ee9-4004-ddc8-6bff-6d8fc75c30c1@intel.com> (raw)
In-Reply-To: <20230606111149.33890-1-przemyslaw.kitszel@intel.com>

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Tue, 6 Jun 2023 13:11:49 +0200

> Refactor __ice_aq_get_set_rss_lut() to improve reader experience and limit
> misuse scenarios (undesired LUT size for given LUT type).

[...]

<related block begin>

> +	opcode = set ? ice_aqc_opc_set_rss_lut : ice_aqc_opc_get_rss_lut;
> +	ice_fill_dflt_direct_cmd_desc(&desc, opcode);
> +	if (set)
> +		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
>  
> -	lut_size = params->lut_size;
> -	lut_type = params->lut_type;
> -	glob_lut_idx = params->global_lut_id;
> +	desc_params = &desc.params.get_set_rss_lut;
>  	vsi_id = ice_get_hw_vsi_num(hw, vsi_handle);
> +	desc_params->vsi_id = cpu_to_le16(vsi_id | ICE_AQC_RSS_VSI_VALID);
>  
> -	cmd_resp = &desc.params.get_set_rss_lut;
> +	if (lut_type == ICE_LUT_GLOBAL)
> +		glob_lut_idx = FIELD_PREP(ICE_AQC_LUT_GLOBAL_IDX,
> +					  params->global_lut_id);
>  
> -	if (set) {
> -		ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_set_rss_lut);
> -		desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> -	} else {
> -		ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_rss_lut);
> -	}
> +	flags = lut_type | glob_lut_idx | ice_lut_size_to_flag(lut_size);
> +	desc_params->flags = cpu_to_le16(flags);

</related block end>

Now the tricky part much harder to discover than FIELD_*(): you have

{u,__be,__le}{8,16,32,64}_{get,encode,replace}_bits()

macros for such cases :D
They aren't described in any kdoc and the indexers are not able to index
them (clangd however can IIRC), you even need some brain processing in
order to realize they exist. See [0].

I'm not saying they're perfectly applicable for this particular case,
just FYI.

>  
> -	cmd_resp->vsi_id = cpu_to_le16(((vsi_id <<
> -					 ICE_AQC_GSET_RSS_LUT_VSI_ID_S) &
> -					ICE_AQC_GSET_RSS_LUT_VSI_ID_M) |
> -				       ICE_AQC_GSET_RSS_LUT_VSI_VALID);

[...]

[0]
https://elixir.bootlin.com/linux/v6.4-rc5/source/include/linux/bitfield.h#L174

Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan