Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 5 Apr 2022 21:12:30 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
Cc:     <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>,
        <UNGLinuxDriver@microchip.com>, <davem@davemloft.net>,
        <pabeni@redhat.com>, <michael@walle.cc>
Subject: Re: [PATCH net-next v3 3/4] net: lan966x: Add FDMA functionality
Message-ID: <20220405211230.4a1a868d@kernel.org>
In-Reply-To: <20220404130655.4004204-4-horatiu.vultur@microchip.com>
References: <20220404130655.4004204-1-horatiu.vultur@microchip.com>
        <20220404130655.4004204-4-horatiu.vultur@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 4 Apr 2022 15:06:54 +0200 Horatiu Vultur wrote:
> Ethernet frames can be extracted or injected to or from the device's
> DDR memory. There is one channel for injection and one channel for
> extraction. Each of these channels contain a linked list of DCBs which
> contains DB. The DCB contains only 1 DB for both the injection and
> extraction. Each DB contains a frame. Every time when a frame is received
> or transmitted an interrupt is generated.
> 
> It is not possible to use both the FDMA and the manual
> injection/extraction of the frames. Therefore the FDMA has priority over
> the manual because of better performance values.
> 
> FDMA:
> iperf -c 192.168.1.1
> [  5]   0.00-10.02  sec   420 MBytes   352 Mbits/sec    0 sender
> [  5]   0.00-10.03  sec   420 MBytes   351 Mbits/sec      receiver
> 
> iperf -c 192.168.1.1 -R
> [  5]   0.00-10.01  sec   528 MBytes   442 Mbits/sec    0 sender
> [  5]   0.00-10.00  sec   524 MBytes   440 Mbits/sec      receiver
> 
> Manual:
> iperf -c 192.168.1.1
> [  5]   0.00-10.02  sec  93.8 MBytes  78.5 Mbits/sec    0 sender
> [  5]   0.00-10.03  sec  93.8 MBytes  78.4 Mbits/sec      receiver
> 
> ipers -c 192.168.1.1 -R
> [  5]   0.00-10.03  sec   121 MBytes   101 Mbits/sec    0 sender
> [  5]   0.00-10.01  sec   118 MBytes  99.0 Mbits/sec      receiver
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> +static struct sk_buff *lan966x_fdma_rx_alloc_skb(struct lan966x_rx *rx,
> +						 struct lan966x_db *db)
> +{
> +	struct lan966x *lan966x = rx->lan966x;
> +	struct sk_buff *skb;
> +	dma_addr_t dma_addr;
> +	struct page *page;
> +	void *buff_addr;
> +
> +	page = dev_alloc_pages(rx->page_order);
> +	if (unlikely(!page))
> +		return NULL;
> +
> +	dma_addr = dma_map_page(lan966x->dev, page, 0,
> +				PAGE_SIZE << rx->page_order,
> +				DMA_FROM_DEVICE);
> +	if (unlikely(dma_mapping_error(lan966x->dev, dma_addr)))
> +		goto free_page;
> +
> +	buff_addr = page_address(page);
> +	skb = build_skb(buff_addr, PAGE_SIZE << rx->page_order);

build_skb() after the packet comes in rather than upfront, that way 
the skb will be in the CPU cache already when sent up the stack.

> +	if (unlikely(!skb))
> +		goto unmap_page;
> +
> +	db->dataptr = dma_addr;
> +
> +	return skb;
> +
> +unmap_page:
> +	dma_unmap_page(lan966x->dev, dma_addr,
> +		       PAGE_SIZE << rx->page_order,
> +		       DMA_FROM_DEVICE);
> +
> +free_page:
> +	__free_pages(page, rx->page_order);
> +	return NULL;
> +}

