Re: [PATCH v3 1/1 net-next] net: fec: add napi support to improve proformance

2013/1/25 Waskiewicz Jr, Peter P [off-list ref]:

On Fri, Jan 25, 2013 at 09:29:03AM +0800, Frank Li wrote:

[...]

quoted

 static void
+fec_enet_rx_int_enable(struct net_device *ndev, bool enabled)
+{
+     struct fec_enet_private *fep = netdev_priv(ndev);
+     uint    int_events;
+
+     int_events = readl(fep->hwp + FEC_IMASK);
+     if (enabled)
+             int_events |= FEC_ENET_RXF;
+     else
+             int_events &= ~FEC_ENET_RXF;
+     writel(int_events, fep->hwp + FEC_IMASK);
+}

You hold your hw_lock when this is called to disable the interrupt, but
not when you re-enable it.  See below.

[...]

quoted

@@ -813,7 +831,14 @@ fec_enet_interrupt(int irq, void *dev_id)

              if (int_events & FEC_ENET_RXF) {
                      ret = IRQ_HANDLED;
-                     fec_enet_rx(ndev);
+
+                     spin_lock(&fep->hw_lock);
+                     /* Disable the RX interrupt */
+                     if (napi_schedule_prep(&fep->napi)) {
+                             fec_enet_rx_int_enable(ndev, false);
+                             __napi_schedule(&fep->napi);
+                     }
+                     spin_unlock(&fep->hw_lock);
              }

              /* Transmit OK, or non-fatal error. Update the buffer

@@ -834,7 +859,16 @@ fec_enet_interrupt(int irq, void *dev_id)
      return ret;
 }

-
+static int fec_enet_rx_napi(struct napi_struct *napi, int budget)
+{
+     struct net_device *ndev = napi->dev;
+     int pkgs = fec_enet_rx(ndev, budget);
+     if (pkgs < budget) {
+             napi_complete(napi);
+             fec_enet_rx_int_enable(ndev, true);

Here you're not locking when re-enabling, but you do in the disable path.

Also, when you're disabling interrupts above, you're doing that in your
HW interrupt handler, you should be using spin_lock_irqsave()/irq_restore().

Good catch. Thanks!

Cheers,
-PJ