2014-02-20 07:13:18

by Dan Williams

[permalink] [raw]
Subject: [PATCH] ioat: fix tasklet tear down

Since commit 77873803363c "net_dma: mark broken" we no longer pin dma
engines active for the network-receive-offload use case. As a result
the ->free_chan_resources() that occurs after the driver self-test no
longer has a NET_DMA induced ->alloc_chan_resources() to back it up. A
late firing irq can lead to ksoftirqd spinning indefinitely due to the
tasklet_disable() performed by ->free_chan_resources(). Only
->alloc_chan_resources() can clear this condition in affected kernels.

This problem has been present since commit 3e037454bcfa "I/OAT: Add
support for MSI and MSI-X" in 2.6.24, but is now exposed. Given the
NET_DMA use case is deprecated we can revisit moving the driver to use
threaded irqs. For now, just tear down the irq and tasklet properly by:

1/ Disable the irq from triggering the tasklet

2/ Disable the irq from re-arming

3/ Flush inflight interrupts

4/ Flush the timer

5/ Flush inflight tasklets

References:
https://lkml.org/lkml/2014/1/27/282
https://lkml.org/lkml/2014/2/19/672

Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: <[email protected]>
Reported-by: Mike Galbraith <[email protected]>
Reported-by: Stanislav Fomichev <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---

Passes my testing, but would appreciate a tested-by.

drivers/dma/ioat/dma.c | 52 +++++++++++++++++++++++++++++++++++++++------
drivers/dma/ioat/dma.h | 1 +
drivers/dma/ioat/dma_v2.c | 11 ++++------
drivers/dma/ioat/dma_v3.c | 3 +++
4 files changed, 54 insertions(+), 13 deletions(-)

diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
index 87529181efcc..4e3549a16132 100644
--- a/drivers/dma/ioat/dma.c
+++ b/drivers/dma/ioat/dma.c
@@ -77,7 +77,8 @@ static irqreturn_t ioat_dma_do_interrupt(int irq, void *data)
attnstatus = readl(instance->reg_base + IOAT_ATTNSTATUS_OFFSET);
for_each_set_bit(bit, &attnstatus, BITS_PER_LONG) {
chan = ioat_chan_by_index(instance, bit);
- tasklet_schedule(&chan->cleanup_task);
+ if (test_bit(IOAT_RUN, &chan->state))
+ tasklet_schedule(&chan->cleanup_task);
}

writeb(intrctrl, instance->reg_base + IOAT_INTRCTRL_OFFSET);
@@ -93,7 +94,8 @@ static irqreturn_t ioat_dma_do_interrupt_msix(int irq, void *data)
{
struct ioat_chan_common *chan = data;

- tasklet_schedule(&chan->cleanup_task);
+ if (test_bit(IOAT_RUN, &chan->state))
+ tasklet_schedule(&chan->cleanup_task);

return IRQ_HANDLED;
}
@@ -116,7 +118,6 @@ void ioat_init_channel(struct ioatdma_device *device, struct ioat_chan_common *c
chan->timer.function = device->timer_fn;
chan->timer.data = data;
tasklet_init(&chan->cleanup_task, device->cleanup_fn, data);
- tasklet_disable(&chan->cleanup_task);
}

/**
@@ -354,13 +355,49 @@ static int ioat1_dma_alloc_chan_resources(struct dma_chan *c)
writel(((u64) chan->completion_dma) >> 32,
chan->reg_base + IOAT_CHANCMP_OFFSET_HIGH);

- tasklet_enable(&chan->cleanup_task);
+ set_bit(IOAT_RUN, &chan->state);
ioat1_dma_start_null_desc(ioat); /* give chain to dma device */
dev_dbg(to_dev(chan), "%s: allocated %d descriptors\n",
__func__, ioat->desccount);
return ioat->desccount;
}

