| 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
| ||
|
Message-ID: <20211227183541.3a9f2963@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date: Mon, 27 Dec 2021 18:35:41 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: conleylee@...mail.com
Cc: davem@...emloft.net, mripard@...nel.org, wens@...e.org,
netdev@...r.kernel.org
Subject: Re: [PATCH v4] sun4i-emac.c: add dma support
On Fri, 24 Dec 2021 22:44:31 +0800 conleylee@...mail.com wrote:
> From: Conley Lee <conleylee@...mail.com>
>
> This patch adds support for the emac rx dma present on sun4i.
> The emac is able to move packets from rx fifo to RAM by using dma.
>
> Signed-off-by: Conley Lee <conleylee@...mail.com>
> ---
> drivers/net/ethernet/allwinner/sun4i-emac.c | 209 +++++++++++++++++++-
> 1 file changed, 200 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c
> index cccf8a3ead5e..b17d2df17335 100644
> --- a/drivers/net/ethernet/allwinner/sun4i-emac.c
> +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
> @@ -29,6 +29,7 @@
> #include <linux/platform_device.h>
> #include <linux/phy.h>
> #include <linux/soc/sunxi/sunxi_sram.h>
> +#include <linux/dmaengine.h>
>
> #include "sun4i-emac.h"
>
> @@ -86,6 +87,16 @@ struct emac_board_info {
> unsigned int duplex;
>
> phy_interface_t phy_interface;
> + struct dma_chan *rx_chan;
> + phys_addr_t emac_rx_fifo;
> +};
> +
> +struct emac_dma_req {
> + struct emac_board_info *db;
> + struct dma_async_tx_descriptor *desc;
> + struct sk_buff *sbk;
sbk -> skb ?
> + dma_addr_t rxbuf;
> + int count;
> };
>
> static void emac_update_speed(struct net_device *dev)
> @@ -205,6 +216,113 @@ static void emac_inblk_32bit(void __iomem *reg, void *data, int count)
> readsl(reg, data, round_up(count, 4) / 4);
> }
>
> +static struct emac_dma_req *
> +alloc_emac_dma_req(struct emac_board_info *db,
nit: please use "emac" as a prefix of the name, instead of putting it
inside the name.
> + struct dma_async_tx_descriptor *desc, struct sk_buff *skb,
> + dma_addr_t rxbuf, int count)
> +{
> + struct emac_dma_req *req =
> + kzalloc(sizeof(struct emac_dma_req), GFP_KERNEL);
Please don't put complex initializers inline, also missing an empty
line between variable declaration and code. Should be:
struct emac_dma_req *req;
req = kzalloc(...);
if (!req)
...
You seem to call this from an IRQ handler, shouldn't the flags be
GFP_ATOMIC? Please test with CONFIG_DEBUG_ATOMIC_SLEEP=y.
> + if (!req)
> + return NULL;
> +
> + req->db = db;
> + req->desc = desc;
> + req->sbk = skb;
> + req->rxbuf = rxbuf;
> + req->count = count;
> + return req;
> +}
> +
> +static void free_emac_dma_req(struct emac_dma_req *req)
> +{
> + kfree(req);
> +}
> +
> +static void emac_dma_done_callback(void *arg)
> +{
> + struct emac_dma_req *req = arg;
> + struct emac_board_info *db = req->db;
> + struct sk_buff *skb = req->sbk;
> + struct net_device *dev = db->ndev;
> + int rxlen = req->count;
> + u32 reg_val;
> +
> + dma_unmap_single(db->dev, req->rxbuf, rxlen, DMA_FROM_DEVICE);
> +
> + skb->protocol = eth_type_trans(skb, dev);
> + netif_rx(skb);
> + dev->stats.rx_bytes += rxlen;
> + /* Pass to upper layer */
> + dev->stats.rx_packets++;
> +
> + //re enable cpu receive
Please use the /**/ comment style consistently
> + reg_val = readl(db->membase + EMAC_RX_CTL_REG);
> + reg_val &= ~EMAC_RX_CTL_DMA_EN;
> + writel(reg_val, db->membase + EMAC_RX_CTL_REG);
> +
> + //re enable interrupt
> + reg_val = readl(db->membase + EMAC_INT_CTL_REG);
> + reg_val |= (0x01 << 8);
> + writel(reg_val, db->membase + EMAC_INT_CTL_REG);
> +
> + db->emacrx_completed_flag = 1;
> + free_emac_dma_req(req);
> +}
> +
> +static void emac_dma_inblk_32bit(struct emac_board_info *db,
> + struct sk_buff *skb, int count)
> +{
> + struct dma_async_tx_descriptor *desc;
> + dma_cookie_t cookie;
> + dma_addr_t rxbuf;
> + void *rdptr;
> + struct emac_dma_req *req;
> +
> + rdptr = skb_put(skb, count - 4);
The skb_put can be factored out from both branches it seems.
> + rxbuf = dma_map_single(db->dev, rdptr, count, DMA_FROM_DEVICE);
> +
> + if (dma_mapping_error(db->dev, rxbuf)) {
> + dev_err(db->dev, "dma mapping error.\n");
> + return;
You seem to leak the skb if this function fails, no?
> + }
> +
> + desc = dmaengine_prep_slave_single(db->rx_chan, rxbuf, count,
> + DMA_DEV_TO_MEM,
> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> + if (!desc) {
> + dev_err(db->dev, "prepare slave single failed\n");
> + goto prepare_err;
> + }
> +
> + req = alloc_emac_dma_req(db, desc, skb, rxbuf, count);
> + if (!req) {
> + dev_err(db->dev, "alloc emac dma req error.\n");
> + goto alloc_req_err;
> + }
> +
> + desc->callback_param = req;
> + desc->callback = emac_dma_done_callback;
> +
> + cookie = dmaengine_submit(desc);
> + if (dma_submit_error(cookie)) {
> + dev_err(db->dev, "dma submit error.\n");
> + goto submit_err;
> + }
> +
> + dma_async_issue_pending(db->rx_chan);
> + return;
> +
> +submit_err:
> + free_emac_dma_req(req);
> +
> +alloc_req_err:
> + dmaengine_desc_free(desc);
> +
> +prepare_err:
> + dma_unmap_single(db->dev, rxbuf, count, DMA_FROM_DEVICE);
> +}
> - /* Pass to upper layer */
> - skb->protocol = eth_type_trans(skb, dev);
> - netif_rx(skb);
> - dev->stats.rx_packets++;
> + if (rxlen < dev->mtu || !db->rx_chan) {
> + rdptr = skb_put(skb, rxlen - 4);
> + emac_inblk_32bit(db->membase + EMAC_RX_IO_DATA_REG,
> + rdptr, rxlen);
> + dev->stats.rx_bytes += rxlen;
> +
> + /* Pass to upper layer */
> + skb->protocol = eth_type_trans(skb, dev);
> + netif_rx(skb);
> + dev->stats.rx_packets++;
> + } else {
> + reg_val = readl(db->membase + EMAC_RX_CTL_REG);
> + reg_val |= EMAC_RX_CTL_DMA_EN;
> + writel(reg_val, db->membase + EMAC_RX_CTL_REG);
> + emac_dma_inblk_32bit(db, skb, rxlen);
> + break;
> + }
> }
> +static int emac_configure_dma(struct emac_board_info *db)
> +{
> + struct platform_device *pdev = db->pdev;
> + struct net_device *ndev = db->ndev;
> + struct dma_slave_config conf = {};
> + struct resource *regs;
> + int err = 0;
> +
> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!regs) {
> + netdev_err(ndev, "get io resource from device failed.\n");
> + err = -ENOMEM;
> + goto out_clear_chan;
> + }
> +
> + netdev_info(ndev, "get io resource from device: 0x%x, size = %u\n",
> + regs->start, resource_size(regs));
> + db->emac_rx_fifo = regs->start + EMAC_RX_IO_DATA_REG;
> +
> + db->rx_chan = dma_request_chan(&pdev->dev, "rx");
> + if (IS_ERR(db->rx_chan)) {
> + netdev_err(ndev,
> + "failed to request dma channel. dma is disabled");
nit: this message lacks '\n' at the end
> + err = PTR_ERR(db->rx_chan);
> + goto out_clear_chan;
> + }
Powered by blists - more mailing lists