| Message ID | 20100317235505.GA6674@electric-eye.fr.zoreil.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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;
>
Hello, Patched r8169 (both Eric's and Francois' patches are applied). /*pktgen localhost2localhost*/ [ 498.818640] pktgen 2.72: Packet Generator for packet performance testing. [ 568.999957] ------------[ cut here ]------------ [ 568.999969] WARNING: at net/sched/sch_generic.c:255 dev_watchdog+0xc1/0x125() [ 568.999973] Hardware name: F3JC [ 568.999976] NETDEV WATCHDOG: eth0 (r8169): transmit queue 0 timed out [ 568.999979] Modules linked in: pktgen snd_hwdep snd_hda_codec_si3054 snd_hda_codec_realtek asus_laptop sparse_keymap sdhci_pci sdhci snd_hda_intel mmc_core led_class snd_hda_codec snd_pcm snd_timer psmouse snd soundcore snd_page_alloc sg evdev i2c_i801 rng_core serio_raw r8169 mii uhci_hcd sr_mod ehci_hcd cdrom sd_mod usbcore ata_piix [ 569.000029] Pid: 3350, comm: kpktgend_0 Tainted: G W 2.6.34-rc1-dbg-git6-r8169 #48 [ 569.000033] Call Trace: [ 569.000041] [<c102e293>] warn_slowpath_common+0x65/0x7c [ 569.000046] [<c126c024>] ? dev_watchdog+0xc1/0x125 [ 569.000051] [<c102e2de>] warn_slowpath_fmt+0x24/0x27 [ 569.000056] [<c126c024>] dev_watchdog+0xc1/0x125 [ 569.000063] [<c1036afb>] ? run_timer_softirq+0x120/0x1eb [ 569.000069] [<c1036b51>] run_timer_softirq+0x176/0x1eb [ 569.000074] [<c1036afb>] ? run_timer_softirq+0x120/0x1eb [ 569.000079] [<c126bf63>] ? dev_watchdog+0x0/0x125 [ 569.000084] [<c1032d39>] __do_softirq+0x8d/0x117 [ 569.000089] [<c1032dee>] do_softirq+0x2b/0x43 [ 569.000097] [<f809510d>] ? pktgen_xmit+0xdb7/0xe8e [pktgen] [ 569.000102] [<c1032e9c>] _local_bh_enable_ip+0x88/0xb0 [ 569.000107] [<c1032ecc>] local_bh_enable_ip+0x8/0xa [ 569.000114] [<c12c83a0>] _raw_spin_unlock_bh+0x2f/0x32 [ 569.000120] [<f809510d>] pktgen_xmit+0xdb7/0xe8e [pktgen] [ 569.000129] [<f91674a9>] ? rtl8169_start_xmit+0x0/0x304 [r8169] [ 569.000136] [<c1183940>] ? trace_hardirqs_on_thunk+0xc/0x10 [ 569.000143] [<c105012d>] ? print_lock_contention_bug+0x11/0xb2 [ 569.000150] [<f80935ca>] ? spin_lock+0x8/0xa [pktgen] [ 569.000156] [<f80954a8>] pktgen_thread_worker+0x18d/0x631 [pktgen] [ 569.000163] [<c103f931>] ? autoremove_wake_function+0x0/0x2f [ 569.000169] [<c103f931>] ? autoremove_wake_function+0x0/0x2f [ 569.000175] [<f809531b>] ? pktgen_thread_worker+0x0/0x631 [pktgen] [ 569.000180] [<c103f60e>] kthread+0x6a/0x6f [ 569.000185] [<c103f5a4>] ? kthread+0x0/0x6f [ 569.000191] [<c1002e42>] kernel_thread_helper+0x6/0x1a [ 569.000195] ---[ end trace a22d306b065d4a68 ]--- Sergey On (03/18/10 00:55), Francois Romieu wrote: > > I suspect lot of work is needed on this driver to make it working, but I > > dont have a machine with said adapter. > > 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; >
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;