Hi Florian,

On Tue, Jan 31, 2017 at 12:31 PM, Florian Fainelli [off-list ref] wrote:

On 01/31/2017 11:03 AM, Iyappan Subramanian wrote:

quoted

This patch adds,

     - probe, remove, shutdown
     - open, close and stats
     - create and delete ring
     - request and delete irq

Signed-off-by: Iyappan Subramanian <redacted>
Signed-off-by: Keyur Chudgar <redacted>
---

quoted

+static void xge_delete_desc_rings(struct net_device *ndev)
+{
+     struct xge_pdata *pdata = netdev_priv(ndev);
+     struct device *dev = &pdata->pdev->dev;
+     struct xge_desc_ring *ring;
+
+     ring = pdata->tx_ring;
+     if (ring) {
+             if (ring->skbs)
+                     devm_kfree(dev, ring->skbs);
+             if (ring->pkt_bufs)
+                     devm_kfree(dev, ring->pkt_bufs);
+             devm_kfree(dev, ring);
+     }

The very fact that you have to do the devm_kfree suggests that the way
you manage the lifetime of the ring is not appropriate, and in fact, if
we look at how xge_create_desc_ring() is called, in the driver's probe
function indicates that if the network interface is never openeded, we
are just wasting memory sitting there and doing nothing. You should
consider moving this to the ndo_open(), resp. ndo_close() functions to
optimize memory consumption wrt. the network interface state.

I will move these to open and close functions and will use dma_zalloc() APIs.

quoted

+
+     ring = pdata->rx_ring;
+     if (ring) {
+             if (ring->skbs)
+                     devm_kfree(dev, ring->skbs);
+             devm_kfree(dev, ring);
+     }
+}
+
+static struct xge_desc_ring *xge_create_desc_ring(struct net_device *ndev)
+{
+     struct xge_pdata *pdata = netdev_priv(ndev);
+     struct device *dev = &pdata->pdev->dev;
+     struct xge_desc_ring *ring;
+     u16 size;
+
+     ring = devm_kzalloc(dev, sizeof(struct xge_desc_ring), GFP_KERNEL);
+     if (!ring)
+             return NULL;
+
+     ring->ndev = ndev;
+
+     size = XGENE_ENET_DESC_SIZE * XGENE_ENET_NUM_DESC;
+     ring->desc_addr = dmam_alloc_coherent(dev, size, &ring->dma_addr,
+                                           GFP_KERNEL | __GFP_ZERO);

There is no dmam_zalloc_coherent()? Then again, that seems to be a
candidate for dma_zalloc_coherent() and moving this to the ndo_open()
function.

quoted

+     if (!ring->desc_addr) {
+             devm_kfree(dev, ring);
+             return NULL;
+     }
+
+     xge_setup_desc(ring);
+
+     return ring;
+}
+
+static int xge_refill_buffers(struct net_device *ndev, u32 nbuf)
+{
+     struct xge_pdata *pdata = netdev_priv(ndev);
+     struct xge_desc_ring *ring = pdata->rx_ring;
+     const u8 slots = XGENE_ENET_NUM_DESC - 1;
+     struct device *dev = &pdata->pdev->dev;
+     struct xge_raw_desc *raw_desc;
+     u64 addr_lo, addr_hi;
+     u8 tail = ring->tail;
+     struct sk_buff *skb;
+     dma_addr_t dma_addr;
+     u16 len;
+     int i;
+
+     for (i = 0; i < nbuf; i++) {
+             raw_desc = &ring->raw_desc[tail];
+
+             len = XGENE_ENET_STD_MTU;
+             skb = netdev_alloc_skb(ndev, len);
+             if (unlikely(!skb))
+                     return -ENOMEM;

Are not you leaving holes in your RX ring if you do that?

No.  The probe will fail and clean up the unused buffers.

quoted

+
+             dma_addr = dma_map_single(dev, skb->data, len, DMA_FROM_DEVICE);
+             if (dma_mapping_error(dev, dma_addr)) {
+                     netdev_err(ndev, "DMA mapping error\n");
+                     dev_kfree_skb_any(skb);
+                     return -EINVAL;
+             }

quoted

+static void xge_timeout(struct net_device *ndev)
+{
+     struct xge_pdata *pdata = netdev_priv(ndev);
+     struct netdev_queue *txq;
+
+     xge_mac_reset(pdata);
+
+     txq = netdev_get_tx_queue(ndev, 0);
+     txq->trans_start = jiffies;
+     netif_tx_start_queue(txq);

It most likely is not that simple, don't you want to walk the list of
pending transmissed SKBs and free them all?

I'll add more exhaustive clean up and restart of Tx hardware.

quoted

+}
+
+static void xge_get_stats64(struct net_device *ndev,
+                         struct rtnl_link_stats64 *storage)
+{
+     struct xge_pdata *pdata = netdev_priv(ndev);
+     struct xge_stats *stats = &pdata->stats;
+
+     storage->tx_packets += stats->tx_packets;
+     storage->tx_bytes += stats->tx_bytes;
+
+     storage->rx_packets += stats->rx_packets;
+     storage->rx_bytes += stats->rx_bytes;

Pretty sure you need some synchronization primitives here for non 64-bit
architectures (maybe this driver is not used outside of 64-bit, but still).

Synchronization primitives are not required for this driver, yet!

quoted

+
+     ndev->hw_features = ndev->features;
+
+     ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
+     if (ret) {
+             netdev_err(ndev, "No usable DMA configuration\n");
+             goto err;
+     }
+
+     ret = xge_init_hw(ndev);
+     if (ret)
+             goto err;

Missing netif_carrier_off() right before the register_netdev().

I'll add them.

quoted

+
+     ret = register_netdev(ndev);
+     if (ret) {
+             netdev_err(ndev, "Failed to register netdev\n");
+             goto err;
+     }

--
Florian