[PATCH v2 0/3] dmaengine: rcar-dmac: fix resource freeing synchronization

[PATCH v2 0/3] dmaengine: rcar-dmac: fix resource freeing synchronization

[Niklas Söderlund]

Hi,

This series fix resource freeing synchronization by:

1. Patch 1/3
   Store the IRQ number in the global struct so it can be used later
   together with synchronize_irq().

2. Patch 2/3
   Adding support for the device_synchronize() callback in patch 2/3.

3. Patch 3/3
   Waiting for any ISR that might still be running after the channel is
   halted prior to freeing its resources. This was patch previously part
   of a patch sent out by Yoshihiro Shimoda and authored by Hiroyuki
   Yokoyama, see [1].

   In that thread it was suggested by Lars-Peter Clausen to instead
   implement the device_synchronize() callback. Unfortunately this is not
   enough to solve the issue. In rcar_dmac_free_chan_resources() the
   channel is halted by a call to rcar_dmac_chan_halt() and then directly
   moves on to freeing resources, here it is still needed to add a wait
   for any ISR to finish before freeing the resources, despite that a
   device_synchronize() have been added.  This is because call chain:

   dma_release_channel()
     dma_chan_put()
       dmaengine_synchronize()
       rcar_dmac_free_chan_resources()
         rcar_dmac_chan_halt()

   Here dmaengine_synchronize() is called prior to rcar_dmac_chan_halt()
   so an extra synchronisation to wait for any running ISR is still
   needed.

By both adding a device_synchronize() which can be used in conjunction
with device_terminate_all() and fiends and by adding an explicit
synchronize_irq() when freeing channel resources I feel the
synchronisation for freeing channel resources are in a much better
shape. It also solves the issue in the original mail thread.

The series is based on v4.12-rc1 and is tested on r8a7795 Salvator-X.

1. https://patchwork.kernel.org/patch/9557691/

* Changes since v1
- Rebased to v4.12-rc1

Niklas Söderlund (3):
  dmaengine: rcar-dmac: store channel IRQ in struct rcar_dmac_chan
  dmaengine: rcar-dmac: implement device_synchronize()
  dmaengine: rcar-dmac: wait for ISR to finish before freeing resources

 drivers/dma/sh/rcar-dmac.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

-- 
2.13.0

[PATCH v2 1/3] dmaengine: rcar-dmac: store channel IRQ in struct rcar_dmac_chan

[Niklas Söderlund]

