[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <eff20fb6-d2d3-4b12-8d46-1cd891ad0156@intel.com>
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!
Powered by blists - more mailing lists