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]

Message-ID: <97ed09be-cb2b-48c3-846d-7a0e453ef816@intel.com>
Date: Fri, 1 Dec 2023 12:12:05 -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/1/2023 6:32 AM, Alexander Lobakin wrote:
> From: Jesse Brandeburg <jesse.brandeburg@...el.com>

>> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
>> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
>> @@ -505,7 +505,7 @@ static void idpf_rx_post_buf_refill(struct idpf_sw_queue *refillq, u16 buf_id)
>>  
>>  	/* store the buffer ID and the SW maintained GEN bit to the refillq */
>>  	refillq->ring[nta] =
>> -		((buf_id << IDPF_RX_BI_BUFID_S) & IDPF_RX_BI_BUFID_M) |
>> +		FIELD_PREP(IDPF_RX_BI_BUFID_M, buf_id) |
>>  		(!!(test_bit(__IDPF_Q_GEN_CHK, refillq->flags)) <<
>>  		 IDPF_RX_BI_GEN_S);
> 
> Why isn't that one converted as well?

Because it's not a constant, and it's not checking a mask with "&", so
the automation ignored it. I *did* a test, and we could convert the
return value from test_bit (a bool) into the IDPF_RX_BI_GEN_M mask with
FIELD_PREP, since C-code allows the luxury of converting a bool to a
"1", even though it's a bit type ugly in this age of strict typing.

> 
>>  
>> @@ -1825,14 +1825,14 @@ static bool idpf_tx_clean_complq(struct idpf_queue *complq, int budget,
>>  		u16 gen;
>>  
>>  		/* if the descriptor isn't done, no work yet to do */
>> -		gen = (le16_to_cpu(tx_desc->qid_comptype_gen) &
>> -		      IDPF_TXD_COMPLQ_GEN_M) >> IDPF_TXD_COMPLQ_GEN_S;
>> +		gen = FIELD_GET(IDPF_TXD_COMPLQ_GEN_M,
>> +				le16_to_cpu(tx_desc->qid_comptype_gen));
> 
> The definition:
> 
> #define IDPF_TXD_COMPLQ_GEN_M		BIT_ULL(IDPF_TXD_COMPLQ_GEN_S)
> 
> Please don't use FIELD_*() API for 1 bit.

Did you mean that gen is effectively used as a bool? I think that has
nothing to do with my change over to FIELD_GET, but I could see how
redesigning this code would be useful, but not as part of this
conversion series.

> 
> 		gen = !!(le16_to_cpu(tx_desc->qid_comptype_gen) &
> 			 IDPF_TXD_COMPLQ_GEN_M);
> 
> is enough.

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.

> 
> 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).

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.

> 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?

Thanks,
Jesse