[PATCH] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[PATCH] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[Adrian Cinal]

The flag DMA_TX_APPEND_CRC was written to the first (instead of the last)
DMA descriptor in the TX path, with each descriptor corresponding to a
single skbuff fragment (or the skbuff head). This lead to packets with no
FCS appearing on the wire if the kernel allocated the packet in fragments,
which would always happen when using PACKET_MMAP/TPACKET
(cf. tpacket_fill_skb() in af_packet.c).

Signed-off-by: Adrian Cinal <adriancinal1@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 1174684a7f23..df4b0e557c76 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2137,16 +2137,16 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 		len_stat = (size << DMA_BUFLENGTH_SHIFT) |
 			   (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);
 
-		/* Note: if we ever change from DMA_TX_APPEND_CRC below we
-		 * will need to restore software padding of "runt" packets
-		 */
 		if (!i) {
-			len_stat |= DMA_TX_APPEND_CRC | DMA_SOP;
+			len_stat |= DMA_SOP;
 			if (skb->ip_summed == CHECKSUM_PARTIAL)
 				len_stat |= DMA_TX_DO_CSUM;
 		}
+		/* Note: if we ever change from DMA_TX_APPEND_CRC below we
+		 * will need to restore software padding of "runt" packets
+		 */
 		if (i == nr_frags)
-			len_stat |= DMA_EOP;
+			len_stat |= DMA_TX_APPEND_CRC | DMA_EOP;
 
 		dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping, len_stat);
 	}
-- 
2.43.0

Re: [PATCH] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]

Hello,

On 12/26/2023 6:19 PM, Adrian Cinal wrote:
> The flag DMA_TX_APPEND_CRC was written to the first (instead of the last)
> DMA descriptor in the TX path, with each descriptor corresponding to a
> single skbuff fragment (or the skbuff head). This lead to packets with no
> FCS appearing on the wire if the kernel allocated the packet in fragments,
> which would always happen when using PACKET_MMAP/TPACKET
> (cf. tpacket_fill_skb() in af_packet.c).

s/lead/leads/

> 
> Signed-off-by: Adrian Cinal <adriancinal1@gmail.com>

This looks like we could have a Fixes: tag for this change because it 
does fix an actual bug:

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")

thanks Adrian!

> ---
>   drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 1174684a7f23..df4b0e557c76 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -2137,16 +2137,16 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>   		len_stat = (size << DMA_BUFLENGTH_SHIFT) |
>   			   (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);
>   
> -		/* Note: if we ever change from DMA_TX_APPEND_CRC below we
> -		 * will need to restore software padding of "runt" packets
> -		 */
>   		if (!i) {
> -			len_stat |= DMA_TX_APPEND_CRC | DMA_SOP;
> +			len_stat |= DMA_SOP;
>   			if (skb->ip_summed == CHECKSUM_PARTIAL)
>   				len_stat |= DMA_TX_DO_CSUM;
>   		}
> +		/* Note: if we ever change from DMA_TX_APPEND_CRC below we
> +		 * will need to restore software padding of "runt" packets
> +		 */
>   		if (i == nr_frags)
> -			len_stat |= DMA_EOP;
> +			len_stat |= DMA_TX_APPEND_CRC | DMA_EOP;
>   
>   		dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping, len_stat);
>   	}

-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[PATCH v2] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[Adrian Cinal]

The flag DMA_TX_APPEND_CRC was written to the first (instead of the last)
DMA descriptor in the TX path, with each descriptor corresponding to a
single skbuff fragment (or the skbuff head). This led to packets with no
FCS appearing on the wire if the kernel allocated the packet in fragments,
which would always happen when using PACKET_MMAP/TPACKET
(cf. tpacket_fill_skb() in af_packet.c).

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Signed-off-by: Adrian Cinal <adriancinal1@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 1174684a7f23..df4b0e557c76 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2137,16 +2137,16 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 		len_stat = (size << DMA_BUFLENGTH_SHIFT) |
 			   (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);
 
-		/* Note: if we ever change from DMA_TX_APPEND_CRC below we
-		 * will need to restore software padding of "runt" packets
-		 */
 		if (!i) {
-			len_stat |= DMA_TX_APPEND_CRC | DMA_SOP;
+			len_stat |= DMA_SOP;
 			if (skb->ip_summed == CHECKSUM_PARTIAL)
 				len_stat |= DMA_TX_DO_CSUM;
 		}
+		/* Note: if we ever change from DMA_TX_APPEND_CRC below we
+		 * will need to restore software padding of "runt" packets
+		 */
 		if (i == nr_frags)
-			len_stat |= DMA_EOP;
+			len_stat |= DMA_TX_APPEND_CRC | DMA_EOP;
 
 		dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping, len_stat);
 	}
-- 
2.43.0

Re: [PATCH v2] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[Doug Berger]

On 12/27/2023 4:04 AM, Adrian Cinal wrote:
> The flag DMA_TX_APPEND_CRC was written to the first (instead of the last)
> DMA descriptor in the TX path, with each descriptor corresponding to a
> single skbuff fragment (or the skbuff head). This led to packets with no
> FCS appearing on the wire if the kernel allocated the packet in fragments,
> which would always happen when using PACKET_MMAP/TPACKET
> (cf. tpacket_fill_skb() in af_packet.c).
> 
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Signed-off-by: Adrian Cinal <adriancinal1@gmail.com>
> ---
>   drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 1174684a7f23..df4b0e557c76 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -2137,16 +2137,16 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>   		len_stat = (size << DMA_BUFLENGTH_SHIFT) |
>   			   (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);
>   
> -		/* Note: if we ever change from DMA_TX_APPEND_CRC below we
> -		 * will need to restore software padding of "runt" packets
> -		 */
>   		if (!i) {
> -			len_stat |= DMA_TX_APPEND_CRC | DMA_SOP;
> +			len_stat |= DMA_SOP;
>   			if (skb->ip_summed == CHECKSUM_PARTIAL)
>   				len_stat |= DMA_TX_DO_CSUM;
>   		}
> +		/* Note: if we ever change from DMA_TX_APPEND_CRC below we
> +		 * will need to restore software padding of "runt" packets
> +		 */
>   		if (i == nr_frags)
> -			len_stat |= DMA_EOP;
> +			len_stat |= DMA_TX_APPEND_CRC | DMA_EOP;
>   
>   		dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping, len_stat);
>   	}
Hmm... this is a little surprising since the documentation is actually 
pretty specific that the hardware signal derived from this flag be set 
along with the SOP signal.

Based on that I think I would prefer the flag to be set for all 
descriptors of a packet rather than just the last, but let me look into 
this a little further.

Thanks for bringing this to my attention,
     Doug

Re: [PATCH v2] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[Adrian Cinal]

