From: Sergey Senozhatsky <sergey.senozhatsky@gmail•com>
To: Francois Romieu <romieu@fr•zoreil.com>
Cc: Eric Dumazet <eric.dumazet@gmail•com>,
Oleg Nesterov <oleg@redhat•com>,
David Miller <davem@davemloft•net>,
netdev@vger•kernel.org
Subject: Re: [PATCH] r8169: Fix rtl8169_rx_interrupt()
Date: Thu, 18 Mar 2010 14:32:32 +0200 [thread overview]
Message-ID: <20100318123232.GC3364@swordfish.minsk.epam.com> (raw)
In-Reply-To: <20100317235505.GA6674@electric-eye.fr.zoreil.com>
[-- Attachment #1: Type: text/plain, Size: 5145 bytes --]
Hello,
This hunk is rejected. I suppose I should apply patch against unpatched version (Eric's patch).
Correct? /* Eric's patch does make sense to my mind. */
@@ -4604,22 +4601,19 @@
void __iomem *ioaddr = tp->mmio_addr;
int work_done;
+
+ RTL_W16(IntrStatus, tp->napi_event);
+
work_done = rtl8169_rx_interrupt(dev, tp, ioaddr, (u32) budget);
rtl8169_tx_interrupt(dev, tp, ioaddr);
if (work_done < budget) {
napi_complete(napi);
- /* We need for force the visibility of tp->intr_mask
- * for other CPUs, as we can loose an MSI interrupt
- * and potentially wait for a retransmit timeout if we don't.
- * The posted write to IntrMask is safe, as it will
- * eventually make it to the chip and we won't loose anything
- * until it does.
- */
- tp->intr_mask = 0xffff;
- smp_wmb();
RTL_W16(IntrMask, tp->intr_event);
+ mmiowb();
+
+ rtl_napi_cond_schedule(tp, RTL_R16(IntrStatus));
}
Sergey
On (03/18/10 00:55), Francois Romieu wrote:
> This one should help too if Sergey owns a (MSI) 8168.
>
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index dfc3573..721e7f3 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -4532,21 +4532,39 @@ static int rtl8169_rx_interrupt(struct net_device *dev,
> return count;
> }
>
> +static void rtl_napi_cond_schedule(struct rtl8169_private *tp, u16 status)
> +{
> + if (status & tp->napi_event) {
> + void __iomem *ioaddr = tp->mmio_addr;
> +
> + RTL_W16(IntrMask, tp->intr_event & ~tp->napi_event);
> + mmiowb();
> + napi_schedule(&tp->napi);
> + }
> +}
> +
> static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
> {
> struct net_device *dev = dev_instance;
> struct rtl8169_private *tp = netdev_priv(dev);
> void __iomem *ioaddr = tp->mmio_addr;
> int handled = 0;
> - int status;
> + u16 status;
>
> /* loop handling interrupts until we have no new ones or
> * we hit a invalid/hotplug case.
> */
> status = RTL_R16(IntrStatus);
> while (status && status != 0xffff) {
> + u16 acked;
> +
> handled = 1;
>
> + acked = (status & RxFIFOOver) ? (status | RxOverflow) : status;
> + acked &= ~tp->napi_event;
> +
> + RTL_W16(IntrStatus, acked);
> +
> /* Handle all of the error cases first. These will reset
> * the chip, so just exit the loop.
> */
> @@ -4557,7 +4575,7 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>
> /* Work around for rx fifo overflow */
> if (unlikely(status & RxFIFOOver) &&
> - (tp->mac_version == RTL_GIGA_MAC_VER_11)) {
> + (tp->mac_version == RTL_GIGA_MAC_VER_11)) {
> netif_stop_queue(dev);
> rtl8169_tx_timeout(dev);
> break;
> @@ -4571,30 +4589,9 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
> if (status & LinkChg)
> rtl8169_check_link_status(dev, tp, ioaddr);
>
> - /* We need to see the lastest version of tp->intr_mask to
> - * avoid ignoring an MSI interrupt and having to wait for
> - * another event which may never come.
> - */
> - smp_rmb();
> - if (status & tp->intr_mask & tp->napi_event) {
> - RTL_W16(IntrMask, tp->intr_event & ~tp->napi_event);
> - tp->intr_mask = ~tp->napi_event;
> -
> - if (likely(napi_schedule_prep(&tp->napi)))
> - __napi_schedule(&tp->napi);
> - else
> - netif_info(tp, intr, dev,
> - "interrupt %04x in poll\n", status);
> - }
> + rtl_napi_cond_schedule(tp, status);
>
> - /* We only get a new MSI interrupt when all active irq
> - * sources on the chip have been acknowledged. So, ack
> - * everything we've seen and check if new sources have become
> - * active to avoid blocking all interrupts from the chip.
> - */
> - RTL_W16(IntrStatus,
> - (status & RxFIFOOver) ? (status | RxOverflow) : status);
> - status = RTL_R16(IntrStatus);
> + break;
> }
>
> return IRQ_RETVAL(handled);
> @@ -4607,22 +4604,19 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
> void __iomem *ioaddr = tp->mmio_addr;
> int work_done;
>
> +
> + RTL_W16(IntrStatus, tp->napi_event);
> +
> work_done = rtl8169_rx_interrupt(dev, tp, ioaddr, (u32) budget);
> rtl8169_tx_interrupt(dev, tp, ioaddr);
>
> if (work_done < budget) {
> napi_complete(napi);
>
> - /* We need for force the visibility of tp->intr_mask
> - * for other CPUs, as we can loose an MSI interrupt
> - * and potentially wait for a retransmit timeout if we don't.
> - * The posted write to IntrMask is safe, as it will
> - * eventually make it to the chip and we won't loose anything
> - * until it does.
> - */
> - tp->intr_mask = 0xffff;
> - smp_wmb();
> RTL_W16(IntrMask, tp->intr_event);
> + mmiowb();
> +
> + rtl_napi_cond_schedule(tp, RTL_R16(IntrStatus));
> }
>
> return work_done;
>
[-- Attachment #2: Type: application/pgp-signature, Size: 316 bytes --]
next prev parent reply other threads:[~2010-03-18 12:31 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20100307192305.GA598@elte.hu>
2010-03-08 12:51 ` inconsistent lock state Oleg Nesterov
2010-03-15 21:01 ` Eric Dumazet
2010-03-15 21:09 ` David Miller
2010-03-16 0:33 ` [PATCH] r8169: Fix rtl8169_rx_interrupt() Eric Dumazet
2010-03-16 6:50 ` Sergey Senozhatsky
2010-03-16 7:12 ` Eric Dumazet
2010-03-16 14:50 ` Sergey Senozhatsky
2010-03-16 15:00 ` Sergey Senozhatsky
2010-03-16 15:05 ` Eric Dumazet
2010-03-16 15:10 ` Sergey Senozhatsky
2010-03-16 15:20 ` Eric Dumazet
2010-03-16 18:26 ` Sergey Senozhatsky
2010-03-16 18:48 ` Eric Dumazet
2010-03-16 19:02 ` Sergey Senozhatsky
2010-03-17 7:25 ` Sergey Senozhatsky
2010-03-17 7:37 ` Eric Dumazet
2010-03-17 7:58 ` Sergey Senozhatsky
2010-03-17 10:58 ` Sergey Senozhatsky
2010-03-17 13:54 ` Eric Dumazet
2010-03-18 12:28 ` Sergey Senozhatsky
2010-03-17 23:55 ` Francois Romieu
2010-03-18 12:32 ` Sergey Senozhatsky [this message]
2010-03-18 13:31 ` Sergey Senozhatsky
2010-03-25 11:30 ` Sergey Senozhatsky
2010-03-25 13:19 ` Eric Dumazet
2010-03-25 13:48 ` Sergey Senozhatsky
2010-03-26 20:29 ` François Romieu
2010-03-31 12:08 ` Eric Dumazet
2010-04-02 1:43 ` David Miller
2010-04-02 13:51 ` Eric Dumazet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100318123232.GC3364@swordfish.minsk.epam.com \
--to=sergey.senozhatsky@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=oleg@redhat$(echo .)com \
--cc=romieu@fr$(echo .)zoreil.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox