| 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: <MN0PR12MB5953B3DD4936EAFC7BBD944AB7C9A@MN0PR12MB5953.namprd12.prod.outlook.com>
Date: Fri, 6 Oct 2023 19:04:29 +0000
From: "Pandey, Radhey Shyam" <radhey.shyam.pandey@....com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"krzysztof.kozlowski+dt@...aro.org"
<krzysztof.kozlowski+dt@...aro.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
"Simek, Michal" <michal.simek@....com>,
"linux@...linux.org.uk" <linux@...linux.org.uk>,
"f.fainelli@...il.com" <f.fainelli@...il.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"git (AMD-Xilinx)" <git@....com>
Subject: RE: [PATCH net-next v7 3/3] net: axienet: Introduce dmaengine support
> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Thursday, October 5, 2023 2:23 AM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@....com>
> Cc: davem@...emloft.net; edumazet@...gle.com; pabeni@...hat.com;
> robh+dt@...nel.org; krzysztof.kozlowski+dt@...aro.org;
> conor+dt@...nel.org; Simek, Michal <michal.simek@....com>;
> linux@...linux.org.uk; f.fainelli@...il.com; netdev@...r.kernel.org;
> devicetree@...r.kernel.org; linux-kernel@...r.kernel.org; linux-arm-
> kernel@...ts.infradead.org; git (AMD-Xilinx) <git@....com>
> Subject: Re: [PATCH net-next v7 3/3] net: axienet: Introduce dmaengine
> support
>
> On Thu, 28 Sep 2023 01:02:31 +0530 Radhey Shyam Pandey wrote:
> > +static int axienet_rx_submit_desc(struct net_device *ndev);
>
> can the forward declaration be avoided?
axienet_dma_rx_cb()- make call to axienet_rx_submit_desc(). and
axienet_rx_submit_desc() registers axienet_dma_rx_cb as callback
func. There seems to be circular dependency.
>
> > +/**
> > + * axienet_dma_tx_cb - DMA engine callback for TX channel.
> > + * @data: Pointer to the axienet_local structure.
> > + * @result: error reporting through dmaengine_result.
> > + * This function is called by dmaengine driver for TX channel to
> > +notify
> > + * that the transmit is done.
> > + */
> > +static void axienet_dma_tx_cb(void *data, const struct
> > +dmaengine_result *result) {
> > + struct axienet_local *lp = data;
> > + struct skbuf_dma_descriptor *skbuf_dma;
> > +
> > + skbuf_dma = axienet_get_tx_desc(lp, lp->tx_ring_tail++);
> > + u64_stats_update_begin(&lp->tx_stat_sync);
> > + u64_stats_add(&lp->tx_bytes, skbuf_dma->skb->len);
> > + u64_stats_add(&lp->tx_packets, 1);
> > + u64_stats_update_end(&lp->tx_stat_sync);
> > + dma_unmap_sg(lp->dev, skbuf_dma->sgl, skbuf_dma->sg_len,
> DMA_TO_DEVICE);
> > + dev_kfree_skb_any(skbuf_dma->skb);
>
> dev_consume_skb_any(), kfree is for drops
Thanks, will use dev_consume_skb_any in next version.
>
> > +/**
> > + * axienet_start_xmit_dmaengine - Starts the transmission.
> > + * @skb: sk_buff pointer that contains data to be Txed.
> > + * @ndev: Pointer to net_device structure.
> > + *
> > + * Return: NETDEV_TX_OK on success or any non space errors.
> > + * NETDEV_TX_BUSY when free element in TX skb ring buffer
> > + * is not available.
> > + *
> > + * This function is invoked from xmit to initiate transmission. The
> > + * function sets the skbs, register dma call back API and submit
> > + * the dma transaction.
> > + * Additionally if checksum offloading is supported,
> > + * it populates AXI Stream Control fields with appropriate values.
> > + */
> > +static netdev_tx_t
> > +axienet_start_xmit_dmaengine(struct sk_buff *skb, struct net_device
> > +*ndev) {
> > + struct dma_async_tx_descriptor *dma_tx_desc = NULL;
> > + struct axienet_local *lp = netdev_priv(ndev);
> > + struct skbuf_dma_descriptor *skbuf_dma;
> > + u32 app_metadata[DMA_NUM_APP_WORDS] = {0};
> > + u32 csum_start_off;
> > + u32 csum_index_off;
> > + int sg_len;
> > + int ret;
> > +
> > + sg_len = skb_shinfo(skb)->nr_frags + 1;
> > + if (!CIRC_SPACE(lp->tx_ring_head, lp->tx_ring_tail,
> TX_BD_NUM_MAX)) {
> > + netif_stop_queue(ndev);
> > + if (net_ratelimit())
> > + netdev_warn(ndev, "TX ring unexpectedly full\n");
>
> I don't see you stopping the queue when the ring gets full, am I not looking
> into the right place? Otherwise this is sort of expected to occasionally hapen
We are calling stop_queue if TX ring is full. Is that what you meant?
netif_stop_queue(ndev);
>
> > + return NETDEV_TX_BUSY;
> > + }
> > +
> > + skbuf_dma = axienet_get_tx_desc(lp, lp->tx_ring_head);
> > + if (!skbuf_dma)
> > + return NETDEV_TX_OK;
>
> Leaks the skb?
Accepted. Will add dev_kfree_skb in this and below TX_OK error path and
also increment stats.tx_errors++.
>
> > + lp->tx_ring_head++;
> > + sg_init_table(skbuf_dma->sgl, sg_len);
> > + ret = skb_to_sgvec(skb, skbuf_dma->sgl, 0, skb->len);
> > + if (ret < 0)
> > + return NETDEV_TX_OK;
> > +
> > + ret = dma_map_sg(lp->dev, skbuf_dma->sgl, sg_len,
> DMA_TO_DEVICE);
> > + if (!ret)
> > + return NETDEV_TX_OK;
> > +
> > + /* Fill up app fields for checksum */
> > + if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > + if (lp->features & XAE_FEATURE_FULL_TX_CSUM) {
> > + /* Tx Full Checksum Offload Enabled */
> > + app_metadata[0] |= 2;
> > + } else if (lp->features & XAE_FEATURE_PARTIAL_TX_CSUM) {
> > + csum_start_off = skb_transport_offset(skb);
> > + csum_index_off = csum_start_off + skb-
> >csum_offset;
> > + /* Tx Partial Checksum Offload Enabled */
> > + app_metadata[0] |= 1;
> > + app_metadata[1] = (csum_start_off << 16) |
> csum_index_off;
> > + }
> > + } else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
> > + app_metadata[0] |= 2; /* Tx Full Checksum Offload Enabled
> */
> > + }
> > +
> > + dma_tx_desc = lp->tx_chan->device->device_prep_slave_sg(lp-
> >tx_chan,
> > +skbuf_dma->sgl,
>
> Possibly store the device_prep_slave_sg pointer to a temporary variable to
> make this line less painfully long?
Will modify in next version.
>
> > + skbuf_dma = axienet_get_rx_desc(lp, lp->rx_ring_head);
> > + if (!skbuf_dma)
> > + return -ENOMEM;
> > + lp->rx_ring_head++;
> > + skb = netdev_alloc_skb(ndev, lp->max_frm_size);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + sg_init_table(skbuf_dma->sgl, 1);
> > + addr = dma_map_single(lp->dev, skb->data, lp->max_frm_size,
> DMA_FROM_DEVICE);
> > + if (unlikely(dma_mapping_error(lp->dev, addr))) {
> > + netdev_err(ndev, "DMA mapping error\n");
>
> needs rate limiting
Agree. Will change it to rate limit call.
>
> > + ret = -ENOMEM;
> > + goto rx_submit_err_free_skb;
>
> is it legal to unmap dma error ?
Will fix this error handling in next version.
>
> > + }
> > + sg_dma_address(skbuf_dma->sgl) = addr;
> > + sg_dma_len(skbuf_dma->sgl) = lp->max_frm_size;
> > + dma_rx_desc = dmaengine_prep_slave_sg(lp->rx_chan, skbuf_dma-
> >sgl,
> > + 1, DMA_DEV_TO_MEM,
> > + DMA_PREP_INTERRUPT);
>
> > +/**
> > + * axienet_init_dmaengine - init the dmaengine code.
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * Return: 0, on success.
> > + * non-zero error value on failure
> > + *
> > + * This is the dmaengine initialization code.
> > + */
> > +static int axienet_init_dmaengine(struct net_device *ndev) {
> > + struct axienet_local *lp = netdev_priv(ndev);
> > + struct skbuf_dma_descriptor *skbuf_dma;
> > + int i, ret;
> > +
> > + lp->tx_chan = dma_request_chan(lp->dev, "tx_chan0");
> > + if (IS_ERR(lp->tx_chan)) {
> > + ret = PTR_ERR(lp->tx_chan);
> > + return dev_err_probe(lp->dev, ret, "No Ethernet DMA (TX)
> channel
> > +found\n");
>
> Why use dev_err_probe()? This is not on the probe path. If ndo_open fails it
> fails, it won't get retried later by itself, right?
I see , will fix it, and remove _probe variant.
>
> > @@ -1238,7 +1540,24 @@ static int axienet_open(struct net_device
> > *ndev)
> >
> > phylink_start(lp->phylink);
> >
> > - if (!lp->use_dmaengine) {
> > + if (lp->use_dmaengine) {
> > + /* Enable interrupts for Axi Ethernet core (if defined) */
> > + if (lp->eth_irq > 0) {
> > + ret = request_irq(lp->eth_irq, axienet_eth_irq,
> IRQF_SHARED,
> > + ndev->name, ndev);
> > + if (ret)
> > + goto error_code;
> > + }
> > +
> > + ret = axienet_init_dmaengine(ndev);
> > +
>
> pointless new line
Will fix in next version.
>
> > + if (ret < 0) {
> > + if (lp->eth_irq > 0)
> > + free_irq(lp->eth_irq, ndev);
>
> can't this be on the error path?
It's just single instance so didn't introduce error label for it.
But I can move to error path.
>
> > + goto error_code;
> > + }
> > +
>
> pointless new line
Will fix it in next version.
>
> > + } else {
>
> --
> pw-bot: cr
Powered by blists - more mailing lists