Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

On 09.10.2021 11:27, Biju Das wrote:

quoted

quoted

[...]

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Fillup ravb_rx_gbeth() function to support RZ/G2L.

This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
consistent with the naming convention used in sh_eth driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref][...]

diff --git a/drivers/net/ethernet/renesas/ravb_main.c

b/drivers/net/ethernet/renesas/ravb_main.c
index 37164a983156..42573eac82b9 100644

--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c

@@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct

net_device

*ndev)

quoted

  	}
  }

+static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
+	u8 *hw_csum;
+
+	/* The hardware checksum is contained in sizeof(__sum16) (2)

bytes

quoted

quoted

quoted

quoted

quoted

+	 * appended to packet data
+	 */
+	if (unlikely(skb->len < sizeof(__sum16)))
+		return;
+	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);

[...]

quoted

quoted

quoted

quoted

Please check the section 30.5.6.1 checksum calculation handling>
And figure 30.25 the field of checksum attaching field

    I have.

quoted

Also see Table 30.17 for checksum values for non-error conditions.

quoted

TCP/UDP/ICPM checksum is at last 2bytes.

    What are you arguing with then? :-)
    My point was that your code fetched the TCP/UDP/ICMP checksum
ISO the IP checksum because it subtracts sizeof(__sum16), while
should probably subtract sizeof(__wsum)

Agreed. My code missed IP4 checksum result. May be we need to
extract 2 checksum info from last 4 bytes.  First checksum(2bytes)
is IP4 header checksum and next checksum(2 bytes)  for TCP/UDP/ICMP
and use this info finding the non error case mentioned in  Table

30.17.

quoted

quoted

quoted

For eg:-
IPV6 non error-condition -->  "0xFFFF"-->IPV4HeaderCSum value and

"0x0000"

quoted

quoted

quoted

TCP/UDP/ICMP CSUM value

IPV4 non error-condition -->  "0x0000"-->IPV4HeaderCSum value and

"0x0000"

quoted

quoted

quoted

TCP/UDP/ICMP CSUM value

Do you agree?

quoted

What I meant here is some thing like below, please let me know if you
have any issues with this, otherwise I would like to send the patch

with below changes.

quoted

quoted

Further improvements can happen later.

Please let me know.

+/* Hardware checksum status */
+#define IPV4_RX_CSUM_OK                0x00000000
+#define IPV6_RX_CSUM_OK                0xFFFF0000

    Mhm, this should prolly come from the IP headers...

[...]

quoted

diff --git a/drivers/net/ethernet/renesas/ravb_main.c

b/drivers/net/ethernet/renesas/ravb_main.c
index bbb42e5328e4..d9201fbbd472 100644

--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c

@@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct

net_device *ndev)

  static void ravb_rx_csum_gbeth(struct sk_buff *skb)  {
-       u16 *hw_csum;
+       u32 csum_result;

    This is not against the patch currently under investigation. :-)

quoted

+       u8 *hw_csum;

         /* The hardware checksum is contained in sizeof(__sum16) (2)

bytes

quoted

quoted

          * appended to packet data
          */
-       if (unlikely(skb->len < sizeof(__sum16)))
+       if (unlikely(skb->len < sizeof(__wsum)))

    I think this usage of __wsum is valid (I remember that I suggested

it). We have 2 16-bit checksums here

    I meant "I don't think", of course. :-)

Ok will use 2 * sizeof(__sum16) instead and extract IPV4 header csum and TCP/UDP/ICMP csum result.

    I'm not sure how to deal with the later...

All error condition/unsupported cases will be passed to stack with CHECKSUM_NONE
and only non-error cases will be set as CHECKSUM_UNNCESSARY.

Does it sounds good to you?

    No. The networking stack needs to know about the bad checksums too.

Regards,
Biju

quoted

[...]

MBR, Sergey