| Message ID | 20210316081505.72898-1-bmeng.cn@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/net: fsl_etsec: Tx padding length should exclude CRC | expand |
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?
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) {
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.
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
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?
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
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.
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) {
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) {
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(-)