The IRQ number is needed after probe to be able to add synchronisation
points in other places in the driver when freeing resources and to
implement a device_synchronize() callback. Store the IRQ number in the
struct rcar_dmac_chan so that it can be used later.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/dma/sh/rcar-dmac.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index db41795fe42ae6ed..c68c3336bdad44df 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -144,6 +144,7 @@ struct rcar_dmac_chan_map {
  * @chan: base DMA channel object
  * @iomem: channel I/O memory base
  * @index: index of this channel in the controller
+ * @irq: channel IRQ
  * @src: slave memory address and size on the source side
  * @dst: slave memory address and size on the destination side
  * @mid_rid: hardware MID/RID for the DMA client using this channel
@@ -161,6 +162,7 @@ struct rcar_dmac_chan {
 	struct dma_chan chan;
 	void __iomem *iomem;
 	unsigned int index;
+	int irq;
 
 	struct rcar_dmac_chan_slave src;
 	struct rcar_dmac_chan_slave dst;
@@ -1647,7 +1649,6 @@ static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
 	struct dma_chan *chan = &rchan->chan;
 	char pdev_irqname[5];
 	char *irqname;
-	int irq;
 	int ret;
 
 	rchan->index = index;
@@ -1664,8 +1665,8 @@ static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
 
 	/* Request the channel interrupt. */
 	sprintf(pdev_irqname, "ch%u", index);
-	irq = platform_get_irq_byname(pdev, pdev_irqname);
-	if (irq < 0) {
+	rchan->irq = platform_get_irq_byname(pdev, pdev_irqname);
+	if (rchan->irq < 0) {
 		dev_err(dmac->dev, "no IRQ specified for channel %u\n", index);
 		return -ENODEV;
 	}
@@ -1675,11 +1676,13 @@ static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
 	if (!irqname)
 		return -ENOMEM;
 
-	ret = devm_request_threaded_irq(dmac->dev, irq, rcar_dmac_isr_channel,
+	ret = devm_request_threaded_irq(dmac->dev, rchan->irq,
+					rcar_dmac_isr_channel,
 					rcar_dmac_isr_channel_thread, 0,
 					irqname, rchan);
 	if (ret) {
-		dev_err(dmac->dev, "failed to request IRQ %u (%d)\n", irq, ret);
+		dev_err(dmac->dev, "failed to request IRQ %u (%d)\n",
+			rchan->irq, ret);
 		return ret;
 	}
 
-- 
2.13.0

Re: [PATCH v2 1/3] dmaengine: rcar-dmac: store channel IRQ in struct rcar_dmac_chan

[Vinod Koul]

On Tue, May 16, 2017 at 01:09:15AM +0200, Niklas S�derlund wrote:
> The IRQ number is needed after probe to be able to add synchronisation
> points in other places in the driver when freeing resources and to
> implement a device_synchronize() callback. Store the IRQ number in the
> struct rcar_dmac_chan so that it can be used later.
> 
> Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/dma/sh/rcar-dmac.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index db41795fe42ae6ed..c68c3336bdad44df 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -144,6 +144,7 @@ struct rcar_dmac_chan_map {
>   * @chan: base DMA channel object
>   * @iomem: channel I/O memory base
>   * @index: index of this channel in the controller
> + * @irq: channel IRQ
>   * @src: slave memory address and size on the source side
>   * @dst: slave memory address and size on the destination side
>   * @mid_rid: hardware MID/RID for the DMA client using this channel
> @@ -161,6 +162,7 @@ struct rcar_dmac_chan {
>  	struct dma_chan chan;
>  	void __iomem *iomem;
>  	unsigned int index;
> +	int irq;

Thats a bit odd choice to store per ch, I would have stored in rcar_dmac.
Any specific reason, do we have per ch irq here?

>  
>  	struct rcar_dmac_chan_slave src;
>  	struct rcar_dmac_chan_slave dst;
> @@ -1647,7 +1649,6 @@ static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
>  	struct dma_chan *chan = &rchan->chan;
>  	char pdev_irqname[5];
>  	char *irqname;
> -	int irq;
>  	int ret;
>  
>  	rchan->index = index;
> @@ -1664,8 +1665,8 @@ static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
>  
>  	/* Request the channel interrupt. */
>  	sprintf(pdev_irqname, "ch%u", index);
> -	irq = platform_get_irq_byname(pdev, pdev_irqname);
> -	if (irq < 0) {
> +	rchan->irq = platform_get_irq_byname(pdev, pdev_irqname);
> +	if (rchan->irq < 0) {
>  		dev_err(dmac->dev, "no IRQ specified for channel %u\n", index);
>  		return -ENODEV;
>  	}
> @@ -1675,11 +1676,13 @@ static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
>  	if (!irqname)
>  		return -ENOMEM;
>  
> -	ret = devm_request_threaded_irq(dmac->dev, irq, rcar_dmac_isr_channel,
> +	ret = devm_request_threaded_irq(dmac->dev, rchan->irq,
> +					rcar_dmac_isr_channel,
>  					rcar_dmac_isr_channel_thread, 0,
>  					irqname, rchan);
>  	if (ret) {
> -		dev_err(dmac->dev, "failed to request IRQ %u (%d)\n", irq, ret);
> +		dev_err(dmac->dev, "failed to request IRQ %u (%d)\n",
> +			rchan->irq, ret);

orthogonal, while at it care to fix explicit free of irq in remove, it seems
this driver doesn't do so..

-- 
~Vinod

Re: [PATCH v2 1/3] dmaengine: rcar-dmac: store channel IRQ in struct rcar_dmac_chan

[Geert Uytterhoeven]

Hi Vinod,

On Fri, May 19, 2017 at 5:51 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, May 16, 2017 at 01:09:15AM +0200, Niklas Söderlund wrote:
>> The IRQ number is needed after probe to be able to add synchronisation
>> points in other places in the driver when freeing resources and to
>> implement a device_synchronize() callback. Store the IRQ number in the
>> struct rcar_dmac_chan so that it can be used later.
>>
>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> ---
>>  drivers/dma/sh/rcar-dmac.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
>> index db41795fe42ae6ed..c68c3336bdad44df 100644
>> --- a/drivers/dma/sh/rcar-dmac.c
>> +++ b/drivers/dma/sh/rcar-dmac.c
>> @@ -144,6 +144,7 @@ struct rcar_dmac_chan_map {
>>   * @chan: base DMA channel object
>>   * @iomem: channel I/O memory base
>>   * @index: index of this channel in the controller
>> + * @irq: channel IRQ
>>   * @src: slave memory address and size on the source side
>>   * @dst: slave memory address and size on the destination side
>>   * @mid_rid: hardware MID/RID for the DMA client using this channel
>> @@ -161,6 +162,7 @@ struct rcar_dmac_chan {
>>       struct dma_chan chan;
>>       void __iomem *iomem;
>>       unsigned int index;
>> +     int irq;
>
> Thats a bit odd choice to store per ch, I would have stored in rcar_dmac.
> Any specific reason, do we have per ch irq here?

Yes, each channel has its own interrupt.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 1/3] dmaengine: rcar-dmac: store channel IRQ in struct rcar_dmac_chan

[Vinod Koul]

On Fri, May 19, 2017 at 08:59:03AM +0200, Geert Uytterhoeven wrote:
> Hi Vinod,
> 
> On Fri, May 19, 2017 at 5:51 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> > On Tue, May 16, 2017 at 01:09:15AM +0200, Niklas S�derlund wrote:
> >> The IRQ number is needed after probe to be able to add synchronisation
> >> points in other places in the driver when freeing resources and to
> >> implement a device_synchronize() callback. Store the IRQ number in the
> >> struct rcar_dmac_chan so that it can be used later.
> >>
> >> Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> >> ---
> >>  drivers/dma/sh/rcar-dmac.c | 13 ++++++++-----
> >>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> >> index db41795fe42ae6ed..c68c3336bdad44df 100644
> >> --- a/drivers/dma/sh/rcar-dmac.c
> >> +++ b/drivers/dma/sh/rcar-dmac.c
> >> @@ -144,6 +144,7 @@ struct rcar_dmac_chan_map {
> >>   * @chan: base DMA channel object
> >>   * @iomem: channel I/O memory base
> >>   * @index: index of this channel in the controller
> >> + * @irq: channel IRQ
> >>   * @src: slave memory address and size on the source side
> >>   * @dst: slave memory address and size on the destination side
> >>   * @mid_rid: hardware MID/RID for the DMA client using this channel
> >> @@ -161,6 +162,7 @@ struct rcar_dmac_chan {
> >>       struct dma_chan chan;
> >>       void __iomem *iomem;
> >>       unsigned int index;
> >> +     int irq;
> >
> > Thats a bit odd choice to store per ch, I would have stored in rcar_dmac.
> > Any specific reason, do we have per ch irq here?
> 
> Yes, each channel has its own interrupt.

Then this one is just fine :)

-- 
~Vinod

[PATCH v2 2/3] dmaengine: rcar-dmac: implement device_synchronize()

[Niklas Söderlund]

Implement the device_synchronize() callback which wait until a dma
channel is stopped to provide a synchronization point.

This protects the driver from multiple race conditions when terminating
and freeing resources. E.g. the completion callback still running after
device_terminate_all() has completed.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/dma/sh/rcar-dmac.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index c68c3336bdad44df..fb07cd5fe77b3c43 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1365,6 +1365,13 @@ static void rcar_dmac_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&rchan->lock, flags);
 }
 
+static void rcar_dmac_device_synchronize(struct dma_chan *chan)
+{
+	struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan);
+
+	synchronize_irq(rchan->irq);
+}
+
 /* -----------------------------------------------------------------------------
  * IRQ handling
  */
@@ -1846,6 +1853,7 @@ static int rcar_dmac_probe(struct platform_device *pdev)
 	engine->device_terminate_all = rcar_dmac_chan_terminate_all;
 	engine->device_tx_status = rcar_dmac_tx_status;
 	engine->device_issue_pending = rcar_dmac_issue_pending;
+	engine->device_synchronize = rcar_dmac_device_synchronize;
 
 	ret = dma_async_device_register(engine);
 	if (ret < 0)
-- 
2.13.0

[PATCH v2 3/3] dmaengine: rcar-dmac: wait for ISR to finish before freeing resources

[Niklas Söderlund]

This fixes a race condition where the channel resources could be freed
before the ISR had finished running resulting in a NULL pointer
reference from the ISR.

[  167.148934] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[  167.157051] pgd = ffff80003c641000
[  167.160449] [00000000] *pgd=000000007c507003, *pud=000000007c4ff003, *pmd=0000000000000000
[  167.168719] Internal error: Oops: 96000046 [#1] PREEMPT SMP
[  167.174289] Modules linked in:
[  167.177348] CPU: 3 PID: 10547 Comm: dma_ioctl Not tainted 4.11.0-rc1-00001-g8d92afddc2f6633a #73
[  167.186131] Hardware name: Renesas Salvator-X board based on r8a7795 (DT)
[  167.192917] task: ffff80003a411a00 task.stack: ffff80003bcd4000
[  167.198850] PC is at rcar_dmac_chan_prep_sg+0xe0/0x400
[  167.203985] LR is at rcar_dmac_chan_prep_sg+0x48/0x400

Based of previous work by:
    Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/dma/sh/rcar-dmac.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index fb07cd5fe77b3c43..d2cb4a0916e62e62 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1010,7 +1010,11 @@ static void rcar_dmac_free_chan_resources(struct dma_chan *chan)
 	rcar_dmac_chan_halt(rchan);
 	spin_unlock_irq(&rchan->lock);
 
-	/* Now no new interrupts will occur */
+	/*
+	 * Now no new interrupts will occur, but one might already be
+	 * running. Wait for it to finish before freeing resources.
+	 */
+	synchronize_irq(rchan->irq);
 
 	if (rchan->mid_rid >= 0) {
 		/* The caller is holding dma_list_mutex */
-- 
2.13.0

Re: [PATCH v2 0/3] dmaengine: rcar-dmac: fix resource freeing synchronization

[Vinod Koul]

On Tue, May 16, 2017 at 01:09:14AM +0200, Niklas S�derlund wrote:
> Hi,
> 
> This series fix resource freeing synchronization by:
> 
> 1. Patch 1/3
>    Store the IRQ number in the global struct so it can be used later
>    together with synchronize_irq().
> 
> 2. Patch 2/3
>    Adding support for the device_synchronize() callback in patch 2/3.
> 
> 3. Patch 3/3
>    Waiting for any ISR that might still be running after the channel is
>    halted prior to freeing its resources. This was patch previously part
>    of a patch sent out by Yoshihiro Shimoda and authored by Hiroyuki
>    Yokoyama, see [1].
> 
>    In that thread it was suggested by Lars-Peter Clausen to instead
>    implement the device_synchronize() callback. Unfortunately this is not
>    enough to solve the issue. In rcar_dmac_free_chan_resources() the
>    channel is halted by a call to rcar_dmac_chan_halt() and then directly
>    moves on to freeing resources, here it is still needed to add a wait
>    for any ISR to finish before freeing the resources, despite that a
>    device_synchronize() have been added.  This is because call chain:
> 
>    dma_release_channel()
>      dma_chan_put()
>        dmaengine_synchronize()
>        rcar_dmac_free_chan_resources()
>          rcar_dmac_chan_halt()
> 
>    Here dmaengine_synchronize() is called prior to rcar_dmac_chan_halt()
>    so an extra synchronisation to wait for any running ISR is still
>    needed.
> 
> By both adding a device_synchronize() which can be used in conjunction
> with device_terminate_all() and fiends and by adding an explicit
> synchronize_irq() when freeing channel resources I feel the
> synchronisation for freeing channel resources are in a much better
> shape. It also solves the issue in the original mail thread.

Applied now, thanks

-- 
~Vinod