Hi Jakub,
On 26/07/23 9:39 am, Jakub Kicinski wrote:
> On Mon, 24 Jul 2023 16:59:30 +0530 MD Danish Anwar wrote:
>> drivers/net/ethernet/ti/Kconfig | 13 +
>> drivers/net/ethernet/ti/Makefile | 3 +
>> drivers/net/ethernet/ti/icssg_prueth.c | 1831 ++++++++++++++++++++++++
>> drivers/net/ethernet/ti/icssg_prueth.h | 48 +
>
> Please create a sub-directory for the driver.
>
>> +static int prueth_ndev_add_tx_napi(struct prueth_emac *emac)
>> +{
>> + struct prueth *prueth = emac->prueth;
>> + int i, ret;
>> +
>> + for (i = 0; i < emac->tx_ch_num; i++) {
>> + struct prueth_tx_chn *tx_chn = &emac->tx_chns[i];
>> +
>> + netif_napi_add_tx_weight(emac->ndev, &tx_chn->napi_tx,
>> + emac_napi_tx_poll, NAPI_POLL_WEIGHT);
>
> Skip specifying weight, please.
>
Sure, Will change this to 'netif_napi_add_tx(emac->ndev, &tx_chn->
emac_napi_tx_poll);'
>> +/**
>> + * emac_ndo_start_xmit - EMAC Transmit function
>> + * @skb: SKB pointer
>> + * @ndev: EMAC network adapter
>> + *
>> + * Called by the system to transmit a packet - we queue the packet in
>> + * EMAC hardware transmit queue
>> + * Doesn't wait for completion we'll check for TX completion in
>> + * emac_tx_complete_packets().
>> + *
>> + * Return: enum netdev_tx
>> + */
>> +static enum netdev_tx emac_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> + struct cppi5_host_desc_t *first_desc, *next_desc, *cur_desc;
>> + struct prueth_emac *emac = netdev_priv(ndev);
>> + struct netdev_queue *netif_txq;
>> + struct prueth_tx_chn *tx_chn;
>> + dma_addr_t desc_dma, buf_dma;
>> + int i, ret = 0, q_idx;
>> + void **swdata;
>> + u32 pkt_len;
>> + u32 *epib;
>> +
>> + pkt_len = skb_headlen(skb);
>> + q_idx = skb_get_queue_mapping(skb);
>> +
>> + tx_chn = &emac->tx_chns[q_idx];
>> + netif_txq = netdev_get_tx_queue(ndev, q_idx);
>> +
>> + /* Map the linear buffer */
>> + buf_dma = dma_map_single(tx_chn->dma_dev, skb->data, pkt_len, DMA_TO_DEVICE);
>> + if (dma_mapping_error(tx_chn->dma_dev, buf_dma)) {
>> + netdev_err(ndev, "tx: failed to map skb buffer\n");
>> + ret = NETDEV_TX_BUSY;
>
> Drop it if it can't be mapped and return OK. What's going to re-enable
> the queue in this case?
>
Sure. I will drop the packet and return NETDEV_TX_OK.
>> + goto drop_stop_q;
>> + }
>> +
>> + first_desc = k3_cppi_desc_pool_alloc(tx_chn->desc_pool);
>> + if (!first_desc) {
>> + netdev_dbg(ndev, "tx: failed to allocate descriptor\n");
>> + dma_unmap_single(tx_chn->dma_dev, buf_dma, pkt_len, DMA_TO_DEVICE);
>> + ret = NETDEV_TX_BUSY;
>> + goto drop_stop_q_busy;
>> + }
>> +
>> + cppi5_hdesc_init(first_desc, CPPI5_INFO0_HDESC_EPIB_PRESENT,
>> + PRUETH_NAV_PS_DATA_SIZE);
>> + cppi5_hdesc_set_pkttype(first_desc, 0);
>> + epib = first_desc->epib;
>> + epib[0] = 0;
>> + epib[1] = 0;
>> +
>> + /* set dst tag to indicate internal qid at the firmware which is at
>> + * bit8..bit15. bit0..bit7 indicates port num for directed
>> + * packets in case of switch mode operation
>> + */
>> + cppi5_desc_set_tags_ids(&first_desc->hdr, 0, (emac->port_id | (q_idx << 8)));
>> + k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
>> + cppi5_hdesc_attach_buf(first_desc, buf_dma, pkt_len, buf_dma, pkt_len);
>> + swdata = cppi5_hdesc_get_swdata(first_desc);
>> + *swdata = skb;
>> +
>> + if (!skb_is_nonlinear(skb))
>> + goto tx_push;
>
> Why the goto? The loop won't be entered.
>
skb_is_nonlinear() will return true when skb is fragmented i.e.
skb_shinfo(skb)->nr_frags > 0.
Makes sense to drop the if condition. As for non-fragmented skb,
skb_shinfo(skb)->nr_frags = 0 and we won't enter for loop and will eventually
reach at label tx_push.
I will drop the if condition.
>> + /* Handle the case where skb is fragmented in pages */
>> + cur_desc = first_desc;
>> + for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
>> + skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
>> + u32 frag_size = skb_frag_size(frag);
>> +
>> + next_desc = k3_cppi_desc_pool_alloc(tx_chn->desc_pool);
>> + if (!next_desc) {
>> + netdev_err(ndev,
>> + "tx: failed to allocate frag. descriptor\n");
>> + ret = NETDEV_TX_BUSY;
>> + goto drop_free_descs;
>> + }
>> +
>> + buf_dma = skb_frag_dma_map(tx_chn->dma_dev, frag, 0, frag_size,
>> + DMA_TO_DEVICE);
>> + if (dma_mapping_error(tx_chn->dma_dev, buf_dma)) {
>> + netdev_err(ndev, "tx: Failed to map skb page\n");
>> + k3_cppi_desc_pool_free(tx_chn->desc_pool, next_desc);
>> + ret = NETDEV_TX_BUSY;
>> + goto drop_free_descs;
>
> this label frees the skb, you can't return BUSY
>
Sure. Will return OK here.
>> + }
>> +
>> + cppi5_hdesc_reset_hbdesc(next_desc);
>> + k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
>> + cppi5_hdesc_attach_buf(next_desc,
>> + buf_dma, frag_size, buf_dma, frag_size);
>> +
>> + desc_dma = k3_cppi_desc_pool_virt2dma(tx_chn->desc_pool,
>> + next_desc);
>> + k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &desc_dma);
>> + cppi5_hdesc_link_hbdesc(cur_desc, desc_dma);
>> +
>> + pkt_len += frag_size;
>> + cur_desc = next_desc;
>> + }
>> + WARN_ON(pkt_len != skb->len);
>
> WARN_ON_ONCE() if at all
>
Sure.
>> +
>> +tx_push:
>> + /* report bql before sending packet */
>> + netdev_tx_sent_queue(netif_txq, pkt_len);
>> +
>> + cppi5_hdesc_set_pktlen(first_desc, pkt_len);
>> + desc_dma = k3_cppi_desc_pool_virt2dma(tx_chn->desc_pool, first_desc);
>> + /* cppi5_desc_dump(first_desc, 64); */
>> +
>> + skb_tx_timestamp(skb); /* SW timestamp if SKBTX_IN_PROGRESS not set */
>> + ret = k3_udma_glue_push_tx_chn(tx_chn->tx_chn, first_desc, desc_dma);
>> + if (ret) {
>> + netdev_err(ndev, "tx: push failed: %d\n", ret);
>> + goto drop_free_descs;
>> + }
>> +
>> + if (k3_cppi_desc_pool_avail(tx_chn->desc_pool) < MAX_SKB_FRAGS) {
>> + netif_tx_stop_queue(netif_txq);
>> + /* Barrier, so that stop_queue visible to other cpus */
>> + smp_mb__after_atomic();
>> +
>> + if (k3_cppi_desc_pool_avail(tx_chn->desc_pool) >=
>> + MAX_SKB_FRAGS)
>
> MAX_FRAGS + 1?
>
I think MAX_SKB_FRAGS is OK. If the available pool = MAX_SKB_FRAGS we should be
able to wake the queue.
>> + netif_tx_wake_queue(netif_txq);
>> + }
>> +
>> + return NETDEV_TX_OK;
>
>
>> +static int emac_napi_rx_poll(struct napi_struct *napi_rx, int budget)
>> +{
>> + struct prueth_emac *emac = prueth_napi_to_emac(napi_rx);
>> + int rx_flow = PRUETH_RX_FLOW_DATA;
>> + int flow = PRUETH_MAX_RX_FLOWS;
>> + int num_rx = 0;
>> + int cur_budget;
>> + int ret;
>> +
>> + while (flow--) {
>> + cur_budget = budget - num_rx;
>> +
>> + while (cur_budget--) {
>> + ret = emac_rx_packet(emac, flow);
>> + if (ret)
>> + break;
>> + num_rx++;
>> + }
>> +
>> + if (num_rx >= budget)
>> + break;
>> + }
>> +
>> + if (num_rx < budget) {
>> + napi_complete(napi_rx);
>
> Prefer using napi_complete_done()
>
Sure.
>> + enable_irq(emac->rx_chns.irq[rx_flow]);
>> + }
>> +
>> + return num_rx;
>> +}
>
>> +static void emac_ndo_tx_timeout(struct net_device *ndev, unsigned int txqueue)
>> +{
>> + struct prueth_emac *emac = netdev_priv(ndev);
>> +
>> + if (netif_msg_tx_err(emac))
>> + netdev_err(ndev, "xmit timeout");
>
> Core already prints something, you can drop this.
>
Sure, I will drop this print.
>> + ndev->stats.tx_errors++;
>> +}
>
>> +static void emac_ndo_set_rx_mode_work(struct work_struct *work)
>> +{
>> + struct prueth_emac *emac = container_of(work, struct prueth_emac, rx_mode_work);
>> + struct net_device *ndev = emac->ndev;
>> + bool promisc, allmulti;
>> +
>> + if (!netif_running(ndev))
>> + return;
>> +
>> + promisc = ndev->flags & IFF_PROMISC;
>> + allmulti = ndev->flags & IFF_ALLMULTI;
>> + emac_set_port_state(emac, ICSSG_EMAC_PORT_UC_FLOODING_DISABLE);
>> + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_DISABLE);
>> +
>> + if (promisc) {
>> + emac_set_port_state(emac, ICSSG_EMAC_PORT_UC_FLOODING_ENABLE);
>> + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE);
>> + return;
>> + }
>> +
>> + if (allmulti) {
>> + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE);
>> + return;
>> + }
>> +
>> + if (!netdev_mc_empty(ndev)) {
>> + emac_set_port_state(emac, ICSSG_EMAC_PORT_MC_FLOODING_ENABLE);
>> + return;
>> + }
>> +}
>
> There's no need for locking in this work?
>
No I don't think any lock is required here. emac_set_port_state() aquires lock
before updating port status. Also emac_ndo_set_rx_mode_work() is scheduled by a
singlethreaded workqueue.
>> + netif_napi_add(ndev, &emac->napi_rx,
>> + emac_napi_rx_poll);
>
> nit: fits on a line
Sure I will move it to one line.
>
>> +static struct platform_driver prueth_driver = {
>> + .probe = prueth_probe,
>> + .remove = prueth_remove,
>
> Please use .remove_new (which has a void return).
Sure I will use .remove_new instead of .remove
Please let me know if this looks ok to you. I will try to address these and
send next revision.
--
Thanks and Regards,
Danish.
