[PATCH net-next 0/6] net: hip04: fix some problem for hip04 drivers
Ding Tianhong
dingtianhong at huawei.com
Wed Apr 15 23:28:14 PDT 2015
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 at arndb.de>
>
> 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);
> }
>
>
> .
>
More information about the linux-arm-kernel
mailing list