+void ioat_stop(struct ioat_chan_common *chan)
+{
+ struct ioatdma_device *device = chan->device;
+ struct pci_dev *pdev = device->pdev;
+ int chan_id = chan_num(chan);
+ struct msix_entry *msix;
+
+ /* 1/ stop irq from firing tasklets
+ * 2/ stop the tasklet from re-arming irqs
+ */
+ clear_bit(IOAT_RUN, &chan->state);
+
+ /* flush inflight interrupts */
+ switch (device->irq_mode) {
+ case IOAT_MSIX:
+ msix = &device->msix_entries[chan_id];
+ synchronize_irq(msix->vector);
+ break;
+ case IOAT_MSI:
+ case IOAT_INTX:
+ synchronize_irq(pdev->irq);
+ break;
+ default:
+ break;
+ }
+
+ /* flush inflight timers */
+ del_timer_sync(&chan->timer);
+
+ /* flush inflight tasklet runs */
+ tasklet_kill(&chan->cleanup_task);
+
+ /* final cleanup now that everything is quiesced and can't re-arm */
+ device->cleanup_fn((unsigned long) &chan->common);
+}
+
/**
* ioat1_dma_free_chan_resources - release all the descriptors
* @chan: the channel to be cleaned
@@ -379,9 +416,7 @@ static void ioat1_dma_free_chan_resources(struct dma_chan *c)
if (ioat->desccount == 0)
return;

- tasklet_disable(&chan->cleanup_task);
- del_timer_sync(&chan->timer);
- ioat1_cleanup(ioat);
+ ioat_stop(chan);

/* Delay 100ms after reset to allow internal DMA logic to quiesce
* before removing DMA descriptor resources.
@@ -526,8 +561,11 @@ ioat1_dma_prep_memcpy(struct dma_chan *c, dma_addr_t dma_dest,
static void ioat1_cleanup_event(unsigned long data)
{
struct ioat_dma_chan *ioat = to_ioat_chan((void *) data);
+ struct ioat_chan_common *chan = &ioat->base;

ioat1_cleanup(ioat);
+ if (!test_bit(IOAT_RUN, &chan->state))
+ return;
writew(IOAT_CHANCTRL_RUN, ioat->base.reg_base + IOAT_CHANCTRL_OFFSET);
}

diff --git a/drivers/dma/ioat/dma.h b/drivers/dma/ioat/dma.h
index 11fb877ddca9..e982f00a9843 100644
--- a/drivers/dma/ioat/dma.h
+++ b/drivers/dma/ioat/dma.h
@@ -356,6 +356,7 @@ bool ioat_cleanup_preamble(struct ioat_chan_common *chan,
void ioat_kobject_add(struct ioatdma_device *device, struct kobj_type *type);
void ioat_kobject_del(struct ioatdma_device *device);
int ioat_dma_setup_interrupts(struct ioatdma_device *device);
+void ioat_stop(struct ioat_chan_common *chan);
extern const struct sysfs_ops ioat_sysfs_ops;
extern struct ioat_sysfs_entry ioat_version_attr;
extern struct ioat_sysfs_entry ioat_cap_attr;
diff --git a/drivers/dma/ioat/dma_v2.c b/drivers/dma/ioat/dma_v2.c
index 5d3affe7e976..8d1058085eeb 100644
--- a/drivers/dma/ioat/dma_v2.c
+++ b/drivers/dma/ioat/dma_v2.c
@@ -190,8 +190,11 @@ static void ioat2_cleanup(struct ioat2_dma_chan *ioat)
void ioat2_cleanup_event(unsigned long data)
{
struct ioat2_dma_chan *ioat = to_ioat2_chan((void *) data);
+ struct ioat_chan_common *chan = &ioat->base;

ioat2_cleanup(ioat);
+ if (!test_bit(IOAT_RUN, &chan->state))
+ return;
writew(IOAT_CHANCTRL_RUN, ioat->base.reg_base + IOAT_CHANCTRL_OFFSET);
}

@@ -553,10 +556,10 @@ int ioat2_alloc_chan_resources(struct dma_chan *c)
ioat->issued = 0;
ioat->tail = 0;
ioat->alloc_order = order;
+ set_bit(IOAT_RUN, &chan->state);
spin_unlock_bh(&ioat->prep_lock);
spin_unlock_bh(&chan->cleanup_lock);

- tasklet_enable(&chan->cleanup_task);
ioat2_start_null_desc(ioat);

/* check that we got off the ground */
@@ -566,7 +569,6 @@ int ioat2_alloc_chan_resources(struct dma_chan *c)
} while (i++ < 20 && !is_ioat_active(status) && !is_ioat_idle(status));

