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

Hi Sergey,

Subject: 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

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

quoted

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

CHECKSUM_UNNCESSARY.

quoted

Does it sounds good to you?

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

Currently some of the drivers is doing this way only. It doesn't pass bad checksum.
Non-error case sets CHECKSUM_UNNCESSARY and other case sets CHECKSUM_NONE to handle
It by stack.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qualcomm/emac/emac-mac.c#L1147
[2] https://elixir.bootlin.com/linux/latest/source/drivers/staging/octeon/ethernet-rx.c#L343

Regards,
Biju

quoted

Regards,
Biju

quoted

quoted

[...]

MBR, Sergey