> +static int lan966x_fdma_tx_alloc(struct lan966x_tx *tx)
> +{
> +	struct lan966x *lan966x = tx->lan966x;
> +	struct lan966x_tx_dcb *dcb;
> +	struct lan966x_db *db;
> +	int size;
> +	int i, j;
> +
> +	tx->dcbs_buf = kcalloc(FDMA_DCB_MAX, sizeof(struct lan966x_tx_dcb_buf),
> +			       GFP_ATOMIC);
> +	if (!tx->dcbs_buf)
> +		return -ENOMEM;
> +
> +	/* calculate how many pages are needed to allocate the dcbs */
> +	size = sizeof(struct lan966x_tx_dcb) * FDMA_DCB_MAX;
> +	size = ALIGN(size, PAGE_SIZE);
> +	tx->dcbs = dma_alloc_coherent(lan966x->dev, size, &tx->dma, GFP_ATOMIC);

This functions seems to only be called from probe, so GFP_KERNEL 
is better.

> +	if (!tx->dcbs)
> +		goto out;
> +
> +	/* Now for each dcb allocate the db */
> +	for (i = 0; i < FDMA_DCB_MAX; ++i) {
> +		dcb = &tx->dcbs[i];
> +
> +		for (j = 0; j < FDMA_TX_DCB_MAX_DBS; ++j) {
> +			db = &dcb->db[j];
> +			db->dataptr = 0;
> +			db->status = 0;
> +		}
> +
> +		lan966x_fdma_tx_add_dcb(tx, dcb);
> +	}
> +
> +	return 0;
> +
> +out:
> +	kfree(tx->dcbs_buf);
> +	return -ENOMEM;
> +}

