* [PATCH 0/3] dma: mv_xor: spinlock related improvements
@ 2013-12-27 11:38 Thomas Petazzoni
2013-12-27 11:38 ` [PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status() Thomas Petazzoni
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Thomas Petazzoni @ 2013-12-27 11:38 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status()
2013-12-27 11:38 [PATCH 0/3] dma: mv_xor: spinlock related improvements Thomas Petazzoni
@ 2013-12-27 11:38 ` Thomas Petazzoni
2013-12-30 9:55 ` Shevchenko, Andriy
2013-12-31 22:48 ` Dan Williams
2013-12-27 11:38 ` [PATCH 2/3] dma: mv_xor: use __mv_xor_slot_cleanup() in mv_xor_free_chan_resources() Thomas Petazzoni
` (3 subsequent siblings)
4 siblings, 2 replies; 9+ messages in thread
From: Thomas Petazzoni @ 2013-12-27 11:38 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] dma: mv_xor: use __mv_xor_slot_cleanup() in mv_xor_free_chan_resources()
2013-12-27 11:38 [PATCH 0/3] dma: mv_xor: spinlock related improvements Thomas Petazzoni
2013-12-27 11:38 ` [PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status() Thomas Petazzoni
@ 2013-12-27 11:38 ` Thomas Petazzoni
2013-12-27 11:38 ` [PATCH 3/3] dma: mv_xor: rename __mv_xor_slot_cleanup() to mv_xor_slot_cleanup() Thomas Petazzoni
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Thomas Petazzoni @ 2013-12-27 11:38 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] dma: mv_xor: rename __mv_xor_slot_cleanup() to mv_xor_slot_cleanup()
2013-12-27 11:38 [PATCH 0/3] dma: mv_xor: spinlock related improvements Thomas Petazzoni
2013-12-27 11:38 ` [PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status() Thomas Petazzoni
2013-12-27 11:38 ` [PATCH 2/3] dma: mv_xor: use __mv_xor_slot_cleanup() in mv_xor_free_chan_resources() Thomas Petazzoni
@ 2013-12-27 11:38 ` Thomas Petazzoni
2013-12-27 15:54 ` [PATCH 0/3] dma: mv_xor: spinlock related improvements Ezequiel Garcia
2013-12-27 17:48 ` Jason Cooper
4 siblings, 0 replies; 9+ messages in thread
From: Thomas Petazzoni @ 2013-12-27 11:38 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 0/3] dma: mv_xor: spinlock related improvements
2013-12-27 11:38 [PATCH 0/3] dma: mv_xor: spinlock related improvements Thomas Petazzoni
` (2 preceding siblings ...)
2013-12-27 11:38 ` [PATCH 3/3] dma: mv_xor: rename __mv_xor_slot_cleanup() to mv_xor_slot_cleanup() Thomas Petazzoni
@ 2013-12-27 15:54 ` Ezequiel Garcia
2013-12-27 17:48 ` Jason Cooper
4 siblings, 0 replies; 9+ messages in thread
From: Ezequiel Garcia @ 2013-12-27 15:54 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 0/3] dma: mv_xor: spinlock related improvements
2013-12-27 11:38 [PATCH 0/3] dma: mv_xor: spinlock related improvements Thomas Petazzoni
` (3 preceding siblings ...)
2013-12-27 15:54 ` [PATCH 0/3] dma: mv_xor: spinlock related improvements Ezequiel Garcia
@ 2013-12-27 17:48 ` Jason Cooper
4 siblings, 0 replies; 9+ messages in thread
From: Jason Cooper @ 2013-12-27 17:48 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status()
2013-12-27 11:38 ` [PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status() Thomas Petazzoni
@ 2013-12-30 9:55 ` Shevchenko, Andriy
2013-12-31 22:48 ` Dan Williams
1 sibling, 0 replies; 9+ messages in thread
From: Shevchenko, Andriy @ 2013-12-30 9:55 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status()
2013-12-27 11:38 ` [PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status() Thomas Petazzoni
2013-12-30 9:55 ` Shevchenko, Andriy
@ 2013-12-31 22:48 ` Dan Williams
2014-03-07 15:51 ` Ezequiel Garcia
1 sibling, 1 reply; 9+ messages in thread
From: Dan Williams @ 2013-12-31 22:48 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status()
2013-12-31 22:48 ` Dan Williams
@ 2014-03-07 15:51 ` Ezequiel Garcia
0 siblings, 0 replies; 9+ messages in thread
From: Ezequiel Garcia @ 2014-03-07 15:51 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-03-07 15:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-27 11:38 [PATCH 0/3] dma: mv_xor: spinlock related improvements Thomas Petazzoni
2013-12-27 11:38 ` [PATCH 1/3] dma: mv_xor: take channel spinlock in mv_xor_status() Thomas Petazzoni
2013-12-30 9:55 ` Shevchenko, Andriy
2013-12-31 22:48 ` Dan Williams
2014-03-07 15:51 ` Ezequiel Garcia
2013-12-27 11:38 ` [PATCH 2/3] dma: mv_xor: use __mv_xor_slot_cleanup() in mv_xor_free_chan_resources() Thomas Petazzoni
2013-12-27 11:38 ` [PATCH 3/3] dma: mv_xor: rename __mv_xor_slot_cleanup() to mv_xor_slot_cleanup() Thomas Petazzoni
2013-12-27 15:54 ` [PATCH 0/3] dma: mv_xor: spinlock related improvements Ezequiel Garcia
2013-12-27 17:48 ` Jason Cooper
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).