On Wed, 27 Dec 2023 at 21:39, Doug Berger <opendmb@gmail.com> wrote:
>
> On 12/27/2023 4:04 AM, Adrian Cinal wrote:
> > The flag DMA_TX_APPEND_CRC was written to the first (instead of the last)
> > DMA descriptor in the TX path, with each descriptor corresponding to a
> > single skbuff fragment (or the skbuff head). This led to packets with no
> > FCS appearing on the wire if the kernel allocated the packet in fragments,
> > which would always happen when using PACKET_MMAP/TPACKET
> > (cf. tpacket_fill_skb() in af_packet.c).
> >
> > Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> > Signed-off-by: Adrian Cinal <adriancinal1@gmail.com>
> > ---
> >   drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > index 1174684a7f23..df4b0e557c76 100644
> > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > @@ -2137,16 +2137,16 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
> >               len_stat = (size << DMA_BUFLENGTH_SHIFT) |
> >                          (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);
> >
> > -             /* Note: if we ever change from DMA_TX_APPEND_CRC below we
> > -              * will need to restore software padding of "runt" packets
> > -              */
> >               if (!i) {
> > -                     len_stat |= DMA_TX_APPEND_CRC | DMA_SOP;
> > +                     len_stat |= DMA_SOP;
> >                       if (skb->ip_summed == CHECKSUM_PARTIAL)
> >                               len_stat |= DMA_TX_DO_CSUM;
> >               }
> > +             /* Note: if we ever change from DMA_TX_APPEND_CRC below we
> > +              * will need to restore software padding of "runt" packets
> > +              */
> >               if (i == nr_frags)
> > -                     len_stat |= DMA_EOP;
> > +                     len_stat |= DMA_TX_APPEND_CRC | DMA_EOP;
> >
> >               dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping, len_stat);
> >       }
> Hmm... this is a little surprising since the documentation is actually
> pretty specific that the hardware signal derived from this flag be set
> along with the SOP signal.
>
> Based on that I think I would prefer the flag to be set for all
> descriptors of a packet rather than just the last, but let me look into
> this a little further.
>
> Thanks for bringing this to my attention,
>      Doug

Hello,

I confirm that it works just as well when the flag is set for all
descriptors rather than just the last. Tested on a BCM2711.

Adrian

Re: [PATCH v2] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2891 bytes --]



On 12/28/2023 9:10 AM, Adrian Cinal wrote:
> On Wed, 27 Dec 2023 at 21:39, Doug Berger <opendmb@gmail.com> wrote:
>>
>> On 12/27/2023 4:04 AM, Adrian Cinal wrote:
>>> The flag DMA_TX_APPEND_CRC was written to the first (instead of the last)
>>> DMA descriptor in the TX path, with each descriptor corresponding to a
>>> single skbuff fragment (or the skbuff head). This led to packets with no
>>> FCS appearing on the wire if the kernel allocated the packet in fragments,
>>> which would always happen when using PACKET_MMAP/TPACKET
>>> (cf. tpacket_fill_skb() in af_packet.c).
>>>
>>> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
>>> Signed-off-by: Adrian Cinal <adriancinal1@gmail.com>
>>> ---
>>>    drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 +++++-----
>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> index 1174684a7f23..df4b0e557c76 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> @@ -2137,16 +2137,16 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>>>                len_stat = (size << DMA_BUFLENGTH_SHIFT) |
>>>                           (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);
>>>
>>> -             /* Note: if we ever change from DMA_TX_APPEND_CRC below we
>>> -              * will need to restore software padding of "runt" packets
>>> -              */
>>>                if (!i) {
>>> -                     len_stat |= DMA_TX_APPEND_CRC | DMA_SOP;
>>> +                     len_stat |= DMA_SOP;
>>>                        if (skb->ip_summed == CHECKSUM_PARTIAL)
>>>                                len_stat |= DMA_TX_DO_CSUM;
>>>                }
>>> +             /* Note: if we ever change from DMA_TX_APPEND_CRC below we
>>> +              * will need to restore software padding of "runt" packets
>>> +              */
>>>                if (i == nr_frags)
>>> -                     len_stat |= DMA_EOP;
>>> +                     len_stat |= DMA_TX_APPEND_CRC | DMA_EOP;
>>>
>>>                dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping, len_stat);
>>>        }
>> Hmm... this is a little surprising since the documentation is actually
>> pretty specific that the hardware signal derived from this flag be set
>> along with the SOP signal.
>>
>> Based on that I think I would prefer the flag to be set for all
>> descriptors of a packet rather than just the last, but let me look into
>> this a little further.
>>
>> Thanks for bringing this to my attention,
>>       Doug
> 
> Hello,
> 
> I confirm that it works just as well when the flag is set for all
> descriptors rather than just the last. Tested on a BCM2711.

Could you share how you triggered the problematic path? Thanks!
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH v2] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[Adrian Cinal]

