| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 01 Feb 2024 12:41:32 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Herve Codina <herve.codina@...tlin.com>, Vadim Fedorenko
<vadim.fedorenko@...ux.dev>, "David S. Miller" <davem@...emloft.net>, Eric
Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, Andrew Lunn <andrew@...n.ch>, Mark Brown
<broonie@...nel.org>, Christophe Leroy <christophe.leroy@...roup.eu>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 1/6] net: wan: Add support for QMC HDLC
On Tue, 2024-01-30 at 09:40 +0100, Herve Codina wrote:
> The QMC HDLC driver provides support for HDLC using the QMC (QUICC
> Multichannel Controller) to transfer the HDLC data.
>
> Signed-off-by: Herve Codina <herve.codina@...tlin.com>
> Reviewed-by: Christophe Leroy <christophe.leroy@...roup.eu>
> Acked-by: Jakub Kicinski <kuba@...nel.org>
> ---
> drivers/net/wan/Kconfig | 12 +
> drivers/net/wan/Makefile | 1 +
> drivers/net/wan/fsl_qmc_hdlc.c | 422 +++++++++++++++++++++++++++++++++
> 3 files changed, 435 insertions(+)
> create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
>
> diff --git a/drivers/net/wan/Kconfig b/drivers/net/wan/Kconfig
> index 7dda87756d3f..31ab2136cdf1 100644
> --- a/drivers/net/wan/Kconfig
> +++ b/drivers/net/wan/Kconfig
> @@ -197,6 +197,18 @@ config FARSYNC
> To compile this driver as a module, choose M here: the
> module will be called farsync.
>
> +config FSL_QMC_HDLC
> + tristate "Freescale QMC HDLC support"
> + depends on HDLC
> + depends on CPM_QMC
> + help
> + HDLC support using the Freescale QUICC Multichannel Controller (QMC).
> +
> + To compile this driver as a module, choose M here: the
> + module will be called fsl_qmc_hdlc.
> +
> + If unsure, say N.
> +
> config FSL_UCC_HDLC
> tristate "Freescale QUICC Engine HDLC support"
> depends on HDLC
> diff --git a/drivers/net/wan/Makefile b/drivers/net/wan/Makefile
> index 8119b49d1da9..00e9b7ee1e01 100644
> --- a/drivers/net/wan/Makefile
> +++ b/drivers/net/wan/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_WANXL) += wanxl.o
> obj-$(CONFIG_PCI200SYN) += pci200syn.o
> obj-$(CONFIG_PC300TOO) += pc300too.o
> obj-$(CONFIG_IXP4XX_HSS) += ixp4xx_hss.o
> +obj-$(CONFIG_FSL_QMC_HDLC) += fsl_qmc_hdlc.o
> obj-$(CONFIG_FSL_UCC_HDLC) += fsl_ucc_hdlc.o
> obj-$(CONFIG_SLIC_DS26522) += slic_ds26522.o
>
> diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c
> new file mode 100644
> index 000000000000..e7b2b72a6050
> --- /dev/null
> +++ b/drivers/net/wan/fsl_qmc_hdlc.c
> @@ -0,0 +1,422 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Freescale QMC HDLC Device Driver
> + *
> + * Copyright 2023 CS GROUP France
> + *
> + * Author: Herve Codina <herve.codina@...tlin.com>
> + */
> +
> +#include <linux/dma-mapping.h>
> +#include <linux/hdlc.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <soc/fsl/qe/qmc.h>
> +
> +struct qmc_hdlc_desc {
> + struct net_device *netdev;
> + struct sk_buff *skb; /* NULL if the descriptor is not in use */
> + dma_addr_t dma_addr;
> + size_t dma_size;
> +};
> +
> +struct qmc_hdlc {
> + struct device *dev;
> + struct qmc_chan *qmc_chan;
> + struct net_device *netdev;
> + bool is_crc32;
> + spinlock_t tx_lock; /* Protect tx descriptors */
> + struct qmc_hdlc_desc tx_descs[8];
> + unsigned int tx_out;
> + struct qmc_hdlc_desc rx_descs[4];
> +};
> +
> +static inline struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
> +{
> + return dev_to_hdlc(netdev)->priv;
> +}
Please, no 'inline' function in c files. You could move this function
and the struct definition into a new, local, header file.
> +static int qmc_hdlc_recv_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc *desc, size_t size);
> +
> +#define QMC_HDLC_RX_ERROR_FLAGS (QMC_RX_FLAG_HDLC_OVF | \
> + QMC_RX_FLAG_HDLC_UNA | \
> + QMC_RX_FLAG_HDLC_ABORT | \
> + QMC_RX_FLAG_HDLC_CRC)
> +
> +static void qmc_hcld_recv_complete(void *context, size_t length, unsigned int flags)
> +{
> + struct qmc_hdlc_desc *desc = context;
> + struct net_device *netdev = desc->netdev;
> + struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
> + int ret;
Please, respect the reverse x-mas tree order for local variable
definition.
> + dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size, DMA_FROM_DEVICE);
> +
> + if (flags & QMC_HDLC_RX_ERROR_FLAGS) {
> + netdev->stats.rx_errors++;
> + if (flags & QMC_RX_FLAG_HDLC_OVF) /* Data overflow */
> + netdev->stats.rx_over_errors++;
> + if (flags & QMC_RX_FLAG_HDLC_UNA) /* bits received not multiple of 8 */
> + netdev->stats.rx_frame_errors++;
> + if (flags & QMC_RX_FLAG_HDLC_ABORT) /* Received an abort sequence */
> + netdev->stats.rx_frame_errors++;
> + 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;
> +
> + skb_put(desc->skb, length);
> + desc->skb->protocol = hdlc_type_trans(desc->skb, netdev);
> + netif_rx(desc->skb);
> + }
> +
> + /* Re-queue a transfer using the same descriptor */
> + ret = qmc_hdlc_recv_queue(qmc_hdlc, desc, desc->dma_size);
> + if (ret) {
> + dev_err(qmc_hdlc->dev, "queue recv desc failed (%d)\n", ret);
> + netdev->stats.rx_errors++;
> + }
> +}
[...]
> +static netdev_tx_t qmc_hdlc_xmit(struct sk_buff *skb, struct net_device *netdev)
> +{
> + struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
> + struct qmc_hdlc_desc *desc;
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&qmc_hdlc->tx_lock, flags);
> + desc = &qmc_hdlc->tx_descs[qmc_hdlc->tx_out];
> + if (WARN_ONCE(!desc->skb, "No tx descriptors available\n")) {
> + /* Should never happen.
> + * Previous xmit should have already stopped the queue.
> + */
> + netif_stop_queue(netdev);
> + spin_unlock_irqrestore(&qmc_hdlc->tx_lock, flags);
> + return NETDEV_TX_BUSY;
> + }
> + spin_unlock_irqrestore(&qmc_hdlc->tx_lock, flags);
> +
> + desc->netdev = netdev;
> + desc->dma_size = skb->len;
> + desc->skb = skb;
> + ret = qmc_hdlc_xmit_queue(qmc_hdlc, desc);
> + if (ret) {
> + desc->skb = NULL; /* Release the descriptor */
> + if (ret == -EBUSY) {
> + netif_stop_queue(netdev);
> + return NETDEV_TX_BUSY;
> + }
> + dev_kfree_skb(skb);
> + netdev->stats.tx_dropped++;
> + return NETDEV_TX_OK;
> + }
> +
> + qmc_hdlc->tx_out = (qmc_hdlc->tx_out + 1) % ARRAY_SIZE(qmc_hdlc->tx_descs);
> +
> + spin_lock_irqsave(&qmc_hdlc->tx_lock, flags);
> + if (qmc_hdlc->tx_descs[qmc_hdlc->tx_out].skb)
> + netif_stop_queue(netdev);
> + spin_unlock_irqrestore(&qmc_hdlc->tx_lock, flags);
The locking schema is quite bad, as the drivers acquires and releases 3
locks for each tx packet. You could improve that using the qmc_chan-
>tx_lock to protect the whole qmc_hdlc_xmit() function, factoring out a
lockless variant of qmc_hdlc_xmit_queue(), and using it here.
In general is quite bad that the existing infra does not allow
leveraging NAPI. Have you considered expanding the QMC to accomodate
such user?
Cheers,
Paolo
Powered by blists - more mailing lists