On 2015/4/15 22:25, Arnd Bergmann wrote: > 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 will check and fix it. > 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 > Thanks for the suggestion, I notices that I miss this patch in my series from the maillist, I need time to check the code and try to test it, thanks for the work.:) Ding > 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