[-- Attachment #1: Type: text/plain, Size: 3798 bytes --]

I attach a minimal reproducible example (largely inspired by
https://github.com/OpenDataPlane/odp/blob/master/platform/linux-generic/pktio/socket_mmap.c).
It sets up the "Packet MMAP" interface to the kernel and sends a
single broadcast frame.

If payload_size (parsed from argv[2]) is smaller than 46, then

    payload_size + sizeof(struct ether_header) + sizeof(struct llc_header) < 64

and the receiver sees a runt packet (without DMA_TX_APPEND_CRC in the
last descriptor
padding to 64 bytes is not performed either). Otherwise the packet is
rejected on the
grounds of bad (actually missing) CRC.

gcc bcmgenet_mre.c
./a.out end0 40  -->  runt packet
./a.out end0 50  -->  bad CRC

--
Adrian

On Thu, 28 Dec 2023 at 09:25, Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
>
>
> On 12/28/2023 9:10 AM, Adrian Cinal wrote:
> > On Wed, 27 Dec 2023 at 21:39, Doug Berger <opendmb@gmail.com> wrote:
> >>
> >> On 12/27/2023 4:04 AM, Adrian Cinal wrote:
> >>> The flag DMA_TX_APPEND_CRC was written to the first (instead of the last)
> >>> DMA descriptor in the TX path, with each descriptor corresponding to a
> >>> single skbuff fragment (or the skbuff head). This led to packets with no
> >>> FCS appearing on the wire if the kernel allocated the packet in fragments,
> >>> which would always happen when using PACKET_MMAP/TPACKET
> >>> (cf. tpacket_fill_skb() in af_packet.c).
> >>>
> >>> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> >>> Signed-off-by: Adrian Cinal <adriancinal1@gmail.com>
> >>> ---
> >>>    drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 +++++-----
> >>>    1 file changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> >>> index 1174684a7f23..df4b0e557c76 100644
> >>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> >>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> >>> @@ -2137,16 +2137,16 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
> >>>                len_stat = (size << DMA_BUFLENGTH_SHIFT) |
> >>>                           (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);
> >>>
> >>> -             /* Note: if we ever change from DMA_TX_APPEND_CRC below we
> >>> -              * will need to restore software padding of "runt" packets
> >>> -              */
> >>>                if (!i) {
> >>> -                     len_stat |= DMA_TX_APPEND_CRC | DMA_SOP;
> >>> +                     len_stat |= DMA_SOP;
> >>>                        if (skb->ip_summed == CHECKSUM_PARTIAL)
> >>>                                len_stat |= DMA_TX_DO_CSUM;
> >>>                }
> >>> +             /* Note: if we ever change from DMA_TX_APPEND_CRC below we
> >>> +              * will need to restore software padding of "runt" packets
> >>> +              */
> >>>                if (i == nr_frags)
> >>> -                     len_stat |= DMA_EOP;
> >>> +                     len_stat |= DMA_TX_APPEND_CRC | DMA_EOP;
> >>>
> >>>                dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping, len_stat);
> >>>        }
> >> Hmm... this is a little surprising since the documentation is actually
> >> pretty specific that the hardware signal derived from this flag be set
> >> along with the SOP signal.
> >>
> >> Based on that I think I would prefer the flag to be set for all
> >> descriptors of a packet rather than just the last, but let me look into
> >> this a little further.
> >>
> >> Thanks for bringing this to my attention,
> >>       Doug
> >
> > Hello,
> >
> > I confirm that it works just as well when the flag is set for all
> > descriptors rather than just the last. Tested on a BCM2711.
>
> Could you share how you triggered the problematic path? Thanks!
> --
> Florian

