From: maxime.ripard@free-electrons.com (Maxime Ripard)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v11 0/2] Add support for the Allwinner A31 DMA Controller
Date: Mon, 28 Jul 2014 11:41:48 +0200 [thread overview]
Message-ID: <20140728094148.GC3952@lukather> (raw)
In-Reply-To: <20140725164518.GQ21766@n2100.arm.linux.org.uk>
Hi Russell,
On Fri, Jul 25, 2014 at 05:45:19PM +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 25, 2014 at 06:37:46PM +0200, Maxime Ripard wrote:
> > Hi Vinod,
> >
> > On Fri, Jul 25, 2014 at 06:42:17PM +0530, Vinod Koul wrote:
> > > - don't use devm_request_irq(). You have irq enabled and you have killed
> > > tasklet. This is too racy. You need to ensure no irqs can be generated before killing
> > > tasklets.
> >
> > Ok, would calling disable_irq before killing the tasklet an option for
> > you ? that would allow to keep the devm_request_irq.
>
> That's not really an acceptable approach if you can use shared
> interrupts.
We don't, but yes, I see your point.
> A better alternative would be devm_free_irq() to give a definite point
> that the interrupt is unregistered in the driver remove sequence. That
> allows you to keep the advantage of devm_request_irq() to clean up during
> the initialisation side.
Ah, right, thanks.
> An alternative approach would be to ensure that the hardware is quiesced,
> and interrupts are disabled. Then call synchronize_irq() on it, and at
> that point, you should be certain that your interrupt handler should not
> process any further interrupts for your device (though, in a shared
> interrupt environment, it would still be called should a different device
> on the shared line raise its interrupt.)
Actually, unless I'm missing something, that's pretty much what we're
doing here.
I disable all interrupts in the DMA controller, I call
synchronize_irq, and then kill the tasklet. The only interrupts I
could get are spurious, and we made sure such kind of interrupts
couldn't schedule the tasklet either.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140728/5d00e9e9/attachment.sig>
WARNING: multiple messages have this Message-ID (diff)
From: Maxime Ripard <maxime.ripard@free-electrons.com>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Vinod Koul <vinod.koul@intel.com>,
andriy.shevchenko@intel.com, Arnd Bergmann <arnd@arndb.de>,
linux-kernel@vger.kernel.org, zhuzhenhua@allwinnertech.com,
dmaengine@vger.kernel.org, linux-sunxi@googlegroups.com,
kevin.z.m.zh@gmail.com, sunny@allwinnertech.com,
shuge@allwinnertech.com, Dan Williams <dan.j.williams@intel.com>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v11 0/2] Add support for the Allwinner A31 DMA Controller
Date: Mon, 28 Jul 2014 11:41:48 +0200 [thread overview]
Message-ID: <20140728094148.GC3952@lukather> (raw)
In-Reply-To: <20140725164518.GQ21766@n2100.arm.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1840 bytes --]
Hi Russell,
On Fri, Jul 25, 2014 at 05:45:19PM +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 25, 2014 at 06:37:46PM +0200, Maxime Ripard wrote:
> > Hi Vinod,
> >
> > On Fri, Jul 25, 2014 at 06:42:17PM +0530, Vinod Koul wrote:
> > > - don't use devm_request_irq(). You have irq enabled and you have killed
> > > tasklet. This is too racy. You need to ensure no irqs can be generated before killing
> > > tasklets.
> >
> > Ok, would calling disable_irq before killing the tasklet an option for
> > you ? that would allow to keep the devm_request_irq.
>
> That's not really an acceptable approach if you can use shared
> interrupts.
We don't, but yes, I see your point.
> A better alternative would be devm_free_irq() to give a definite point
> that the interrupt is unregistered in the driver remove sequence. That
> allows you to keep the advantage of devm_request_irq() to clean up during
> the initialisation side.
Ah, right, thanks.
> An alternative approach would be to ensure that the hardware is quiesced,
> and interrupts are disabled. Then call synchronize_irq() on it, and at
> that point, you should be certain that your interrupt handler should not
> process any further interrupts for your device (though, in a shared
> interrupt environment, it would still be called should a different device
> on the shared line raise its interrupt.)
Actually, unless I'm missing something, that's pretty much what we're
doing here.
I disable all interrupts in the DMA controller, I call
synchronize_irq, and then kill the tasklet. The only interrupts I
could get are spurious, and we made sure such kind of interrupts
couldn't schedule the tasklet either.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
next prev parent reply other threads:[~2014-07-28 9:41 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-17 19:46 [PATCH v11 0/2] Add support for the Allwinner A31 DMA Controller Maxime Ripard
2014-07-17 19:46 ` Maxime Ripard
2014-07-17 19:46 ` [PATCH v11 1/2] Documentation: dt: Add Allwinner A31 DMA controller bindings Maxime Ripard
2014-07-17 19:46 ` Maxime Ripard
2014-07-17 19:46 ` [PATCH v11 2/2] dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller Maxime Ripard
2014-07-17 19:46 ` Maxime Ripard
2014-07-24 12:13 ` [PATCH v11 0/2] Add support for the Allwinner A31 DMA Controller Maxime Ripard
2014-07-24 12:13 ` Maxime Ripard
2014-07-24 23:44 ` Andrew Morton
2014-07-24 23:44 ` Andrew Morton
2014-07-25 6:11 ` Vinod Koul
2014-07-25 6:11 ` Vinod Koul
2014-07-25 6:13 ` Vinod Koul
2014-07-25 6:13 ` Vinod Koul
2014-07-25 7:14 ` Andrew Morton
2014-07-25 7:14 ` Andrew Morton
2014-07-25 13:12 ` Vinod Koul
2014-07-25 13:12 ` Vinod Koul
2014-07-25 16:37 ` Maxime Ripard
2014-07-25 16:37 ` Maxime Ripard
2014-07-25 16:42 ` Vinod Koul
2014-07-25 16:42 ` Vinod Koul
2014-07-28 10:14 ` Maxime Ripard
2014-07-28 10:14 ` Maxime Ripard
2014-07-28 14:18 ` Vinod Koul
2014-07-28 14:18 ` Vinod Koul
2014-07-29 16:03 ` Maxime Ripard
2014-07-29 16:03 ` Maxime Ripard
2014-07-25 16:45 ` Russell King - ARM Linux
2014-07-25 16:45 ` Russell King - ARM Linux
2014-07-28 9:41 ` Maxime Ripard [this message]
2014-07-28 9:41 ` Maxime Ripard
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=20140728094148.GC3952@lukather \
--to=maxime.ripard@free-electrons.com \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.