hw/net: fsl_etsec: Tx padding length should exclude CRC

[PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

Posted by Bin Meng 3 years, 1 month ago

As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
min_frame_len should excluce CRC, so it should be 60 instead of 64.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 hw/net/fsl_etsec/rings.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index d6be0d7d18..8f08446415 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -259,7 +259,7 @@ static void process_tx_bd(eTSEC         *etsec,
                 || etsec->regs[MACCFG2].value & MACCFG2_PADCRC) {
 
                 /* Padding and CRC (Padding implies CRC) */
-                tx_padding_and_crc(etsec, 64);
+                tx_padding_and_crc(etsec, 60);
 
             } else if (etsec->first_bd.flags & BD_TX_TC
                        || etsec->regs[MACCFG2].value & MACCFG2_CRC_EN) {
-- 
2.17.1

Re: [PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

Posted by Bin Meng 3 years, 1 month ago

On Tue, Mar 16, 2021 at 4:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> min_frame_len should excluce CRC, so it should be 60 instead of 64.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  hw/net/fsl_etsec/rings.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Ping?

Re: [PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

Posted by David Gibson 3 years, 1 month ago

On Mon, Mar 22, 2021 at 09:29:12AM +0800, Bin Meng wrote:
> On Tue, Mar 16, 2021 at 4:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> > min_frame_len should excluce CRC, so it should be 60 instead of 64.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  hw/net/fsl_etsec/rings.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> 
> Ping?

Sorry, I was away on holiday last week.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

Posted by David Gibson 3 years, 1 month ago

On Tue, Mar 16, 2021 at 04:15:05PM +0800, Bin Meng wrote:
> As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> min_frame_len should excluce CRC, so it should be 60 instead of 64.

Sorry, your reasoning still isn't clear to me.  If qemu is not adding
the CRC, what is?  Will it always append a CRC after this padding is
complete?

> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  hw/net/fsl_etsec/rings.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index d6be0d7d18..8f08446415 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -259,7 +259,7 @@ static void process_tx_bd(eTSEC         *etsec,
>                  || etsec->regs[MACCFG2].value & MACCFG2_PADCRC) {
>  
>                  /* Padding and CRC (Padding implies CRC) */
> -                tx_padding_and_crc(etsec, 64);
> +                tx_padding_and_crc(etsec, 60);
>  
>              } else if (etsec->first_bd.flags & BD_TX_TC
>                         || etsec->regs[MACCFG2].value & MACCFG2_CRC_EN) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

Posted by Bin Meng 3 years, 1 month ago

Hi David,

On Mon, Mar 22, 2021 at 12:11 PM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Tue, Mar 16, 2021 at 04:15:05PM +0800, Bin Meng wrote:
> > As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> > min_frame_len should excluce CRC, so it should be 60 instead of 64.
>
> Sorry, your reasoning still isn't clear to me.  If qemu is not adding
> the CRC, what is?

No one is padding CRC in QEMU. QEMU network backends pass payload
without CRC in between.

> Will it always append a CRC after this padding is complete?

No.

Regards,
Bin

Re: [PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

Posted by David Gibson 3 years, 1 month ago

On Mon, Mar 22, 2021 at 12:33:06PM +0800, Bin Meng wrote:
> Hi David,
> 
> On Mon, Mar 22, 2021 at 12:11 PM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >
> > On Tue, Mar 16, 2021 at 04:15:05PM +0800, Bin Meng wrote:
> > > As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> > > min_frame_len should excluce CRC, so it should be 60 instead of 64.
> >
> > Sorry, your reasoning still isn't clear to me.  If qemu is not adding
> > the CRC, what is?
> 
> No one is padding CRC in QEMU. QEMU network backends pass payload
> without CRC in between.

Ok, but the CRCs must be added if the packets are bridged onto a real
device, yes?  Where does that happen?
> 
> > Will it always append a CRC after this padding is complete?
> 
> No.

If that's true, then won't the packets still be shorter than expected
if we only pad to 60 bytes?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

Posted by Bin Meng 3 years, 1 month ago

Hi David,

On Mon, Mar 22, 2021 at 1:24 PM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Mon, Mar 22, 2021 at 12:33:06PM +0800, Bin Meng wrote:
> > Hi David,
> >
> > On Mon, Mar 22, 2021 at 12:11 PM David Gibson
> > <david@gibson.dropbear.id.au> wrote:
> > >
> > > On Tue, Mar 16, 2021 at 04:15:05PM +0800, Bin Meng wrote:
> > > > As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> > > > min_frame_len should excluce CRC, so it should be 60 instead of 64.
> > >
> > > Sorry, your reasoning still isn't clear to me.  If qemu is not adding
> > > the CRC, what is?
> >
> > No one is padding CRC in QEMU. QEMU network backends pass payload
> > without CRC in between.
>
> Ok, but the CRCs must be added if the packets are bridged onto a real
> device, yes?  Where does that happen?

I've never used it like that before. What's the command line to test that?

> >
> > > Will it always append a CRC after this padding is complete?
> >
> > No.
>
> If that's true, then won't the packets still be shorter than expected
> if we only pad to 60 bytes?

In QEMU packets are transmitted without CRC between network backends,
and when a NIC receives a packet, the minimum required payload length
is 60 bytes without a CRC.

Regards,
Bin

Re: [PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

Posted by David Gibson 3 years, 1 month ago

On Mon, Mar 22, 2021 at 01:48:07PM +0800, Bin Meng wrote:
> Hi David,
> 
> On Mon, Mar 22, 2021 at 1:24 PM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >
> > On Mon, Mar 22, 2021 at 12:33:06PM +0800, Bin Meng wrote:
> > > Hi David,
> > >
> > > On Mon, Mar 22, 2021 at 12:11 PM David Gibson
> > > <david@gibson.dropbear.id.au> wrote:
> > > >
> > > > On Tue, Mar 16, 2021 at 04:15:05PM +0800, Bin Meng wrote:
> > > > > As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> > > > > min_frame_len should excluce CRC, so it should be 60 instead of 64.
> > > >
> > > > Sorry, your reasoning still isn't clear to me.  If qemu is not adding
> > > > the CRC, what is?
> > >
> > > No one is padding CRC in QEMU. QEMU network backends pass payload
> > > without CRC in between.
> >
> > Ok, but the CRCs must be added if the packets are bridged onto a real
> > device, yes?  Where does that happen?
> 
> I've never used it like that before. What's the command line to test that?
> 
> > >
> > > > Will it always append a CRC after this padding is complete?
> > >
> > > No.
> >
> > If that's true, then won't the packets still be shorter than expected
> > if we only pad to 60 bytes?
> 
> In QEMU packets are transmitted without CRC between network backends,
> and when a NIC receives a packet, the minimum required payload length
> is 60 bytes without a CRC.

Ok, I see what you're saying, and indeed it already pads to 60 bytes,
rather than 64 on the Rx side.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] hw/net: fsl_etsec: Tx padding length should exclude CRC

Posted by David Gibson 3 years, 1 month ago

On Tue, Mar 16, 2021 at 04:15:05PM +0800, Bin Meng wrote:
> As the comment of tx_padding_and_crc() says: "Never add CRC in QEMU",
> min_frame_len should excluce CRC, so it should be 60 instead of 64.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Applied to ppc-for-6.0, thanks.

> ---
> 
>  hw/net/fsl_etsec/rings.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index d6be0d7d18..8f08446415 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -259,7 +259,7 @@ static void process_tx_bd(eTSEC         *etsec,
>                  || etsec->regs[MACCFG2].value & MACCFG2_PADCRC) {
>  
>                  /* Padding and CRC (Padding implies CRC) */
> -                tx_padding_and_crc(etsec, 64);
> +                tx_padding_and_crc(etsec, 60);
>  
>              } else if (etsec->first_bd.flags & BD_TX_TC
>                         || etsec->regs[MACCFG2].value & MACCFG2_CRC_EN) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson