netdev - Re: [Intel-wired-lan] [PATCH iwl-next v1] idpf: refactor some missing field get/prep conversions

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, 5 Dec 2023 17:10:58 -0800
From: Jesse Brandeburg <jesse.brandeburg@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<anthony.l.nguyen@...el.com>, Julia Lawall <julia.lawall@...ia.fr>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1] idpf: refactor some missing
 field get/prep conversions

On 12/4/2023 2:26 AM, Alexander Lobakin wrote:
yping.
> 
> What is "not a constant"?
> 
> 	ring[nta] = FIELD_PREP(IDPF_RX_BI_GEN_M,
> 			       test_bit(__IDPF_Q_GEN_CHK, flags));
> 
> Is there a problem with this ^ code?
> 
> "The scripts ignored that" is not a good argument I'd say :>

Fixed in v2


>> Generally I'd prefer that the kind of check above returned a bool with a
>> constant conversion of the mask (compile time) to an LE16 mask, and then
>> use that, which is why all of our other drivers do that instead.
> 
> Ah, good point. Smth like
> 
> 		gen = !!(tx_desc->qid_comptype_gen &
> 			 IDPF_TXQ_COMPLQ_GEN_M_LE);

Yeah, it would be nice but I didn't add that to this series. But you
have the idea.

> 
> OTOH x86 is always LE and BE is seen more and more rarely nowadays. It
> might just not worth having a LE-version of each such mask for the sake
> of a bit more optimized code on architectures where our drivers are
> barely used.

Our drivers should work on BE, IMO. sparse annotations takes care of
making sure we have the conversions right, they cost nothing on x86.

> 
>>
>>>
>>> Moreover, you could use le*_{get,encode,replace}_bits() to get/set LE
>>> values right away without 2-step operation (from/to CPU + masks), but
>>> you didn't do that here (see below for an example).

Done in v2.

>>
>> Those aren't widely used yet in our drivers so I wasn't picking them up
>> yet. But thank you for pointing that out.
>>
>> <snip>
>>
>>
>>> In general, I would say those two issues are very common in IDPF and
>>> also the whole your series converting the Intel drivers. The scripts
>>> won't check whether the mask has only 1 bit or whether the value gets
>>> converted from/to LE, so they won't help here.
>>
>> I had been hoping to do some more followup work. it's possible that with
>> some tweaking the coccinelle script could learn how to detect non-pow2
>> constants, and therefore possibly one bit constants as well. Maybe
>> @Julia can help us refine the script and possibly get it into the
>> scripts/coccinelle directory to help other drivers as well.
> 
> Every automated change needs polishing by human.
> 
> Don't FIELD_*() macros already check whether the passed mask is actually
> a contiguous mask?

Yes

> 
>>
>>> Could you maybe manually recheck all the places where bitfield masks are
>>> used at least in IDPF (better in ice, iavf, i40e, ..., as well) and
>>> posted a series that would address them? At the end, manual work is more
>>> valuable than automated conversions :p
>>
>> I think a followup series would work better for this, do you agree?
> 
> Nope :D If you want to convert all of our drivers to use bitfield API,
> I'd like to see a complete mature series instead of incremental series
> of series where followups will refactor the code introduced in the
> earlier ones.

v2 of the previous series sent with audit of all *cpu* conversions and
fixing up leXX_*_bits() opportunities.

Appreciate the reviews!