Re: [PATCH v3] net: add Faraday FTMAC100 10/100 Ethernet driver

Dear Joe,

On Fri, Jan 21, 2011 at 3:00 AM, Joe Perches [off-list ref] wrote:

On Thu, 2011-01-20 at 23:30 +0800, Po-Yu Chuang wrote:

quoted

 drivers/net/ftmac100.c | 1243 ++++++++++++++++++++++++++++++++++++++++++++++++

[]

quoted

+/******************************************************************************
+ * struct napi_struct functions
+ *****************************************************************************/
+static int ftmac100_poll(struct napi_struct *napi, int budget)
+{
+     struct ftmac100 *priv = container_of(napi, struct ftmac100, napi);
+     struct net_device *netdev = priv->netdev;
+     unsigned int status;
+     bool completed = true;
+     int rx = 0;
+
+     status = ioread32(priv->base + FTMAC100_OFFSET_ISR);
+
+     if (status & (FTMAC100_INT_RPKT_FINISH | FTMAC100_INT_NORXBUF)) {
+             /*
+              * FTMAC100_INT_RPKT_FINISH:
+              *      RX DMA has received packets into RX buffer successfully
+              *
+              * FTMAC100_INT_NORXBUF:
+              *      RX buffer unavailable
+              */
+             bool retry;
+
+             do {
+                     retry = ftmac100_rx_packet(priv, &rx);
+             } while (retry && rx < budget);
+
+             if (retry && rx == budget)
+                     completed = false;

Is it useful to retry the NORXBUF case?

The idea is that if I miss packet finished interrupts (then rx buffers used up),
I should retrieve the received packets ASAP to free buffers to HW.
Maybe this is really unnecessary.
I am not quite sure, but I'll do your advice now.

quoted

+     }
+
+     if (status & FTMAC100_INT_NORXBUF) {
+             /* RX buffer unavailable */
+             if (net_ratelimit())
+                     netdev_info(netdev, "INT_NORXBUF\n");
+
+             netdev->stats.rx_over_errors++;
+     }

Perhaps this "if (status & FTMAC100_INT_NORXBUF)" block should be
moved into the test block above it before the retry?

Since status is not changed in the function, it does not matter where
the test is.
But I agree that it is better to handle error cases earlier.

It's possible to miss multiple states because of the ratelimit.

If multiple ISR status bits are possible, it might be better to
combine all netdev_info uses into a single call.

Something like:

       if ((status & (FTMAC100_INT_NORXBUF | FTMAC100_INT_RPKT_LOST |
                      FTMAC100_INT_AHB_ERR | FTMAC100_INT_PHYSTS_CHG)) &&
           net_ratelimit())
               netdev_info(netdev, "ISR status: %x%s%s%s%s\n",
                           status & FTMAC100_INT_NORXBUF ? ": INT_NORXBUF" : "",
                           status & FTMAC100_INT_RPKT_LOST ? ": INT_RPKT_LOST" : "",
                           status & FTMAC100_INT_AHB_ERR ? ": INT_AHB_ERR" : "",
                           status & FTMAC100_INT_PHYSTS_CHG ? " : INT_PHYSTS_CHG" : "");

Agree. Thanks.

best regards,
Po-Yu Chuang