if (is_ioat_active(status) || is_ioat_idle(status)) {
- set_bit(IOAT_RUN, &chan->state);
return 1 << ioat->alloc_order;
} else {
u32 chanerr = readl(chan->reg_base + IOAT_CHANERR_OFFSET);
@@ -809,11 +811,8 @@ void ioat2_free_chan_resources(struct dma_chan *c)
if (!ioat->ring)
return;

- tasklet_disable(&chan->cleanup_task);
- del_timer_sync(&chan->timer);
- device->cleanup_fn((unsigned long) c);
+ ioat_stop(chan);
device->reset_hw(chan);
- clear_bit(IOAT_RUN, &chan->state);

spin_lock_bh(&chan->cleanup_lock);
spin_lock_bh(&ioat->prep_lock);
diff --git a/drivers/dma/ioat/dma_v3.c b/drivers/dma/ioat/dma_v3.c
index 820817e97e62..b9b38a1cf92f 100644
--- a/drivers/dma/ioat/dma_v3.c
+++ b/drivers/dma/ioat/dma_v3.c
@@ -464,8 +464,11 @@ static void ioat3_cleanup(struct ioat2_dma_chan *ioat)
static void ioat3_cleanup_event(unsigned long data)
{
struct ioat2_dma_chan *ioat = to_ioat2_chan((void *) data);
+ struct ioat_chan_common *chan = &ioat->base;

ioat3_cleanup(ioat);
+ if (!test_bit(IOAT_RUN, &chan->state))
+ return;
writew(IOAT_CHANCTRL_RUN, ioat->base.reg_base + IOAT_CHANCTRL_OFFSET);
}


2014-02-20 09:34:19

by Mike Galbraith

[permalink] [raw]
Subject: Re: [PATCH] ioat: fix tasklet tear down

That was quick. Afflicted box is now a happy camper.

(gee, systemd isn't _completely_ evil, it dug up a bug;)

Tested-by: Mike Galbraith <[email protected]>

On Wed, 2014-02-19 at 23:13 -0800, Dan Williams wrote:
> Since commit 77873803363c "net_dma: mark broken" we no longer pin dma
> engines active for the network-receive-offload use case. As a result
> the ->free_chan_resources() that occurs after the driver self-test no
> longer has a NET_DMA induced ->alloc_chan_resources() to back it up. A
> late firing irq can lead to ksoftirqd spinning indefinitely due to the
> tasklet_disable() performed by ->free_chan_resources(). Only
> ->alloc_chan_resources() can clear this condition in affected kernels.
>
> This problem has been present since commit 3e037454bcfa "I/OAT: Add
> support for MSI and MSI-X" in 2.6.24, but is now exposed. Given the
> NET_DMA use case is deprecated we can revisit moving the driver to use
> threaded irqs. For now, just tear down the irq and tasklet properly by:
>
> 1/ Disable the irq from triggering the tasklet
>
> 2/ Disable the irq from re-arming
>
> 3/ Flush inflight interrupts
>
> 4/ Flush the timer
>
> 5/ Flush inflight tasklets
>
> References:
> https://lkml.org/lkml/2014/1/27/282
> https://lkml.org/lkml/2014/2/19/672
>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: <[email protected]>
> Reported-by: Mike Galbraith <[email protected]>
> Reported-by: Stanislav Fomichev <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
>
> Passes my testing, but would appreciate a tested-by.
>
> drivers/dma/ioat/dma.c | 52 +++++++++++++++++++++++++++++++++++++++------
> drivers/dma/ioat/dma.h | 1 +
> drivers/dma/ioat/dma_v2.c | 11 ++++------
> drivers/dma/ioat/dma_v3.c | 3 +++
> 4 files changed, 54 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
> index 87529181efcc..4e3549a16132 100644
> --- a/drivers/dma/ioat/dma.c
> +++ b/drivers/dma/ioat/dma.c
> @@ -77,7 +77,8 @@ static irqreturn_t ioat_dma_do_interrupt(int irq, void *data)
> attnstatus = readl(instance->reg_base + IOAT_ATTNSTATUS_OFFSET);
> for_each_set_bit(bit, &attnstatus, BITS_PER_LONG) {
> chan = ioat_chan_by_index(instance, bit);
> - tasklet_schedule(&chan->cleanup_task);
> + if (test_bit(IOAT_RUN, &chan->state))
> + tasklet_schedule(&chan->cleanup_task);
> }
>
> writeb(intrctrl, instance->reg_base + IOAT_INTRCTRL_OFFSET);
> @@ -93,7 +94,8 @@ static irqreturn_t ioat_dma_do_interrupt_msix(int irq, void *data)
> {
> struct ioat_chan_common *chan = data;
>
> - tasklet_schedule(&chan->cleanup_task);
> + if (test_bit(IOAT_RUN, &chan->state))
> + tasklet_schedule(&chan->cleanup_task);
>
> return IRQ_HANDLED;
> }
> @@ -116,7 +118,6 @@ void ioat_init_channel(struct ioatdma_device *device, struct ioat_chan_common *c
> chan->timer.function = device->timer_fn;
> chan->timer.data = data;
> tasklet_init(&chan->cleanup_task, device->cleanup_fn, data);
> - tasklet_disable(&chan->cleanup_task);
> }
>
> /**
> @@ -354,13 +355,49 @@ static int ioat1_dma_alloc_chan_resources(struct dma_chan *c)
> writel(((u64) chan->completion_dma) >> 32,
> chan->reg_base + IOAT_CHANCMP_OFFSET_HIGH);
>
> - tasklet_enable(&chan->cleanup_task);
> + set_bit(IOAT_RUN, &chan->state);
> ioat1_dma_start_null_desc(ioat); /* give chain to dma device */
> dev_dbg(to_dev(chan), "%s: allocated %d descriptors\n",
> __func__, ioat->desccount);
> return ioat->desccount;
> }
>
> +void ioat_stop(struct ioat_chan_common *chan)
> +{
> + struct ioatdma_device *device = chan->device;
> + struct pci_dev *pdev = device->pdev;
> + int chan_id = chan_num(chan);
> + struct msix_entry *msix;
> +
> + /* 1/ stop irq from firing tasklets
> + * 2/ stop the tasklet from re-arming irqs
> + */
> + clear_bit(IOAT_RUN, &chan->state);
> +
> + /* flush inflight interrupts */
> + switch (device->irq_mode) {
> + case IOAT_MSIX:
> + msix = &device->msix_entries[chan_id];
> + synchronize_irq(msix->vector);
> + break;
> + case IOAT_MSI:
> + case IOAT_INTX:
> + synchronize_irq(pdev->irq);
> + break;
> + default:
> + break;
> + }
> +
> + /* flush inflight timers */
> + del_timer_sync(&chan->timer);
> +
> + /* flush inflight tasklet runs */
> + tasklet_kill(&chan->cleanup_task);
> +
> + /* final cleanup now that everything is quiesced and can't re-arm */
> + device->cleanup_fn((unsigned long) &chan->common);
> +}
> +
> /**
> * ioat1_dma_free_chan_resources - release all the descriptors
> * @chan: the channel to be cleaned
> @@ -379,9 +416,7 @@ static void ioat1_dma_free_chan_resources(struct dma_chan *c)
> if (ioat->desccount == 0)
> return;
>
> - tasklet_disable(&chan->cleanup_task);
> - del_timer_sync(&chan->timer);
> - ioat1_cleanup(ioat);
> + ioat_stop(chan);
>
> /* Delay 100ms after reset to allow internal DMA logic to quiesce
> * before removing DMA descriptor resources.
> @@ -526,8 +561,11 @@ ioat1_dma_prep_memcpy(struct dma_chan *c, dma_addr_t dma_dest,
> static void ioat1_cleanup_event(unsigned long data)
> {
> struct ioat_dma_chan *ioat = to_ioat_chan((void *) data);
> + struct ioat_chan_common *chan = &ioat->base;
>
> ioat1_cleanup(ioat);
> + if (!test_bit(IOAT_RUN, &chan->state))
> + return;
> writew(IOAT_CHANCTRL_RUN, ioat->base.reg_base + IOAT_CHANCTRL_OFFSET);
> }
>
> diff --git a/drivers/dma/ioat/dma.h b/drivers/dma/ioat/dma.h
> index 11fb877ddca9..e982f00a9843 100644
> --- a/drivers/dma/ioat/dma.h
> +++ b/drivers/dma/ioat/dma.h
> @@ -356,6 +356,7 @@ bool ioat_cleanup_preamble(struct ioat_chan_common *chan,
> void ioat_kobject_add(struct ioatdma_device *device, struct kobj_type *type);
> void ioat_kobject_del(struct ioatdma_device *device);
> int ioat_dma_setup_interrupts(struct ioatdma_device *device);
> +void ioat_stop(struct ioat_chan_common *chan);
> extern const struct sysfs_ops ioat_sysfs_ops;
> extern struct ioat_sysfs_entry ioat_version_attr;
> extern struct ioat_sysfs_entry ioat_cap_attr;
> diff --git a/drivers/dma/ioat/dma_v2.c b/drivers/dma/ioat/dma_v2.c
> index 5d3affe7e976..8d1058085eeb 100644
> --- a/drivers/dma/ioat/dma_v2.c
> +++ b/drivers/dma/ioat/dma_v2.c
> @@ -190,8 +190,11 @@ static void ioat2_cleanup(struct ioat2_dma_chan *ioat)
> void ioat2_cleanup_event(unsigned long data)
> {
> struct ioat2_dma_chan *ioat = to_ioat2_chan((void *) data);
> + struct ioat_chan_common *chan = &ioat->base;
>
> ioat2_cleanup(ioat);
> + if (!test_bit(IOAT_RUN, &chan->state))
> + return;
> writew(IOAT_CHANCTRL_RUN, ioat->base.reg_base + IOAT_CHANCTRL_OFFSET);
> }
>
> @@ -553,10 +556,10 @@ int ioat2_alloc_chan_resources(struct dma_chan *c)
> ioat->issued = 0;
> ioat->tail = 0;
> ioat->alloc_order = order;
> + set_bit(IOAT_RUN, &chan->state);
> spin_unlock_bh(&ioat->prep_lock);
> spin_unlock_bh(&chan->cleanup_lock);
>
> - tasklet_enable(&chan->cleanup_task);
> ioat2_start_null_desc(ioat);
>
> /* check that we got off the ground */
> @@ -566,7 +569,6 @@ int ioat2_alloc_chan_resources(struct dma_chan *c)
> } while (i++ < 20 && !is_ioat_active(status) && !is_ioat_idle(status));
>
> if (is_ioat_active(status) || is_ioat_idle(status)) {
> - set_bit(IOAT_RUN, &chan->state);
> return 1 << ioat->alloc_order;
> } else {
> u32 chanerr = readl(chan->reg_base + IOAT_CHANERR_OFFSET);
> @@ -809,11 +811,8 @@ void ioat2_free_chan_resources(struct dma_chan *c)
> if (!ioat->ring)
> return;
>
> - tasklet_disable(&chan->cleanup_task);
> - del_timer_sync(&chan->timer);
> - device->cleanup_fn((unsigned long) c);
> + ioat_stop(chan);
> device->reset_hw(chan);
> - clear_bit(IOAT_RUN, &chan->state);
>
> spin_lock_bh(&chan->cleanup_lock);
> spin_lock_bh(&ioat->prep_lock);
> diff --git a/drivers/dma/ioat/dma_v3.c b/drivers/dma/ioat/dma_v3.c
> index 820817e97e62..b9b38a1cf92f 100644
> --- a/drivers/dma/ioat/dma_v3.c
> +++ b/drivers/dma/ioat/dma_v3.c
> @@ -464,8 +464,11 @@ static void ioat3_cleanup(struct ioat2_dma_chan *ioat)
> static void ioat3_cleanup_event(unsigned long data)
> {
> struct ioat2_dma_chan *ioat = to_ioat2_chan((void *) data);
> + struct ioat_chan_common *chan = &ioat->base;
>
> ioat3_cleanup(ioat);
> + if (!test_bit(IOAT_RUN, &chan->state))
> + return;
> writew(IOAT_CHANCTRL_RUN, ioat->base.reg_base + IOAT_CHANCTRL_OFFSET);
> }
>
>

2014-02-20 10:18:07

by Stanislav Fomichev

[permalink] [raw]
Subject: Re: [PATCH] ioat: fix tasklet tear down

> Passes my testing, but would appreciate a tested-by.
Tested-by: Stanislav Fomichev <[email protected]>

2014-02-20 10:30:50

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] ioat: fix tasklet tear down

