Re: [PATCH net-next 0/6] net: hip04: fix some problem for hip04 drivers

[Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>]

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-r2nGTMty4D4@public.gmane.org>
> 
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html