On Wednesday 15 April 2015 20:30:02 Ding Tianhong wrote: > The patches series was used to fix the issues of the hip04 driver, and added > some optimizations according to some good suggestion. > > Thanks, that looks much better, except for patch 4 that I commented on. I had at some point sent a patch that is archived at http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318120.html I believe that one is also still needed, but I have not tested whether it is correct. Can you have a look at the patch from back then and see if it works, of if you find something wrong about it? I'm sending the unmodified patch from then here again for you to apply or comment. It will have to be rebased on top of your current changes. Arnd 8<---- Subject: [PATCH] net/hip04: refactor interrupt masking The hip04 ethernet driver currently acknowledges all interrupts directly in the interrupt handler, and leaves all interrupts except the RX data enabled the whole time. This causes multiple problems: - When more packets come in between the original interrupt and the NAPI poll function, we will get an extraneous RX interrupt as soon as interrupts are enabled again. - The two error interrupts are by definition combining all errors that may have happened since the last time they were handled, but just acknowledging the irq without dealing with the cause of the condition makes it come back immediately. In particular, when NAPI is intentionally stalling the rx queue, this causes a storm of "rx dropped" messages. - The access to the 'reg_inten' field in hip_priv is used for serializing access, but is in fact racy itself. To deal with these issues, the driver is changed to only acknowledge the IRQ at the point when it is being handled. The RX interrupts now get acked right before reading the number of received frames to keep spurious interrupts to the absolute minimum without losing interrupts. As there is no tx-complete interrupt, only an informational tx-dropped one, we now ack that in the tx reclaim handler, hoping that clearing the descriptors has also eliminated the error condition. The only place that reads the reg_inten now just relies on napi_schedule_prep() to return whether NAPI is active or not, and the poll() function is then going to ack and reenabled the IRQ if necessary. Finally, when we disable interrupts, they are now all disabled together, in order to simplify the logic and to avoid getting rx-dropped interrupts when NAPI is in control of the rx queue. Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c index 525214ef5984..83773247a368 100644 --- a/drivers/net/ethernet/hisilicon/hip04_eth.c +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c @@ -56,6 +56,8 @@ #define RCV_DROP BIT(7) #define TX_DROP BIT(6) #define DEF_INT_ERR (RCV_NOBUF | RCV_DROP | TX_DROP) +#define DEF_INT_RX (RCV_INT | RCV_NOBUF | RCV_DROP) +#define DEF_INT_TX (TX_DROP) #define DEF_INT_MASK (RCV_INT | DEF_INT_ERR) /* TX descriptor config */ @@ -154,7 +156,6 @@ struct hip04_priv { unsigned int port; unsigned int speed; unsigned int duplex; - unsigned int reg_inten; struct napi_struct napi; struct net_device *ndev; @@ -303,17 +304,15 @@ static void hip04_mac_enable(struct net_device *ndev) val |= GE_RX_PORT_EN | GE_TX_PORT_EN; writel_relaxed(val, priv->base + GE_PORT_EN); - /* clear rx int */ - val = RCV_INT; - writel_relaxed(val, priv->base + PPE_RINT); + /* clear prior interrupts */ + writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT); /* config recv int */ val = GE_RX_INT_THRESHOLD | GE_RX_TIMEOUT; writel_relaxed(val, priv->base + PPE_CFG_RX_PKT_INT); /* enable interrupt */ - priv->reg_inten = DEF_INT_MASK; - writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN); + writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN); } static void hip04_mac_disable(struct net_device *ndev) @@ -322,8 +321,7 @@ static void hip04_mac_disable(struct net_device *ndev) u32 val; /* disable int */ - priv->reg_inten &= ~(DEF_INT_MASK); - writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN); + writel_relaxed(0, priv->base + PPE_INTEN); /* disable tx & rx */ val = readl_relaxed(priv->base + GE_PORT_EN); @@ -403,6 +401,8 @@ static int hip04_tx_reclaim(struct net_device *ndev, bool force) priv->tx_tail = tx_tail; smp_wmb(); /* Ensure tx_tail visible to xmit */ + writel_relaxed(DEF_INT_TX, priv->base + PPE_RINT); + out: if (pkts_compl || bytes_compl) netdev_completed_queue(ndev, pkts_compl, bytes_compl); @@ -458,9 +458,7 @@ static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev) if (count >= priv->tx_coalesce_frames) { if (napi_schedule_prep(&priv->napi)) { /* disable rx interrupt and timer */ - priv->reg_inten &= ~(RCV_INT); - writel_relaxed(DEF_INT_MASK & ~RCV_INT, - priv->base + PPE_INTEN); + writel_relaxed(0, priv->base + PPE_INTEN); hrtimer_cancel(&priv->tx_coalesce_timer); __napi_schedule(&priv->napi); } @@ -478,7 +476,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget) struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi); struct net_device *ndev = priv->ndev; struct net_device_stats *stats = &ndev->stats; - unsigned int cnt = hip04_recv_cnt(priv); + unsigned int cnt; struct rx_desc *desc; struct sk_buff *skb; unsigned char *buf; @@ -489,6 +487,10 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget) u16 len; u32 err; + /* acknowledge interrupts and read status */ + writel_relaxed(DEF_INT_RX, priv->base + PPE_RINT); + cnt = hip04_recv_cnt(priv); + while (cnt && !last) { buf = priv->rx_buf[priv->rx_head]; skb = build_skb(buf, priv->rx_buf_size); @@ -539,11 +541,8 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget) cnt = hip04_recv_cnt(priv); } - if (!(priv->reg_inten & RCV_INT)) { - /* enable rx interrupt */ - priv->reg_inten |= RCV_INT; - writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN); - } + /* enable rx interrupt */ + writel_relaxed(DEF_INT_MASK, priv->base + PPE_INTEN); napi_complete(napi); done: /* clean up tx descriptors and start a new timer if necessary */ @@ -564,23 +563,21 @@ static irqreturn_t hip04_mac_interrupt(int irq, void *dev_id) if (!ists) return IRQ_NONE; - writel_relaxed(DEF_INT_MASK, priv->base + PPE_RINT); - if (unlikely(ists & DEF_INT_ERR)) { - if (ists & (RCV_NOBUF | RCV_DROP)) + if (ists & (RCV_NOBUF | RCV_DROP)) { stats->rx_errors++; stats->rx_dropped++; - netdev_err(ndev, "rx drop\n"); + netdev_dbg(ndev, "rx drop\n"); + } if (ists & TX_DROP) { stats->tx_dropped++; - netdev_err(ndev, "tx drop\n"); + netdev_dbg(ndev, "tx drop\n"); } } - if (ists & RCV_INT && napi_schedule_prep(&priv->napi)) { - /* disable rx interrupt */ - priv->reg_inten &= ~(RCV_INT); - writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN); + if (napi_schedule_prep(&priv->napi)) { + /* disable interrupt */ + writel_relaxed(0, priv->base + PPE_INTEN); hrtimer_cancel(&priv->tx_coalesce_timer); __napi_schedule(&priv->napi); } @@ -596,8 +593,7 @@ enum hrtimer_restart tx_done(struct hrtimer *hrtimer) if (napi_schedule_prep(&priv->napi)) { /* disable rx interrupt */ - priv->reg_inten &= ~(RCV_INT); - writel_relaxed(DEF_INT_MASK & ~RCV_INT, priv->base + PPE_INTEN); + writel_relaxed(0, priv->base + PPE_INTEN); __napi_schedule(&priv->napi); } -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