[-- Attachment #2: bcmgenet_mre.c --]
[-- Type: text/x-csrc, Size: 6090 bytes --]

#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <sys/ioctl.h>
#include <net/if.h>
#include <netinet/ether.h>
#include <arpa/inet.h>
#include <linux/if_packet.h>
#include <sys/mman.h>
#include <unistd.h>
#include <assert.h>

#define MTU                   1500
#define FRAME_MEM_SIZE        ( 4 * 1024 * 1024 )
#define BLOCK_SIZE            ( 4 * 1024 )

typedef uint8_t u8;
typedef uint16_t u16;
typedef uint32_t u32;

struct ring {
    int sock;
    int type;
    int version;
    void ** frames;
    size_t frame_num;
    size_t frame_count;
    void * mmap_base;
    size_t mmap_len;
    size_t frame_length;
    struct tpacket_req tpacket_req;
};

struct llc_header {
    u8 dsap;
    u8 ssap;
    u16 control;
};

static inline void get_own_mac(const char * if_name, u8 mac[6]);
static inline void setup_ring(struct ring * ring, int sock, int type);
static inline void send_broadcast_packet(struct ring * tx_ring, size_t payload_size, u8 sender_mac[6]);
static inline u32 round_up_to_pow_2(u32 x);

int main (int argc, char **argv) {

    if (argc < 3) {
        printf("usage: %s <interface name> <payload size>\n", argv[0]);
        exit(EXIT_FAILURE);
    }

    const char * if_name = argv[1];
    u8 own_mac[6];
    get_own_mac(argv[1], own_mac);

    char * endptr = NULL;
    size_t payload_size = strtol(argv[2], &endptr, 0);
    /* Ensure a number was parsed */
    assert(endptr != argv[2]);

    /* Open a raw socket */
    int sock = socket(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
    assert(sock != -1);
    /* Use TPACKET_V2 */
    int ver = TPACKET_V2;
    (void) setsockopt(sock, SOL_PACKET, PACKET_VERSION, &ver, sizeof(ver));
    struct sockaddr_ll sa = {
        .sll_family = PF_PACKET,
        .sll_protocol = htons(ETH_P_ALL),
        .sll_ifindex = if_nametoindex(if_name)
    };
    /* Bind the socket to a network device */
    (void) bind(sock, (const struct sockaddr *) &sa, sizeof(sa));

    /* Set up the rings in the kernel */
    struct ring tx_ring;
    struct ring rx_ring;
    setup_ring(&tx_ring, sock, PACKET_TX_RING);
    setup_ring(&rx_ring, sock, PACKET_RX_RING);

    /* Map both rings into userspace in one mmap() call */
    size_t mmap_len = rx_ring.mmap_len + tx_ring.mmap_len;
    void * mmap_base = mmap(NULL, mmap_len, PROT_READ | PROT_WRITE, \
        MAP_SHARED | MAP_LOCKED | MAP_POPULATE, sock, 0);
    assert(mmap_base != MAP_FAILED);

    /* RX ring is first in the mapped memory, followed by the TX ring */
    rx_ring.mmap_base = mmap_base;
    tx_ring.mmap_base = mmap_base + rx_ring.mmap_len;

    /* Set up the pointers */
    for (size_t i = 0; i < rx_ring.frame_count; i++)
        rx_ring.frames[i] = rx_ring.mmap_base + (i * rx_ring.frame_length);
    for (size_t i = 0; i < tx_ring.frame_count; i++)
        tx_ring.frames[i] = tx_ring.mmap_base + (i * tx_ring.frame_length);

    send_broadcast_packet(&tx_ring, payload_size, own_mac);

    return 0;
}

static inline void get_own_mac(const char * if_name, u8 mac[6]) {

    /* Open any socket as a channel to the kernel */
    int sock = socket(AF_INET, SOCK_DGRAM, 0);
    assert(sock != -1);
    /* Get the HW address */
    struct ifreq ifr;
    (void) strncpy(ifr.ifr_name, if_name, sizeof(ifr.ifr_name) - 1);
    ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
    (void) ioctl(sock, SIOCGIFHWADDR, &ifr);
    (void) close(sock);
    (void) memcpy(mac, ifr.ifr_hwaddr.sa_data, 6);
}

static inline void setup_ring(struct ring * ring, int sock, int type) {

    ring->sock = sock;
    ring->type = type;
    ring->version = TPACKET_V2;
    u32 frame_size = round_up_to_pow_2(MTU + TPACKET_HDRLEN + TPACKET_ALIGNMENT);
    u32 block_size = BLOCK_SIZE;
    if (frame_size > block_size)
        block_size = frame_size;
    u32 block_count = FRAME_MEM_SIZE / block_size;
    u32 frame_count = (block_size / frame_size) * block_count;
    u32 ring_size = frame_count * sizeof(void *);
    /* Allocate an array of pointers to the frames in shared buffer */
    ring->frames = malloc(ring_size);
    assert(ring->frames);

    ring->tpacket_req.tp_block_size = block_size;
    ring->tpacket_req.tp_block_nr = block_count;
    ring->tpacket_req.tp_frame_size = frame_size;
    ring->tpacket_req.tp_frame_nr = frame_count;

    ring->mmap_len = block_size * block_count;
    ring->frame_count = frame_count;
    ring->frame_length = frame_size;
    ring->frame_num = 0;

    (void) setsockopt(sock, SOL_PACKET, type, &ring->tpacket_req, sizeof(ring->tpacket_req));
}

static inline void send_broadcast_packet(struct ring * tx_ring, size_t payload_size, u8 sender_mac[6]) {

    struct tpacket2_hdr * tp_header = tx_ring->frames[tx_ring->frame_num];
    /* Only three lower bits encode the TX status (assert there is space in the ring) */
    u32 tp_status = tp_header->tp_status & 0x7;
    assert(tp_status == TP_STATUS_AVAILABLE);
    u8 * buf = (u8 *)(void *)tp_header + TPACKET2_HDRLEN - \
        sizeof(struct sockaddr_ll);

    /* Fill in the MAC addresses */
    struct ether_header * eth = (struct ether_header *) buf;
    (void) memset(eth->ether_dhost, 0xFF, 6);
    (void) memcpy(eth->ether_shost, sender_mac, 6);
    eth->ether_type = htons(sizeof(struct llc_header) + payload_size);
    /* Fill in the LLC header */
    struct llc_header * llc = (struct llc_header *)(eth + 1);
    llc->dsap = 0;
    llc->ssap = 0;
    llc->control = 0;
    /* Poison the payload */
    u8 * payload = (u8 *)(llc + 1);
    (void) memset(payload, 0xAA, payload_size);

    size_t total_len = sizeof(*eth) + sizeof(*llc) + payload_size;
    tp_header->tp_len = total_len;
    /* Relinquish control of the frame to the kernel */
    tp_header->tp_status = TP_STATUS_SEND_REQUEST;

    /* Ping the kernel to send the packet */
    ssize_t ret = send(tx_ring->sock, NULL, 0, MSG_DONTWAIT);
    assert(ret == total_len);
    tx_ring->frame_num = (tx_ring->frame_num + 1) % tx_ring->frame_count;
}

static inline u32 round_up_to_pow_2(u32 x) {

    u32 i;
    for (i = 1; i < x; i <<= 1) {
        ;
    }
    return i;
}

[PATCH v3] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[Adrian Cinal]

From: Adrian Cinal <adriancinal@gmail.com>

The flag DMA_TX_APPEND_CRC was only written to the first DMA descriptor
in the TX path, where each descriptor corresponds to a single skbuff
fragment (or the skbuff head). This led to packets with no FCS appearing
on the wire if the kernel allocated the packet in fragments, which would
always happen when using PACKET_MMAP/TPACKET (cf. tpacket_fill_skb() in
net/af_packet.c).

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Signed-off-by: Adrian Cinal <adriancinal1@gmail.com>
---
Differs from v2 in that now the flag DMA_TX_APPEND_CRC is set for all
fragments (so as to be in line with the specification requiring the flag
be set alongside DMA_SOP).

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 1174684a7f23..d86e5da6e157 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2140,8 +2140,10 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 		/* Note: if we ever change from DMA_TX_APPEND_CRC below we
 		 * will need to restore software padding of "runt" packets
 		 */
+		len_stat |= DMA_TX_APPEND_CRC;
+
 		if (!i) {
-			len_stat |= DMA_TX_APPEND_CRC | DMA_SOP;
+			len_stat |= DMA_SOP;
 			if (skb->ip_summed == CHECKSUM_PARTIAL)
 				len_stat |= DMA_TX_DO_CSUM;
 		}
-- 
2.43.0

Re: [PATCH v3] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[Doug Berger]

On 12/28/2023 5:56 AM, Adrian Cinal wrote:
> From: Adrian Cinal <adriancinal@gmail.com>
> 
> The flag DMA_TX_APPEND_CRC was only written to the first DMA descriptor
> in the TX path, where each descriptor corresponds to a single skbuff
> fragment (or the skbuff head). This led to packets with no FCS appearing
> on the wire if the kernel allocated the packet in fragments, which would
> always happen when using PACKET_MMAP/TPACKET (cf. tpacket_fill_skb() in
> net/af_packet.c).
> 
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Signed-off-by: Adrian Cinal <adriancinal1@gmail.com>
> ---
> Differs from v2 in that now the flag DMA_TX_APPEND_CRC is set for all
> fragments (so as to be in line with the specification requiring the flag
> be set alongside DMA_SOP).
> 
>   drivers/net/ethernet/broadcom/genet/bcmgenet.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 1174684a7f23..d86e5da6e157 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -2140,8 +2140,10 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>   		/* Note: if we ever change from DMA_TX_APPEND_CRC below we
>   		 * will need to restore software padding of "runt" packets
>   		 */
> +		len_stat |= DMA_TX_APPEND_CRC;
> +
>   		if (!i) {
> -			len_stat |= DMA_TX_APPEND_CRC | DMA_SOP;
> +			len_stat |= DMA_SOP;
>   			if (skb->ip_summed == CHECKSUM_PARTIAL)
>   				len_stat |= DMA_TX_DO_CSUM;
>   		}

Acked-by: Doug Berger <opendmb@gmail.com>

Thanks for your help!
     Doug

Re: [PATCH v3] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]



On 12/28/2023 2:56 PM, Adrian Cinal wrote:
> From: Adrian Cinal <adriancinal@gmail.com>
> 
> The flag DMA_TX_APPEND_CRC was only written to the first DMA descriptor
> in the TX path, where each descriptor corresponds to a single skbuff
> fragment (or the skbuff head). This led to packets with no FCS appearing
> on the wire if the kernel allocated the packet in fragments, which would
> always happen when using PACKET_MMAP/TPACKET (cf. tpacket_fill_skb() in
> net/af_packet.c).
> 
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Signed-off-by: Adrian Cinal <adriancinal1@gmail.com>

Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH v3] net: bcmgenet: Fix FCS generation for fragmented skbuffs

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 28 Dec 2023 14:56:38 +0100 you wrote:
> From: Adrian Cinal <adriancinal@gmail.com>
> 
> The flag DMA_TX_APPEND_CRC was only written to the first DMA descriptor
> in the TX path, where each descriptor corresponds to a single skbuff
> fragment (or the skbuff head). This led to packets with no FCS appearing
> on the wire if the kernel allocated the packet in fragments, which would
> always happen when using PACKET_MMAP/TPACKET (cf. tpacket_fill_skb() in
> net/af_packet.c).
> 
> [...]

Here is the summary with links:
  - [v3] net: bcmgenet: Fix FCS generation for fragmented skbuffs
    https://git.kernel.org/netdev/net/c/e584f2ff1e6c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html