| Message ID | 20240730063133.179598-1-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [net,v1] net: wan: fsl_qmc_hdlc: Discard received CRC | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 5 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 21 jobs. |
On Tue, Jul 30, 2024 at 08:31:33AM +0200, Herve Codina wrote: > Received frame from QMC contains the CRC. > Upper layers don't need this CRC and tcpdump mentioned trailing junk > data due to this CRC presence. > > As some other HDLC driver, simply discard this CRC. It might be nice to specifically site an example. But yes, I see this pattern in hdlc_rx_done(). > > Fixes: d0f2258e79fd ("net: wan: Add support for QMC HDLC") > Cc: stable@vger.kernel.org > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > --- > drivers/net/wan/fsl_qmc_hdlc.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) The above notwithstanding, this looks good to me. Reviewed-by: Simon Horman <horms@kernel.org>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 30 Jul 2024 08:31:33 +0200 you wrote: > Received frame from QMC contains the CRC. > Upper layers don't need this CRC and tcpdump mentioned trailing junk > data due to this CRC presence. > > As some other HDLC driver, simply discard this CRC. > > Fixes: d0f2258e79fd ("net: wan: Add support for QMC HDLC") > Cc: stable@vger.kernel.org > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > [...] Here is the summary with links: - [net,v1] net: wan: fsl_qmc_hdlc: Discard received CRC https://git.kernel.org/netdev/net/c/e549360069b4 You are awesome, thank you!
From: Simon Horman > Sent: 31 July 2024 09:45 > > On Tue, Jul 30, 2024 at 08:31:33AM +0200, Herve Codina wrote: > > Received frame from QMC contains the CRC. > > Upper layers don't need this CRC and tcpdump mentioned trailing junk > > data due to this CRC presence. > > > > As some other HDLC driver, simply discard this CRC. > > It might be nice to specifically site an example. > But yes, I see this pattern in hdlc_rx_done(). Pretty much the only reason you'd want the received CRC is to do error recovery assuming a single short error burst. Not that I've ever seen a driver contain the required code. 'Back of envelope' calculation: 32bit crc, 2kbyte frame, so 18bits of crc per data bit, sqrt for 'birthday paradox' - I bet you can recover 8-bit error bursts. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c index 64b4bfa6fea7..8fcfbde31a1c 100644 --- a/drivers/net/wan/fsl_qmc_hdlc.c +++ b/drivers/net/wan/fsl_qmc_hdlc.c @@ -250,6 +250,7 @@ static void qmc_hcld_recv_complete(void *context, size_t length, unsigned int fl struct qmc_hdlc_desc *desc = context; struct net_device *netdev; struct qmc_hdlc *qmc_hdlc; + size_t crc_size; int ret; netdev = desc->netdev; @@ -268,15 +269,26 @@ static void qmc_hcld_recv_complete(void *context, size_t length, unsigned int fl if (flags & QMC_RX_FLAG_HDLC_CRC) /* CRC error */ netdev->stats.rx_crc_errors++; kfree_skb(desc->skb); - } else { - netdev->stats.rx_packets++; - netdev->stats.rx_bytes += length; + goto re_queue; + } - skb_put(desc->skb, length); - desc->skb->protocol = hdlc_type_trans(desc->skb, netdev); - netif_rx(desc->skb); + /* Discard the CRC */ + crc_size = qmc_hdlc->is_crc32 ? 4 : 2; + if (length < crc_size) { + netdev->stats.rx_length_errors++; + kfree_skb(desc->skb); + goto re_queue; } + length -= crc_size; + + netdev->stats.rx_packets++; + netdev->stats.rx_bytes += length; + + skb_put(desc->skb, length); + desc->skb->protocol = hdlc_type_trans(desc->skb, netdev); + netif_rx(desc->skb); +re_queue: /* Re-queue a transfer using the same descriptor */ ret = qmc_hdlc_recv_queue(qmc_hdlc, desc, desc->dma_size); if (ret) {
Received frame from QMC contains the CRC. Upper layers don't need this CRC and tcpdump mentioned trailing junk data due to this CRC presence. As some other HDLC driver, simply discard this CRC. Fixes: d0f2258e79fd ("net: wan: Add support for QMC HDLC") Cc: stable@vger.kernel.org Signed-off-by: Herve Codina <herve.codina@bootlin.com> --- drivers/net/wan/fsl_qmc_hdlc.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)