netdev - Re: [PATCH iwl-next] ice: clean up __ice_aq_get_set_rss

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 6 Jun 2023 15:31:08 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, Jesse Brandeburg
	<jesse.brandeburg@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	Anirudh Venkataramanan <anirudh.venkataramanan@...el.com>, "Sudheer
 Mogilappagari" <sudheer.mogilappagari@...el.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH iwl-next] ice: clean up __ice_aq_get_set_rss_lut()

From: Przemek Kitszel <przemyslaw.kitszel@...el.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