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: Fri, 1 Dec 2023 15:32:51 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jesse Brandeburg <jesse.brandeburg@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<anthony.l.nguyen@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1] idpf: refactor some missing
 field get/prep conversions

From: Jesse Brandeburg <jesse.brandeburg@...el.com>
Date: Thu, 30 Nov 2023 13:45:11 -0800

> Most of idpf correctly uses FIELD_GET and FIELD_PREP, but a couple spots
> were missed so fix those.
> 
> This conversion was automated via a coccinelle script as posted with the
> previous series.
> 
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> ---
> This patch should be applied after the larger FIELD_PREP/FIELD_GET
> conversion series for the Intel drivers.
> ---
>  .../ethernet/intel/idpf/idpf_singleq_txrx.c    |  7 +++----
>  drivers/net/ethernet/intel/idpf/idpf_txrx.c    | 18 ++++++++----------
>  2 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> index 81288a17da2a..447753495c53 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> @@ -328,10 +328,9 @@ static void idpf_tx_singleq_build_ctx_desc(struct idpf_queue *txq,
>  
>  	if (offload->tso_segs) {
>  		qw1 |= IDPF_TX_CTX_DESC_TSO << IDPF_TXD_CTX_QW1_CMD_S;
> -		qw1 |= ((u64)offload->tso_len << IDPF_TXD_CTX_QW1_TSO_LEN_S) &
> -			IDPF_TXD_CTX_QW1_TSO_LEN_M;
> -		qw1 |= ((u64)offload->mss << IDPF_TXD_CTX_QW1_MSS_S) &
> -			IDPF_TXD_CTX_QW1_MSS_M;
> +		qw1 |= FIELD_PREP(IDPF_TXD_CTX_QW1_TSO_LEN_M,
> +				  offload->tso_len);
> +		qw1 |= FIELD_PREP(IDPF_TXD_CTX_QW1_MSS_M, offload->mss);
>  
>  		u64_stats_update_begin(&txq->stats_sync);
>  		u64_stats_inc(&txq->q_stats.tx.lso_pkts);
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> index 1f728a9004d9..f3009d2a3c2e 100644
> --- 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?

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

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

is enough.

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


>  		if (test_bit(__IDPF_Q_GEN_CHK, complq->flags) != gen)
>  			break;
>  
>  		/* Find necessary info of TX queue to clean buffers */
> -		rel_tx_qid = (le16_to_cpu(tx_desc->qid_comptype_gen) &
> -			 IDPF_TXD_COMPLQ_QID_M) >> IDPF_TXD_COMPLQ_QID_S;
> +		rel_tx_qid = FIELD_GET(IDPF_TXD_COMPLQ_QID_M,
> +				       le16_to_cpu(tx_desc->qid_comptype_gen));

		rel_tx_qid = le16_get_bits(tx_desc->qid_comptype_gen,
					   IDPF_TXD_COMPLQ_QID_M);

>  		if (rel_tx_qid >= complq->txq_grp->num_txq ||
>  		    !complq->txq_grp->txqs[rel_tx_qid]) {
>  			dev_err(&complq->vport->adapter->pdev->dev,
> @@ -1842,9 +1842,8 @@ static bool idpf_tx_clean_complq(struct idpf_queue *complq, int budget,
>  		tx_q = complq->txq_grp->txqs[rel_tx_qid];
>  
>  		/* Determine completion type */
> -		ctype = (le16_to_cpu(tx_desc->qid_comptype_gen) &
> -			IDPF_TXD_COMPLQ_COMPL_TYPE_M) >>
> -			IDPF_TXD_COMPLQ_COMPL_TYPE_S;
> +		ctype = FIELD_GET(IDPF_TXD_COMPLQ_COMPL_TYPE_M,
> +				  le16_to_cpu(tx_desc->qid_comptype_gen));

Same.

>  		switch (ctype) {
>  		case IDPF_TXD_COMPLT_RE:
>  			hw_head = le16_to_cpu(tx_desc->q_head_compl_tag.q_head);
> @@ -1947,8 +1946,7 @@ void idpf_tx_splitq_build_ctb(union idpf_tx_flex_desc *desc,
>  	desc->q.qw1.cmd_dtype =
>  		cpu_to_le16(params->dtype & IDPF_FLEX_TXD_QW1_DTYPE_M);
>  	desc->q.qw1.cmd_dtype |=
> -		cpu_to_le16((td_cmd << IDPF_FLEX_TXD_QW1_CMD_S) &
> -			    IDPF_FLEX_TXD_QW1_CMD_M);
> +		cpu_to_le16(FIELD_PREP(IDPF_FLEX_TXD_QW1_CMD_M, td_cmd));

Same.

>  	desc->q.qw1.buf_size = cpu_to_le16((u16)size);
>  	desc->q.qw1.l2tags.l2tag1 = cpu_to_le16(params->td_tag);
>  }

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

Thanks,
Olek