B1;3202;0cOn Wed, 19 Feb 2014, Dan Williams wrote:

> Since commit 77873803363c "net_dma: mark broken" we no longer pin dma
> engines active for the network-receive-offload use case. As a result
> the ->free_chan_resources() that occurs after the driver self-test no
> longer has a NET_DMA induced ->alloc_chan_resources() to back it up. A
> late firing irq can lead to ksoftirqd spinning indefinitely due to the
> tasklet_disable() performed by ->free_chan_resources(). Only
> ->alloc_chan_resources() can clear this condition in affected kernels.
>
> This problem has been present since commit 3e037454bcfa "I/OAT: Add
> support for MSI and MSI-X" in 2.6.24, but is now exposed. Given the
> NET_DMA use case is deprecated we can revisit moving the driver to use
> threaded irqs. For now, just tear down the irq and tasklet properly by:

Right, moving to threaded irqs would get rid of the whole tasklet
mess.

> 1/ Disable the irq from triggering the tasklet
>
> 2/ Disable the irq from re-arming
>
> 3/ Flush inflight interrupts
>
> 4/ Flush the timer
>
> 5/ Flush inflight tasklets
>
> References:
> https://lkml.org/lkml/2014/1/27/282
> https://lkml.org/lkml/2014/2/19/672
>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: <[email protected]>
> Reported-by: Mike Galbraith <[email protected]>
> Reported-by: Stanislav Fomichev <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>

