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: Wed, 7 Jun 2023 14:56:44 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...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()

On 6/6/23 15:31, Alexander Lobakin wrote:
> 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

Thank you for the mention :) I will keep that in mind, but for now I 
will leave the code with cpu_to_le16(), as the scope of changes is 
reaching single-patch size limits ;P

PK