On Tue, Sep 05, 2023 at 06:53:37PM +0200, Larysa Zaremba wrote:

On Tue, Sep 05, 2023 at 05:37:27PM +0200, Maciej Fijalkowski wrote:

quoted

On Mon, Sep 04, 2023 at 08:01:06PM +0200, Larysa Zaremba wrote:

quoted

On Mon, Sep 04, 2023 at 05:02:40PM +0200, Maciej Fijalkowski wrote:

quoted

On Thu, Aug 24, 2023 at 09:26:42PM +0200, Larysa Zaremba wrote:

quoted

Previously, we only needed RX checksum flags in skb path,
hence all related code was written with skb in mind.
But with the addition of XDP hints via kfuncs to the ice driver,
the same logic will be needed in .xmo_() callbacks.

Put generic process of determining checksum status into
a separate function.

Now we cannot operate directly on skb, when deducing
checksum status, therefore introduce an intermediate enum for checksum
status. Fortunately, in ice, we have only 4 possibilities: checksum
validated at level 0, validated at level 1, no checksum, checksum error.
Use 3 bits for more convenient conversion.

Signed-off-by: Larysa Zaremba <redacted>
---
 drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 105 ++++++++++++------
 1 file changed, 69 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
index b2f241b73934..8b155a502b3b 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c

@@ -102,18 +102,41 @@ ice_rx_hash_to_skb(const struct ice_rx_ring *rx_ring,
 		skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype));
 }
 
+enum ice_rx_csum_status {
+	ICE_RX_CSUM_LVL_0	= 0,
+	ICE_RX_CSUM_LVL_1	= BIT(0),
+	ICE_RX_CSUM_NONE	= BIT(1),
+	ICE_RX_CSUM_ERROR	= BIT(2),
+	ICE_RX_CSUM_FAIL	= ICE_RX_CSUM_NONE | ICE_RX_CSUM_ERROR,
+};
+
 /**
- * ice_rx_csum - Indicate in skb if checksum is good
- * @ring: the ring we care about
- * @skb: skb currently being received and modified
+ * ice_rx_csum_lvl - Get checksum level from status
+ * @status: driver-specific checksum status

nit: describe retval?

I think that kernel-doc is already too much for a one-liner.
Also, checksum level is fully explained in sk_buff documentation.

quoted

quoted

quoted

quoted

+ */
+static u8 ice_rx_csum_lvl(enum ice_rx_csum_status status)
+{
+	return status & ICE_RX_CSUM_LVL_1;
+}
+
+/**
+ * ice_rx_csum_ip_summed - Checksum status from driver-specific to generic
+ * @status: driver-specific checksum status

ditto

Same as above. Moreover, there are only 2 possible return values that anyone can 
easily look up. Describing them here would only balloon the file length.

You really think 5 additional lines would balloon the file length? :D

I am not sure what to say here. We have many pretty pointless kdoc retval
descriptions like 'returns 0 on success, error otherwise' but to me this
is following the guidelines from Documentation/doc-guide/kernel-doc.rst.
If i generate kdoc I don't want to open up the source code to easily look
up retvals.

Just my 0.02$, not a thing that I'd like to keep on arguing on :)

quoted

quoted

quoted

quoted

+ */
+static u8 ice_rx_csum_ip_summed(enum ice_rx_csum_status status)
+{
+	return status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY;

	return !(status & ICE_RX_CSUM_NONE);

?

status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY;

is immediately understandable and results in 3 asm operations (I have checked):

result = status >> 1;
result ^= 1;
result &= 1;

I do not think "!(status & ICE_RX_CSUM_NONE);" could produce less.

oh, nice. Just the fact that branch being added caught my eye.

(...)