| Message ID | 20200217144050.3i4ymbytogod4ijn@kili.mountain (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | dmaengine: coh901318: Fix a double lock bug in dma_tc_handle() | expand |
Hi Dan, On Mon, Feb 17, 2020 at 3:41 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > The caller is already holding the lock so this will deadlock. > > Fixes: 0b58828c923e ("DMAENGINE: COH 901 318 remove irq counting") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > This is the second double lock bug found using static analysis. The > previous one was commit 627469e4445b ("dmaengine: coh901318: Fix a > double-lock bug"). > > The fact that this has been broken for ten years suggests that no one > has the hardware. Or this only runs CONFIG_SMP=n kernels? This seems to be used in arch/arm/boot/dts/ste-u300.dts only, and CONFIG_ARCH_U300 is a ARCH_MULTI_V5 platform, which looks like it doesn't support SMP? Gr{oetje,eeting}s, Geert
On 17-02-20, 23:24, Geert Uytterhoeven wrote: > Hi Dan, > > On Mon, Feb 17, 2020 at 3:41 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > The caller is already holding the lock so this will deadlock. > > > > Fixes: 0b58828c923e ("DMAENGINE: COH 901 318 remove irq counting") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > This is the second double lock bug found using static analysis. The > > previous one was commit 627469e4445b ("dmaengine: coh901318: Fix a > > double-lock bug"). > > > > The fact that this has been broken for ten years suggests that no one > > has the hardware. > > Or this only runs CONFIG_SMP=n kernels? > This seems to be used in arch/arm/boot/dts/ste-u300.dts only, and > CONFIG_ARCH_U300 is a ARCH_MULTI_V5 platform, which looks like > it doesn't support SMP? Should we drop the driver then..?
Hi Vinod, On Wed, Feb 19, 2020 at 10:18 AM Vinod Koul <vkoul@kernel.org> wrote: > On 17-02-20, 23:24, Geert Uytterhoeven wrote: > > On Mon, Feb 17, 2020 at 3:41 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > The caller is already holding the lock so this will deadlock. > > > > > > Fixes: 0b58828c923e ("DMAENGINE: COH 901 318 remove irq counting") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > This is the second double lock bug found using static analysis. The > > > previous one was commit 627469e4445b ("dmaengine: coh901318: Fix a > > > double-lock bug"). > > > > > > The fact that this has been broken for ten years suggests that no one > > > has the hardware. > > > > Or this only runs CONFIG_SMP=n kernels? > > This seems to be used in arch/arm/boot/dts/ste-u300.dts only, and > > CONFIG_ARCH_U300 is a ARCH_MULTI_V5 platform, which looks like > > it doesn't support SMP? > > Should we drop the driver then..? Why? Because spinlocks are no-ops on SMP=n, and spinlock bugs thus don't affect the single platform using the driver? Gr{oetje,eeting}s, Geert
On 19-02-20, 10:20, Geert Uytterhoeven wrote: > Hi Vinod, > > On Wed, Feb 19, 2020 at 10:18 AM Vinod Koul <vkoul@kernel.org> wrote: > > On 17-02-20, 23:24, Geert Uytterhoeven wrote: > > > On Mon, Feb 17, 2020 at 3:41 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > The caller is already holding the lock so this will deadlock. > > > > > > > > Fixes: 0b58828c923e ("DMAENGINE: COH 901 318 remove irq counting") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > --- > > > > This is the second double lock bug found using static analysis. The > > > > previous one was commit 627469e4445b ("dmaengine: coh901318: Fix a > > > > double-lock bug"). > > > > > > > > The fact that this has been broken for ten years suggests that no one > > > > has the hardware. > > > > > > Or this only runs CONFIG_SMP=n kernels? > > > This seems to be used in arch/arm/boot/dts/ste-u300.dts only, and > > > CONFIG_ARCH_U300 is a ARCH_MULTI_V5 platform, which looks like > > > it doesn't support SMP? > > > > Should we drop the driver then..? > > Why? Because spinlocks are no-ops on SMP=n, and spinlock bugs thus don't > affect the single platform using the driver? That doesn't answer the question if anyone has a hardware and we have users :) Sorry I should have written better about hardware and testing rather than cryptic reply which may have suggested about SMP :)
On Wed, Feb 19, 2020 at 10:17 AM Vinod Koul <vkoul@kernel.org> wrote: > On 17-02-20, 23:24, Geert Uytterhoeven wrote: > > On Mon, Feb 17, 2020 at 3:41 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > The caller is already holding the lock so this will deadlock. > > > > > > Fixes: 0b58828c923e ("DMAENGINE: COH 901 318 remove irq counting") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > This is the second double lock bug found using static analysis. The > > > previous one was commit 627469e4445b ("dmaengine: coh901318: Fix a > > > double-lock bug"). > > > > > > The fact that this has been broken for ten years suggests that no one > > > has the hardware. > > > > Or this only runs CONFIG_SMP=n kernels? > > This seems to be used in arch/arm/boot/dts/ste-u300.dts only, and > > CONFIG_ARCH_U300 is a ARCH_MULTI_V5 platform, which looks like > > it doesn't support SMP? > > Should we drop the driver then..? I still have the hardware and it still works if that is the question :D And yeah it only has one CPU, but still has a DMA engine. The patch is fine to apply because it fixes a bug, should the same hardware block be used on SMP. Yours, Linus Walleij
On 21-02-20, 15:50, Linus Walleij wrote: > On Wed, Feb 19, 2020 at 10:17 AM Vinod Koul <vkoul@kernel.org> wrote: > > On 17-02-20, 23:24, Geert Uytterhoeven wrote: > > > On Mon, Feb 17, 2020 at 3:41 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > The caller is already holding the lock so this will deadlock. > > > > > > > > Fixes: 0b58828c923e ("DMAENGINE: COH 901 318 remove irq counting") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > --- > > > > This is the second double lock bug found using static analysis. The > > > > previous one was commit 627469e4445b ("dmaengine: coh901318: Fix a > > > > double-lock bug"). > > > > > > > > The fact that this has been broken for ten years suggests that no one > > > > has the hardware. > > > > > > Or this only runs CONFIG_SMP=n kernels? > > > This seems to be used in arch/arm/boot/dts/ste-u300.dts only, and > > > CONFIG_ARCH_U300 is a ARCH_MULTI_V5 platform, which looks like > > > it doesn't support SMP? > > > > Should we drop the driver then..? > > I still have the hardware and it still works if that is the question :D Thanks for confirming :) > > And yeah it only has one CPU, but still has a DMA engine. > > The patch is fine to apply because it fixes a bug, should the same > hardware block be used on SMP. Applied now.
diff --git a/drivers/dma/coh901318.c b/drivers/dma/coh901318.c index e51d836afcc7..1092d4ce723e 100644 --- a/drivers/dma/coh901318.c +++ b/drivers/dma/coh901318.c @@ -1947,8 +1947,6 @@ static void dma_tc_handle(struct coh901318_chan *cohc) return; } - spin_lock(&cohc->lock); - /* * When we reach this point, at least one queue item * should have been moved over from cohc->queue to @@ -1969,8 +1967,6 @@ static void dma_tc_handle(struct coh901318_chan *cohc) if (coh901318_queue_start(cohc) == NULL) cohc->busy = 0; - spin_unlock(&cohc->lock); - /* * This tasklet will remove items from cohc->active * and thus terminates them.
The caller is already holding the lock so this will deadlock. Fixes: 0b58828c923e ("DMAENGINE: COH 901 318 remove irq counting") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- This is the second double lock bug found using static analysis. The previous one was commit 627469e4445b ("dmaengine: coh901318: Fix a double-lock bug"). The fact that this has been broken for ten years suggests that no one has the hardware. drivers/dma/coh901318.c | 4 ---- 1 file changed, 4 deletions(-)