Reviewed-by: Thomas Gleixner <[email protected]>

As Mike pointed out tsi721_free_chan_resources() has the same issue.

I did a quick scan of all tasklet_disable() sites. The teardown or
similar wreckage is available in:

drivers/atm/he.c
drivers/dma/at_hdmac.c
drivers/dma/pch_dma.c
drivers/input/keyboard/omap-keypad.c
drivers/isdn/gigaset/interface.c
drivers/media/pci/mantis/mantis_dvb.c
drivers/mmc/host/s3cmci.c
drivers/net/ethernet/jme.c
drivers/net/ethernet/silan/sc92031.c
drivers/net/usb/r8152.c
drivers/net/wireless/mwl8k.c
drivers/ntb/ntb_hw.c
drivers/rapidio/devices/tsi721_dma.c
drivers/s390/crypto/ap_bus.c
drivers/spi/spi-pl022.c
drivers/staging/cxt1e1/linux.c
drivers/staging/ozwpan/ozhcd.c
drivers/usb/gadget/fsl_qe_udc.c

That's 18 of 30 usage sites. Impressive....

We need to poke the relevant maintainers to get this solved.

Thanks,

tglx

2014-02-20 16:53:15

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] ioat: fix tasklet tear down

On Thu, Feb 20, 2014 at 2:30 AM, Thomas Gleixner <[email protected]> wrote:
> B1;3202;0cOn Wed, 19 Feb 2014, Dan Williams wrote:
>
>> Since commit 77873803363c "net_dma: mark broken" we no longer pin dma
>> engines active for the network-receive-offload use case. As a result
>> the ->free_chan_resources() that occurs after the driver self-test no
>> longer has a NET_DMA induced ->alloc_chan_resources() to back it up. A
>> late firing irq can lead to ksoftirqd spinning indefinitely due to the
>> tasklet_disable() performed by ->free_chan_resources(). Only
>> ->alloc_chan_resources() can clear this condition in affected kernels.
>>
>> This problem has been present since commit 3e037454bcfa "I/OAT: Add
>> support for MSI and MSI-X" in 2.6.24, but is now exposed. Given the
>> NET_DMA use case is deprecated we can revisit moving the driver to use
>> threaded irqs. For now, just tear down the irq and tasklet properly by:
>
> Right, moving to threaded irqs would get rid of the whole tasklet
> mess.
>
>> 1/ Disable the irq from triggering the tasklet
>>
>> 2/ Disable the irq from re-arming
>>
>> 3/ Flush inflight interrupts
>>
>> 4/ Flush the timer
>>
>> 5/ Flush inflight tasklets
>>
>> References:
>> https://lkml.org/lkml/2014/1/27/282
>> https://lkml.org/lkml/2014/2/19/672
>>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: Steven Rostedt <[email protected]>
>> Cc: <[email protected]>
>> Reported-by: Mike Galbraith <[email protected]>
>> Reported-by: Stanislav Fomichev <[email protected]>
>> Signed-off-by: Dan Williams <[email protected]>
>
> Reviewed-by: Thomas Gleixner <[email protected]>
>
> As Mike pointed out tsi721_free_chan_resources() has the same issue.
>
> I did a quick scan of all tasklet_disable() sites. The teardown or
> similar wreckage is available in:
>
> drivers/atm/he.c
> drivers/dma/at_hdmac.c
> drivers/dma/pch_dma.c
> drivers/input/keyboard/omap-keypad.c
> drivers/isdn/gigaset/interface.c
> drivers/media/pci/mantis/mantis_dvb.c
> drivers/mmc/host/s3cmci.c
> drivers/net/ethernet/jme.c
> drivers/net/ethernet/silan/sc92031.c
> drivers/net/usb/r8152.c
> drivers/net/wireless/mwl8k.c
> drivers/ntb/ntb_hw.c
> drivers/rapidio/devices/tsi721_dma.c
> drivers/s390/crypto/ap_bus.c
> drivers/spi/spi-pl022.c
> drivers/staging/cxt1e1/linux.c
> drivers/staging/ozwpan/ozhcd.c
> drivers/usb/gadget/fsl_qe_udc.c
>
> That's 18 of 30 usage sites. Impressive....
>
> We need to poke the relevant maintainers to get this solved.
>

Maybe also rename tasklet_disable() to tasklet_pause() to make it
clearer "this isn't the tasklet cleanup routine you're looking for"?

2014-02-21 23:31:20

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] ioat: fix tasklet tear down

On Thu, 20 Feb 2014, Dan Williams wrote:
> On Thu, Feb 20, 2014 at 2:30 AM, Thomas Gleixner <[email protected]> wrote:
> > That's 18 of 30 usage sites. Impressive....
> >
> > We need to poke the relevant maintainers to get this solved.
> >
>
> Maybe also rename tasklet_disable() to tasklet_pause() to make it
> clearer "this isn't the tasklet cleanup routine you're looking for"?

Indeed.