From: Jesse Brandeburg <[email protected]> Date: Fri, 1 Dec 2023 12:12:05 -0800
> On 12/1/2023 6:32 AM, Alexander Lobakin wrote: >> From: Jesse Brandeburg <[email protected]> > >>> --- 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. 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 :> > >> >>> >>> @@ -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. Ah, good point. Smth like gen = !!(tx_desc->qid_comptype_gen & IDPF_TXQ_COMPLQ_GEN_M_LE); 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. > >> >> 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. Every automated change needs polishing by human. Don't FIELD_*() macros already check whether the passed mask is actually a contiguous mask? > >> 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. > > Thanks, > Jesse Thanks, Olek _______________________________________________ Intel-wired-lan mailing list [email protected] https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