> +static void lan966x_fdma_wakeup_netdev(struct lan966x *lan966x)
> +{
> +	struct lan966x_port *port;
> +	int i;
> +
> +	for (i = 0; i < lan966x->num_phys_ports; ++i) {
> +		port = lan966x->ports[i];
> +		if (!port)
> +			continue;
> +
> +		if (netif_queue_stopped(port->dev))
> +			netif_wake_queue(port->dev);
> +	}
> +}
> +
> +static void lan966x_fdma_tx_clear_buf(struct lan966x *lan966x, int weight)
> +{
> +	struct lan966x_tx *tx = &lan966x->tx;
> +	struct lan966x_tx_dcb_buf *dcb_buf;
> +	struct lan966x_db *db;
> +	unsigned long flags;
> +	bool clear = false;
> +	int i;
> +
> +	spin_lock_irqsave(&lan966x->tx_lock, flags);
> +	for (i = 0; i < FDMA_DCB_MAX; ++i) {
> +		dcb_buf = &tx->dcbs_buf[i];
> +
> +		if (!dcb_buf->used)
> +			continue;
> +
> +		db = &tx->dcbs[i].db[0];
> +		if (!(db->status & FDMA_DCB_STATUS_DONE))
> +			continue;
> +
> +		dcb_buf->dev->stats.tx_packets++;
> +		dcb_buf->dev->stats.tx_bytes += dcb_buf->skb->len;
> +
> +		dcb_buf->used = false;
> +		dma_unmap_single(lan966x->dev,
> +				 dcb_buf->dma_addr,
> +				 dcb_buf->skb->len,
> +				 DMA_TO_DEVICE);
> +		if (!dcb_buf->ptp)
> +			dev_kfree_skb_any(dcb_buf->skb);
> +
> +		clear = true;
> +	}
> +	spin_unlock_irqrestore(&lan966x->tx_lock, flags);
> +
> +	if (clear)
> +		lan966x_fdma_wakeup_netdev(lan966x);

You may want to keep this call inside the lock so that the tx path
doesn't kick in between unlock and wake and fill up the queues.

> +}
> +
> +static struct sk_buff *lan966x_fdma_rx_get_frame(struct lan966x_rx *rx)
> +{
> +	struct lan966x *lan966x = rx->lan966x;
> +	u64 src_port, timestamp;
> +	struct lan966x_db *db;
> +	struct sk_buff *skb;
> +
> +	/* Check if there is any data */
> +	db = &rx->dcbs[rx->dcb_index].db[rx->db_index];
> +	if (unlikely(!(db->status & FDMA_DCB_STATUS_DONE)))
> +		return NULL;
> +
> +	/* Get the received frame and unmap it */
> +	skb = rx->skb[rx->dcb_index][rx->db_index];
> +	dma_unmap_single(lan966x->dev, (dma_addr_t)db->dataptr,
> +			 FDMA_DCB_STATUS_BLOCKL(db->status),
> +			 DMA_FROM_DEVICE);
> +
> +	skb_put(skb, FDMA_DCB_STATUS_BLOCKL(db->status));
> +
> +	lan966x_ifh_get_src_port(skb->data, &src_port);
> +	lan966x_ifh_get_timestamp(skb->data, &timestamp);
> +
> +	WARN_ON(src_port >= lan966x->num_phys_ports);
> +
> +	skb->dev = lan966x->ports[src_port]->dev;
> +	skb_pull(skb, IFH_LEN * sizeof(u32));
> +
> +	if (likely(!(skb->dev->features & NETIF_F_RXFCS)))
> +		skb_trim(skb, skb->len - ETH_FCS_LEN);
> +
> +	lan966x_ptp_rxtstamp(lan966x, skb, timestamp);
> +	skb->protocol = eth_type_trans(skb, skb->dev);
> +
> +	if (lan966x->bridge_mask & BIT(src_port)) {
> +		skb->offload_fwd_mark = 1;
> +
> +		skb_reset_network_header(skb);
> +		if (!lan966x_hw_offload(lan966x, src_port, skb))
> +			skb->offload_fwd_mark = 0;
> +	}
> +
> +	skb->dev->stats.rx_bytes += skb->len;
> +	skb->dev->stats.rx_packets++;
> +
> +	return skb;
> +}
> +
> +static int lan966x_fdma_napi_poll(struct napi_struct *napi, int weight)
> +{
> +	struct lan966x *lan966x = container_of(napi, struct lan966x, napi);
> +	struct lan966x_rx *rx = &lan966x->rx;
> +	int dcb_reload = rx->dcb_index;
> +	struct lan966x_rx_dcb *old_dcb;
> +	struct lan966x_db *db;
> +	struct sk_buff *skb;
> +	int counter = 0;
> +	u64 nextptr;
> +
> +	lan966x_fdma_tx_clear_buf(lan966x, weight);
> +
> +	/* Get all received skb */
> +	while (counter < weight) {
> +		skb = lan966x_fdma_rx_get_frame(rx);
> +		if (!skb)
> +			break;
> +		napi_gro_receive(&lan966x->napi, skb);
> +
> +		rx->skb[rx->dcb_index][rx->db_index] = NULL;
> +
> +		rx->dcb_index++;
> +		rx->dcb_index &= FDMA_DCB_MAX - 1;
> +		counter++;
> +	}
> +
> +	/* Allocate new skbs and map them */
> +	while (dcb_reload != rx->dcb_index) {
> +		db = &rx->dcbs[dcb_reload].db[rx->db_index];
> +		skb = lan966x_fdma_rx_alloc_skb(rx, db);
> +		if (unlikely(!skb))
> +			break;
> +		rx->skb[dcb_reload][rx->db_index] = skb;
> +
> +		old_dcb = &rx->dcbs[dcb_reload];
> +		dcb_reload++;
> +		dcb_reload &= FDMA_DCB_MAX - 1;
> +
> +		nextptr = rx->dma + ((unsigned long)old_dcb -
> +				     (unsigned long)rx->dcbs);
> +		lan966x_fdma_rx_add_dcb(rx, old_dcb, nextptr);
> +		lan966x_fdma_rx_reload(rx);
> +	}
> +
> +	if (counter < weight && napi_complete_done(napi, counter))
> +		lan_wr(0xff, lan966x, FDMA_INTR_DB_ENA);
> +
> +	return counter;
> +}
> +
> +irqreturn_t lan966x_fdma_irq_handler(int irq, void *args)
> +{
> +	struct lan966x *lan966x = args;
> +	u32 db, err, err_type;
> +
> +	db = lan_rd(lan966x, FDMA_INTR_DB);
> +	err = lan_rd(lan966x, FDMA_INTR_ERR);
> +
> +	if (db) {
> +		lan_wr(0, lan966x, FDMA_INTR_DB_ENA);
> +		lan_wr(db, lan966x, FDMA_INTR_DB);
> +
> +		napi_schedule(&lan966x->napi);
> +	}
> +
> +	if (err) {
> +		err_type = lan_rd(lan966x, FDMA_ERRORS);
> +
> +		WARN(1, "Unexpected error: %d, error_type: %d\n", err, err_type);
> +
> +		lan_wr(err, lan966x, FDMA_INTR_ERR);
> +		lan_wr(err_type, lan966x, FDMA_ERRORS);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int lan966x_fdma_get_next_dcb(struct lan966x_tx *tx)
> +{
> +	struct lan966x_tx_dcb_buf *dcb_buf;
> +	int i;
> +
> +	for (i = 0; i < FDMA_DCB_MAX; ++i) {
> +		dcb_buf = &tx->dcbs_buf[i];
> +		if (!dcb_buf->used && i != tx->last_in_use)
> +			return i;
> +	}
> +
> +	return -1;
> +}
> +
> +int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +	struct lan966x *lan966x = port->lan966x;
> +	struct lan966x_tx_dcb_buf *next_dcb_buf;
> +	struct lan966x_tx_dcb *next_dcb, *dcb;
> +	struct lan966x_tx *tx = &lan966x->tx;
> +	struct lan966x_db *next_db;
> +	int needed_headroom;
> +	int needed_tailroom;
> +	dma_addr_t dma_addr;
> +	int next_to_use;
> +	int err;
> +
> +	/* Get next index */
> +	next_to_use = lan966x_fdma_get_next_dcb(tx);
> +	if (next_to_use < 0) {
> +		netif_stop_queue(dev);
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	if (skb_put_padto(skb, ETH_ZLEN)) {
> +		dev->stats.tx_dropped++;
> +		return NETDEV_TX_OK;
> +	}
> +
> +	/* skb processing */
> +	needed_headroom = max_t(int, IFH_LEN * sizeof(u32) - skb_headroom(skb), 0);
> +	needed_tailroom = max_t(int, ETH_FCS_LEN - skb_tailroom(skb), 0);
> +	if (needed_headroom || needed_tailroom || skb_header_cloned(skb)) {
> +		err = pskb_expand_head(skb, needed_headroom, needed_tailroom,
> +				       GFP_ATOMIC);
> +		if (unlikely(err)) {
> +			dev->stats.tx_dropped++;
> +			err = NETDEV_TX_OK;
> +			goto release;
> +		}
> +	}
> +
> +	skb_tx_timestamp(skb);

This could move down after the dma mapping, so it's closer to when 
the devices gets ownership.

> +	skb_push(skb, IFH_LEN * sizeof(u32));
> +	memcpy(skb->data, ifh, IFH_LEN * sizeof(u32));
> +	skb_put(skb, 4);
> +
> +	dma_addr = dma_map_single(lan966x->dev, skb->data, skb->len,
> +				  DMA_TO_DEVICE);
> +	if (dma_mapping_error(lan966x->dev, dma_addr)) {
> +		dev->stats.tx_dropped++;
> +		err = NETDEV_TX_OK;
> +		goto release;
> +	}
> +
> +	/* Setup next dcb */
> +	next_dcb = &tx->dcbs[next_to_use];
> +	next_dcb->nextptr = FDMA_DCB_INVALID_DATA;
> +
> +	next_db = &next_dcb->db[0];
> +	next_db->dataptr = dma_addr;
> +	next_db->status = FDMA_DCB_STATUS_SOF |
> +			  FDMA_DCB_STATUS_EOF |
> +			  FDMA_DCB_STATUS_INTR |
> +			  FDMA_DCB_STATUS_BLOCKO(0) |
> +			  FDMA_DCB_STATUS_BLOCKL(skb->len);
> +
> +	/* Fill up the buffer */
> +	next_dcb_buf = &tx->dcbs_buf[next_to_use];
> +	next_dcb_buf->skb = skb;
> +	next_dcb_buf->dma_addr = dma_addr;
> +	next_dcb_buf->used = true;
> +	next_dcb_buf->ptp = false;
> +	next_dcb_buf->dev = dev;
> +
> +	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> +	    LAN966X_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> +		next_dcb_buf->ptp = true;
> +
> +	if (likely(lan966x->tx.activated)) {
> +		/* Connect current dcb to the next db */
> +		dcb = &tx->dcbs[tx->last_in_use];
> +		dcb->nextptr = tx->dma + (next_to_use *
> +					  sizeof(struct lan966x_tx_dcb));
> +
> +		lan966x_fdma_tx_reload(tx);
> +	} else {
> +		/* Because it is first time, then just activate */
> +		lan966x->tx.activated = true;
> +		lan966x_fdma_tx_activate(tx);
> +	}
> +
> +	/* Move to next dcb because this last in use */
> +	tx->last_in_use = next_to_use;
> +
> +	return NETDEV_TX_OK;
> +
> +release:
> +	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> +	    LAN966X_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> +		lan966x_ptp_txtstamp_release(port, skb);
> +
> +	dev_kfree_skb_any(skb);
> +	return err;
> +}
> +
> +void lan966x_fdma_netdev_init(struct lan966x *lan966x, struct net_device *dev)
> +{
> +	if (lan966x->fdma_ndev)
> +		return;
> +
> +	lan966x->fdma_ndev = dev;
> +	netif_napi_add(dev, &lan966x->napi, lan966x_fdma_napi_poll,
> +		       FDMA_WEIGHT);

Just use NAPI_POLL_WEIGHT. We should just remove the last argument 
to netif_napi_add() completely but that's another story..

> +	napi_enable(&lan966x->napi);
> +}

> +
> +	if (lan966x->fdma_irq) {
> +		disable_irq(lan966x->fdma_irq);

You don't add any enable_irq() calls, maybe devm_free_irq() is a better
choice?

> +		lan966x->fdma_irq = -ENXIO;

Semantics of lan966x->fdma_irq are pretty unclear.
Why is the condition not "> 0" for this block?

> +	}
>  }
>  
>  static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
> @@ -790,12 +801,12 @@ static void lan966x_init(struct lan966x *lan966x)
>  	/* Do byte-swap and expect status after last data word
>  	 * Extraction: Mode: manual extraction) | Byte_swap
>  	 */
> -	lan_wr(QS_XTR_GRP_CFG_MODE_SET(1) |
> +	lan_wr(QS_XTR_GRP_CFG_MODE_SET(lan966x->fdma ? 2 : 1) |
>  	       QS_XTR_GRP_CFG_BYTE_SWAP_SET(1),
>  	       lan966x, QS_XTR_GRP_CFG(0));
>  
>  	/* Injection: Mode: manual injection | Byte_swap */
> -	lan_wr(QS_INJ_GRP_CFG_MODE_SET(1) |
> +	lan_wr(QS_INJ_GRP_CFG_MODE_SET(lan966x->fdma ? 2 : 1) |
>  	       QS_INJ_GRP_CFG_BYTE_SWAP_SET(1),
>  	       lan966x, QS_INJ_GRP_CFG(0));
>  
> @@ -1017,6 +1028,17 @@ static int lan966x_probe(struct platform_device *pdev)
>  		lan966x->ptp = 1;
>  	}
>  
> +	lan966x->fdma_irq = platform_get_irq_byname(pdev, "fdma");
> +	if (lan966x->fdma_irq > 0) {
> +		err = devm_request_irq(&pdev->dev, lan966x->fdma_irq,
> +				       lan966x_fdma_irq_handler, 0,
> +				       "fdma irq", lan966x);
> +		if (err)
> +			return dev_err_probe(&pdev->dev, err, "Unable to use fdma irq");
> +
> +		lan966x->fdma = true;
> +	}
> +
>  	/* init switch */
>  	lan966x_init(lan966x);
>  	lan966x_stats_init(lan966x);
> @@ -1055,8 +1077,15 @@ static int lan966x_probe(struct platform_device *pdev)
>  	if (err)
>  		goto cleanup_fdb;
>  
> +	err = lan966x_fdma_init(lan966x);

At at quick read it's unclear why this call is not conditional 
on lan988x->fdma ?

> +	if (err)
> +		goto cleanup_ptp;