[PATCH 0/3] dma: mv_xor: spinlock related improvements

[PATCH 0/3] dma: mv_xor: spinlock related improvements

[Thomas Petazzoni]

Vinod, Dan,

Here are three patches that make improvements to the spinlock handling
of the mv_xor driver. The first patch fixes a lock/release of the
channel spinlock in mv_xor_status(), and the next two patches make
followup code improvements/refactoring.

Ezequiel, it would be good to have your review on these patches, since
I know you've spent some time lately doing mv_xor stuff.

Thanks!

Thomas

Thomas Petazzoni (3):
  dma: mv_xor: take channel spinlock in mv_xor_status()
  dma: mv_xor: use __mv_xor_slot_cleanup() in
    mv_xor_free_chan_resources()
  dma: mv_xor: rename __mv_xor_slot_cleanup() to mv_xor_slot_cleanup()

 drivers/dma/mv_xor.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

-- 
1.8.3.2

[PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status()

[Thomas Petazzoni]

The mv_xor_status() function accesses the mv_xor_chan structure, but
was not taking the corresponding spinlock. This patch fixes this
problem.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/dma/mv_xor.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 53fb0c8..526ab27 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -701,14 +701,20 @@ static enum dma_status mv_xor_status(struct dma_chan *chan,
 	struct mv_xor_chan *mv_chan = to_mv_xor_chan(chan);
 	enum dma_status ret;
 
+	spin_lock_bh(&mv_chan->lock);
+
 	ret = dma_cookie_status(chan, cookie, txstate);
 	if (ret == DMA_COMPLETE) {
 		mv_xor_clean_completed_slots(mv_chan);
+		spin_unlock_bh(&mv_chan->lock);
 		return ret;
 	}
-	mv_xor_slot_cleanup(mv_chan);
+	__mv_xor_slot_cleanup(mv_chan);
 
-	return dma_cookie_status(chan, cookie, txstate);
+	ret = dma_cookie_status(chan, cookie, txstate);
+	spin_unlock_bh(&mv_chan->lock);
+
+	return ret;
 }
 
 static void mv_dump_xor_regs(struct mv_xor_chan *chan)
-- 
1.8.3.2

[PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status()

[Shevchenko, Andriy]

On Fri, 2013-12-27 at 12:38 +0100, Thomas Petazzoni wrote:
> The mv_xor_status() function accesses the mv_xor_chan structure, but
> was not taking the corresponding spinlock. This patch fixes this
> problem.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/dma/mv_xor.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 53fb0c8..526ab27 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -701,14 +701,20 @@ static enum dma_status mv_xor_status(struct dma_chan *chan,
>  	struct mv_xor_chan *mv_chan = to_mv_xor_chan(chan);
>  	enum dma_status ret;
>  
> +	spin_lock_bh(&mv_chan->lock);
> +

I don't think it's a proper place for this spinlock since
dma_cookie_status is independent to that.

>  	ret = dma_cookie_status(chan, cookie, txstate);
>  	if (ret == DMA_COMPLETE) {

If you are caring about mv_chan access, better to take it here...

>  		mv_xor_clean_completed_slots(mv_chan);
> +		spin_unlock_bh(&mv_chan->lock);
>  		return ret;
>  	}

...and here.

> -	mv_xor_slot_cleanup(mv_chan);
> +	__mv_xor_slot_cleanup(mv_chan);
>  
> -	return dma_cookie_status(chan, cookie, txstate);
> +	ret = dma_cookie_status(chan, cookie, txstate);
> +	spin_unlock_bh(&mv_chan->lock);
> +
> +	return ret;
>  }
>  
>  static void mv_dump_xor_regs(struct mv_xor_chan *chan)

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status()

[Dan Williams]

On Fri, Dec 27, 2013 at 3:38 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> The mv_xor_status() function accesses the mv_xor_chan structure, but
> was not taking the corresponding spinlock. This patch fixes this
> problem.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/dma/mv_xor.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 53fb0c8..526ab27 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -701,14 +701,20 @@ static enum dma_status mv_xor_status(struct dma_chan *chan,
>         struct mv_xor_chan *mv_chan = to_mv_xor_chan(chan);
>         enum dma_status ret;
>
> +       spin_lock_bh(&mv_chan->lock);
> +
>         ret = dma_cookie_status(chan, cookie, txstate);
>         if (ret == DMA_COMPLETE) {
>                 mv_xor_clean_completed_slots(mv_chan);

I think you can just delete this call to
mv_xor_clean_completed_slots().  The fact that the descriptors are
complete means that __mv_xor_slot_cleanup ran, and if that is the case
there should be nothing to cleanup.

--
Dan

[PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status()

[Ezequiel Garcia]

Hello Dan,

Thomas is a bit busy right now, so I'm picking this where he left it.

On Dec 31, Dan Williams wrote:
> On Fri, Dec 27, 2013 at 3:38 AM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
[..]
> > @@ -701,14 +701,20 @@ static enum dma_status mv_xor_status(struct dma_chan *chan,
> >         struct mv_xor_chan *mv_chan = to_mv_xor_chan(chan);
> >         enum dma_status ret;
> >
> > +       spin_lock_bh(&mv_chan->lock);
> > +
> >         ret = dma_cookie_status(chan, cookie, txstate);
> >         if (ret == DMA_COMPLETE) {
> >                 mv_xor_clean_completed_slots(mv_chan);
> 
> I think you can just delete this call to
> mv_xor_clean_completed_slots().  The fact that the descriptors are
> complete means that __mv_xor_slot_cleanup ran, and if that is the case
> there should be nothing to cleanup.
> 

In that case, we don't need to take the lock anywhere in this function:

1. It's not needed for dma_cookie_status() (as per Andy's comment).
2. It's not needed for mv_xor_slot_cleanup(), which takes the lock.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 2/3] dma: mv_xor: use __mv_xor_slot_cleanup() in mv_xor_free_chan_resources()

[Thomas Petazzoni]

The mv_xor_free_chan_resources() was doing:

 1/ Call mv_xor_slot_cleanup, which take the channel spinlock, does
    its work, and releases the channel spinlock
 2/ Take the channel spinlock
 3/ Do its work
 4/ Release the channel spinlock

This commit changes the function to use the __mv_xor_slot_cleanup()
variant of mv_xor_slot_cleanup(), which assumes the caller already
holds the channel spinlock. The sequence becomes:

 1/ Take the channel spinlock
 2/ Call __mv_xor_slot_cleanup
 3/ Do the rest of the work
 4/ Release the channel spinlock

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/dma/mv_xor.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 526ab27..3d7d36b 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -658,9 +658,10 @@ static void mv_xor_free_chan_resources(struct dma_chan *chan)
 	struct mv_xor_desc_slot *iter, *_iter;
 	int in_use_descs = 0;
 
-	mv_xor_slot_cleanup(mv_chan);
-
 	spin_lock_bh(&mv_chan->lock);
+
+	__mv_xor_slot_cleanup(mv_chan);
+
 	list_for_each_entry_safe(iter, _iter, &mv_chan->chain,
 					chain_node) {
 		in_use_descs++;
-- 
1.8.3.2

[PATCH 3/3] dma: mv_xor: rename __mv_xor_slot_cleanup() to mv_xor_slot_cleanup()

[Thomas Petazzoni]

Now that mv_xor_slot_cleanup() has only one remaining caller: the mv_xor_tasklet(), we:

 1/ get rid of mv_xor_slot_cleanup(), and directly take/release the
    spinlock in mv_xor_tasklet()

 2/ rename __mv_xor_slot_cleanup() to mv_xor_slot_cleanup()

We take this opportunity to add a comment that makes it clear that the
channel spinlock should be held before calling mv_xor_slot_cleanup().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/dma/mv_xor.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 3d7d36b..611795e 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -312,7 +312,8 @@ mv_xor_clean_slot(struct mv_xor_desc_slot *desc,
 	return 0;
 }
 
-static void __mv_xor_slot_cleanup(struct mv_xor_chan *mv_chan)
+/* This function must be called with the mv_xor_chan spinlock held */
+static void mv_xor_slot_cleanup(struct mv_xor_chan *mv_chan)
 {
 	struct mv_xor_desc_slot *iter, *_iter;
 	dma_cookie_t cookie = 0;
@@ -368,18 +369,12 @@ static void __mv_xor_slot_cleanup(struct mv_xor_chan *mv_chan)
 		mv_chan->dmachan.completed_cookie = cookie;
 }
 
-static void
-mv_xor_slot_cleanup(struct mv_xor_chan *mv_chan)
-{
-	spin_lock_bh(&mv_chan->lock);
-	__mv_xor_slot_cleanup(mv_chan);
-	spin_unlock_bh(&mv_chan->lock);
-}
-
 static void mv_xor_tasklet(unsigned long data)
 {
 	struct mv_xor_chan *chan = (struct mv_xor_chan *) data;
+	spin_lock_bh(&chan->lock);
 	mv_xor_slot_cleanup(chan);
+	spin_unlock_bh(&chan->lock);
 }
 
 static struct mv_xor_desc_slot *
@@ -660,7 +655,7 @@ static void mv_xor_free_chan_resources(struct dma_chan *chan)
 
 	spin_lock_bh(&mv_chan->lock);
 
-	__mv_xor_slot_cleanup(mv_chan);
+	mv_xor_slot_cleanup(mv_chan);
 
 	list_for_each_entry_safe(iter, _iter, &mv_chan->chain,
 					chain_node) {
@@ -710,7 +705,7 @@ static enum dma_status mv_xor_status(struct dma_chan *chan,
 		spin_unlock_bh(&mv_chan->lock);
 		return ret;
 	}
-	__mv_xor_slot_cleanup(mv_chan);
+	mv_xor_slot_cleanup(mv_chan);
 
 	ret = dma_cookie_status(chan, cookie, txstate);
 	spin_unlock_bh(&mv_chan->lock);
-- 
1.8.3.2

[PATCH 0/3] dma: mv_xor: spinlock related improvements

[Ezequiel Garcia]

On Fri, Dec 27, 2013 at 12:38:31PM +0100, Thomas Petazzoni wrote:
> Vinod, Dan,
> 
> Here are three patches that make improvements to the spinlock handling
> of the mv_xor driver. The first patch fixes a lock/release of the
> channel spinlock in mv_xor_status(), and the next two patches make
> followup code improvements/refactoring.
> 
> Ezequiel, it would be good to have your review on these patches, since
> I know you've spent some time lately doing mv_xor stuff.
> 

Spinlocks aren't the easiest thing to get right, and this driver is not
particularly simple. Anyway, to the best of my knowledge:

Reviewed-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 0/3] dma: mv_xor: spinlock related improvements

[Jason Cooper]

On Fri, Dec 27, 2013 at 12:38:31PM +0100, Thomas Petazzoni wrote:
> Vinod, Dan,
> 
> Here are three patches that make improvements to the spinlock handling
> of the mv_xor driver. The first patch fixes a lock/release of the
> channel spinlock in mv_xor_status(), and the next two patches make
> followup code improvements/refactoring.
> 
> Ezequiel, it would be good to have your review on these patches, since
> I know you've spent some time lately doing mv_xor stuff.
> 
> Thanks!
> 
> Thomas
> 
> Thomas Petazzoni (3):
>   dma: mv_xor: take channel spinlock in mv_xor_status()
>   dma: mv_xor: use __mv_xor_slot_cleanup() in
>     mv_xor_free_chan_resources()
>   dma: mv_xor: rename __mv_xor_slot_cleanup() to mv_xor_slot_cleanup()
> 
>  drivers/dma/mv_xor.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)

for the whole series:

Acked-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.