[PATCH] sa1111: Prevent deadlock in resume path

[PATCH] sa1111: Prevent deadlock in resume path

[Marek Vasut]

This patch reorganises the sa1111_resume() function in a manner the spinlock
happens after calling the sa1111_wake(). This fixes two bugs:

1) This function called sa1111_wake() which tried to claim the same spinlock
   the sa1111_resume() already claimed. This would result in certain deadlock.

   Original idea for this part: Russell King <linux@arm.linux.org.uk>

2) The function didn't unlock the spinlock in case the chip didn't report
   correct ID.

   Original idea for this part: Julia Lawall <julia@diku.dk>

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 arch/arm/common/sa1111.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
index a52a27c..6f80665 100644
--- a/arch/arm/common/sa1111.c
+++ b/arch/arm/common/sa1111.c
@@ -951,8 +951,6 @@ static int sa1111_resume(struct platform_device *dev)
 	if (!save)
 		return 0;
 
-	spin_lock_irqsave(&sachip->lock, flags);
-
 	/*
 	 * Ensure that the SA1111 is still here.
 	 * FIXME: shouldn't do this here.
@@ -969,6 +967,13 @@ static int sa1111_resume(struct platform_device *dev)
 	 * First of all, wake up the chip.
 	 */
 	sa1111_wake(sachip);
+
+	/*
+	 * Only lock for write ops. Also, sa1111_wake must be called with
+	 * released spinlock!
+	 */
+	spin_lock_irqsave(&sachip->lock, flags);
+
 	sa1111_writel(0, sachip->base + SA1111_INTC + SA1111_INTEN0);
 	sa1111_writel(0, sachip->base + SA1111_INTC + SA1111_INTEN1);
 
-- 
1.7.0

[PATCH] sa1111: Prevent deadlock in resume path

[Russell King - ARM Linux]

On Wed, May 26, 2010 at 09:11:44PM +0200, Marek Vasut wrote:
> This patch reorganises the sa1111_resume() function in a manner the spinlock
> happens after calling the sa1111_wake(). This fixes two bugs:
> 
> 1) This function called sa1111_wake() which tried to claim the same spinlock
>    the sa1111_resume() already claimed. This would result in certain deadlock.
> 
>    Original idea for this part: Russell King <linux@arm.linux.org.uk>
> 
> 2) The function didn't unlock the spinlock in case the chip didn't report
>    correct ID.
> 
>    Original idea for this part: Julia Lawall <julia@diku.dk>
> 
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>

Yea, good enough.

[PATCH] sa1111: Prevent deadlock in resume path

[Marek Vasut]

Dne St 26. kv?tna 2010 21:14:25 Russell King - ARM Linux napsal(a):
> On Wed, May 26, 2010 at 09:11:44PM +0200, Marek Vasut wrote:
> > This patch reorganises the sa1111_resume() function in a manner the
> > spinlock happens after calling the sa1111_wake(). This fixes two bugs:
> > 
> > 1) This function called sa1111_wake() which tried to claim the same
> > spinlock
> > 
> >    the sa1111_resume() already claimed. This would result in certain
> >    deadlock.
> >    
> >    Original idea for this part: Russell King <linux@arm.linux.org.uk>
> > 
> > 2) The function didn't unlock the spinlock in case the chip didn't report
> > 
> >    correct ID.
> >    
> >    Original idea for this part: Julia Lawall <julia@diku.dk>
> > 
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> 
> Yea, good enough.

You want me to fight your patch tracking system or will you just merge it into 
your tree ?

[PATCH] sa1111: Prevent deadlock in resume path

[Russell King - ARM Linux]

On Wed, May 26, 2010 at 09:15:57PM +0200, Marek Vasut wrote:
> Dne St 26. kv?tna 2010 21:14:25 Russell King - ARM Linux napsal(a):
> > On Wed, May 26, 2010 at 09:11:44PM +0200, Marek Vasut wrote:
> > > This patch reorganises the sa1111_resume() function in a manner the
> > > spinlock happens after calling the sa1111_wake(). This fixes two bugs:
> > > 
> > > 1) This function called sa1111_wake() which tried to claim the same
> > > spinlock
> > > 
> > >    the sa1111_resume() already claimed. This would result in certain
> > >    deadlock.
> > >    
> > >    Original idea for this part: Russell King <linux@arm.linux.org.uk>
> > > 
> > > 2) The function didn't unlock the spinlock in case the chip didn't report
> > > 
> > >    correct ID.
> > >    
> > >    Original idea for this part: Julia Lawall <julia@diku.dk>
> > > 
> > > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > 
> > Yea, good enough.
> 
> You want me to fight your patch tracking system or will you just merge it into 
> your tree ?

What do you mean "fight" ?  Just send a standard git formatted patch
to the email address with an additional KernelVersion: tag.  It's
not at all hard.

[PATCH] sa1111: Prevent deadlock in resume path

[Marek Vasut]

Dne St 26. kv?tna 2010 22:18:24 Russell King - ARM Linux napsal(a):
> On Wed, May 26, 2010 at 09:15:57PM +0200, Marek Vasut wrote:
> > Dne St 26. kv?tna 2010 21:14:25 Russell King - ARM Linux napsal(a):
> > > On Wed, May 26, 2010 at 09:11:44PM +0200, Marek Vasut wrote:
> > > > This patch reorganises the sa1111_resume() function in a manner the
> > > > spinlock happens after calling the sa1111_wake(). This fixes two
> > > > bugs:
> > > > 
> > > > 1) This function called sa1111_wake() which tried to claim the same
> > > > spinlock
> > > > 
> > > >    the sa1111_resume() already claimed. This would result in certain
> > > >    deadlock.
> > > >    
> > > >    Original idea for this part: Russell King <linux@arm.linux.org.uk>
> > > > 
> > > > 2) The function didn't unlock the spinlock in case the chip didn't
> > > > report
> > > > 
> > > >    correct ID.
> > > >    
> > > >    Original idea for this part: Julia Lawall <julia@diku.dk>
> > > > 
> > > > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > > 
> > > Yea, good enough.
> > 
> > You want me to fight your patch tracking system or will you just merge it
> > into your tree ?
> 
> What do you mean "fight" ?  Just send a standard git formatted patch
> to the email address with an additional KernelVersion: tag.  It's
> not at all hard.

What address, how, any howto?

btw. why ?! Look at Eric, he doesn't need such crap, why do you complicate it so 
much ?

[PATCH] sa1111: Prevent deadlock in resume path

[Russell King - ARM Linux]

On Wed, May 26, 2010 at 11:33:46PM +0200, Marek Vasut wrote:
> Dne St 26. kv?tna 2010 22:18:24 Russell King - ARM Linux napsal(a):
> > On Wed, May 26, 2010 at 09:15:57PM +0200, Marek Vasut wrote:
> > > Dne St 26. kv?tna 2010 21:14:25 Russell King - ARM Linux napsal(a):
> > > > On Wed, May 26, 2010 at 09:11:44PM +0200, Marek Vasut wrote:
> > > > > This patch reorganises the sa1111_resume() function in a manner the
> > > > > spinlock happens after calling the sa1111_wake(). This fixes two
> > > > > bugs:
> > > > > 
> > > > > 1) This function called sa1111_wake() which tried to claim the same
> > > > > spinlock
> > > > > 
> > > > >    the sa1111_resume() already claimed. This would result in certain
> > > > >    deadlock.
> > > > >    
> > > > >    Original idea for this part: Russell King <linux@arm.linux.org.uk>
> > > > > 
> > > > > 2) The function didn't unlock the spinlock in case the chip didn't
> > > > > report
> > > > > 
> > > > >    correct ID.
> > > > >    
> > > > >    Original idea for this part: Julia Lawall <julia@diku.dk>
> > > > > 
> > > > > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > > > 
> > > > Yea, good enough.
> > > 
> > > You want me to fight your patch tracking system or will you just merge it
> > > into your tree ?
> > 
> > What do you mean "fight" ?  Just send a standard git formatted patch
> > to the email address with an additional KernelVersion: tag.  It's
> > not at all hard.
> 
> What address, how, any howto?
> 
> btw. why ?! Look at Eric, he doesn't need such crap, why do you complicate it so 
> much ?

You expect a helpful reply with such an attitude.  Sorry, I'm not going
to bother with attacks like this.  You earn the prize of figuring it out
yourself.

[PATCH] sa1111: Prevent deadlock in resume path

[Marek Vasut]

Dne St 26. kv?tna 2010 23:42:36 Russell King - ARM Linux napsal(a):
> On Wed, May 26, 2010 at 11:33:46PM +0200, Marek Vasut wrote:
> > Dne St 26. kv?tna 2010 22:18:24 Russell King - ARM Linux napsal(a):
> > > On Wed, May 26, 2010 at 09:15:57PM +0200, Marek Vasut wrote:
> > > > Dne St 26. kv?tna 2010 21:14:25 Russell King - ARM Linux napsal(a):
> > > > > On Wed, May 26, 2010 at 09:11:44PM +0200, Marek Vasut wrote:
> > > > > > This patch reorganises the sa1111_resume() function in a manner
> > > > > > the spinlock happens after calling the sa1111_wake(). This fixes
> > > > > > two bugs:
> > > > > > 
> > > > > > 1) This function called sa1111_wake() which tried to claim the
> > > > > > same spinlock
> > > > > > 
> > > > > >    the sa1111_resume() already claimed. This would result in
> > > > > >    certain deadlock.
> > > > > >    
> > > > > >    Original idea for this part: Russell King
> > > > > >    <linux@arm.linux.org.uk>
> > > > > > 
> > > > > > 2) The function didn't unlock the spinlock in case the chip
> > > > > > didn't report
> > > > > > 
> > > > > >    correct ID.
> > > > > >    
> > > > > >    Original idea for this part: Julia Lawall <julia@diku.dk>
> > > > > > 
> > > > > > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > > > > 
> > > > > Yea, good enough.
> > > > 
> > > > You want me to fight your patch tracking system or will you just
> > > > merge it into your tree ?
> > > 
> > > What do you mean "fight" ?  Just send a standard git formatted patch
> > > to the email address with an additional KernelVersion: tag.  It's
> > > not at all hard.
> > 
> > What address, how, any howto?
> > 
> > btw. why ?! Look at Eric, he doesn't need such crap, why do you
> > complicate it so much ?
> 
> You expect a helpful reply with such an attitude.  Sorry, I'm not going
> to bother with attacks like this.  You earn the prize of figuring it out
> yourself.

Try reading past the three dots please.

You don't want the patch? Fine by me ... Anyway, please, open your eyes, look 
around, noone in the whole community does such complications. Why do you do 
that? What does it help? Why can't you just pick patches as everyone else does 
-- from email. That's why git supports it.

[PATCH] sa1111: Prevent deadlock in resume path

[Russell King - ARM Linux]

On Wed, May 26, 2010 at 11:52:33PM +0200, Marek Vasut wrote:
> You don't want the patch? Fine by me ... Anyway, please, open your eyes, look 
> around, noone in the whole community does such complications. Why do you do 
> that? What does it help? Why can't you just pick patches as everyone else does 
> -- from email. That's why git supports it.

What you're saying is that *everyone* has to conform to your way of
working.  Sorry, the human race isn't like that.  Humans are individuals,
and each one has their own quirks.

Mine is that I'm dreadful at dealing with patches in email.  Unless I
deal with a message at the point I've read it, it basically doesn't
exist.  So I created the patch system 10 years ago to solve that problem
- to keep patches around and visible, *and* to make them easier for me
to merge.

So your choice is:

1. send patches by email, but have to repeatedly send them to get them
   applied.

2. send the patch to the patch system so that it's easily visible and
   doesn't get buried beneath a mountain of email.

Lastly, I don't do kernel work 7 days a week.  I do have time off when I
don't look at the kernel tree, but I still read email.  Maybe you'd
prefer me to ignore all email on those days I'm not working instead?

The patch system is there to _solve_ a problem.  If you don't want to
use it, the fine, don't expect that problem to be solved for your
patches.

And now, at this point I really don't have any more time for you and
these stupid, idiotic and childish politics.

[PATCH] sa1111: Prevent deadlock in resume path

[Marek Vasut]

Dne ?t 27. kv?tna 2010 00:25:30 Russell King - ARM Linux napsal(a):
> On Wed, May 26, 2010 at 11:52:33PM +0200, Marek Vasut wrote:
> > You don't want the patch? Fine by me ... Anyway, please, open your eyes,
> > look around, noone in the whole community does such complications. Why
> > do you do that? What does it help? Why can't you just pick patches as
> > everyone else does -- from email. That's why git supports it.
> 
> What you're saying is that *everyone* has to conform to your way of
> working. 

Come on, you know it's not what I said.

> Sorry, the human race isn't like that.  Humans are individuals,
> and each one has their own quirks.

Fine by me, there aren't many people I have problems with.
> 
> Mine is that I'm dreadful at dealing with patches in email.  Unless I
> deal with a message at the point I've read it, it basically doesn't
> exist. 

Why don't you set up a mailbox for that ? You read the emails anyway and you can 
drop the patches you Ack into that mailbox ... then run git am on that once in a 
while.
> So I created the patch system 10 years ago to solve that problem
> - to keep patches around and visible, *and* to make them easier for me
> to merge.

Things changed ever since then ...
> 
> So your choice is:
> 
> 1. send patches by email, but have to repeatedly send them to get them
>    applied.
> 
> 2. send the patch to the patch system so that it's easily visible and
>    doesn't get buried beneath a mountain of email.
> 
> Lastly, I don't do kernel work 7 days a week. 

Neither do I, nor am I paid for it. I do it in my free time.

> I do have time off when I
> don't look at the kernel tree, but I still read email. Maybe you'd
> prefer me to ignore all email on those days I'm not working instead?

See above please.
> 
> The patch system is there to _solve_ a problem. 

And it's there to create another one.

> If you don't want to use it, the fine, don't expect that problem to be solved 
> for your patches.

The problem should be solved already. This discussion was here already, it 
repeats and people complain.
> 
> And now, at this point I really don't have any more time for you and
> these stupid, idiotic and childish politics.

I'm just trying to solve a real problem here.

[PATCH] sa1111: Prevent deadlock in resume path

[Nicolas Pitre]

On Thu, 27 May 2010, Marek Vasut wrote:

> Dne ?t 27. kv?tna 2010 00:25:30 Russell King - ARM Linux napsal(a):
> > Mine is that I'm dreadful at dealing with patches in email.  Unless I
> > deal with a message at the point I've read it, it basically doesn't
> > exist. 
> 
> Why don't you set up a mailbox for that ? You read the emails anyway and you can 
> drop the patches you Ack into that mailbox ... then run git am on that once in a 
> while.

Marek, let me give you some bit of wisdom.

Russell's patch system existed way before git was created.  It works 
quite well for random patches.  He even created custom scripts to 
extract patches from his system and stuff them into git.  So if he went 
through such trouble instead of relying on the standard 'git am' tool 
with a patch mailbox then there must be a reason, even if that reason 
makes sense only to him.

The impact on you (and I) is rather minimal.  The process is indeed 
documented at length on his website but the actual operation is not much 
different from posting the patch to this mailing list.  And I'm glad to 
make my own life a tiny bit more complicated due to having to email the 
same patch twice (once here and once to the patch system) if that makes 
a big difference to Russell's life.

If you do have a real problem with that system, then just create your 
own git tree and ask RMK to pull it.  That has worked quite well too, 
especially for large patch series.

So please just accept that the ARM process is odd and deal with it.  
The day you end up maintaining a body of code with people routing their 
patches through you then it will be your turn to set the rules for them.


Nicolas

[PATCH] sa1111: Prevent deadlock in resume path

[Pavel Machek]

On Wed 2010-05-26 21:18:24, Russell King - ARM Linux wrote:
> On Wed, May 26, 2010 at 09:15:57PM +0200, Marek Vasut wrote:
> > Dne St 26. kv?tna 2010 21:14:25 Russell King - ARM Linux napsal(a):
> > > On Wed, May 26, 2010 at 09:11:44PM +0200, Marek Vasut wrote:
> > > > This patch reorganises the sa1111_resume() function in a manner the
> > > > spinlock happens after calling the sa1111_wake(). This fixes two bugs:
> > > > 
> > > > 1) This function called sa1111_wake() which tried to claim the same
> > > > spinlock
> > > > 
> > > >    the sa1111_resume() already claimed. This would result in certain
> > > >    deadlock.
> > > >    
> > > >    Original idea for this part: Russell King <linux@arm.linux.org.uk>
> > > > 
> > > > 2) The function didn't unlock the spinlock in case the chip didn't report
> > > > 
> > > >    correct ID.
> > > >    
> > > >    Original idea for this part: Julia Lawall <julia@diku.dk>
> > > > 
> > > > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > > 
> > > Yea, good enough.
> > 
> > You want me to fight your patch tracking system or will you just merge it into 
> > your tree ?
> 
> What do you mean "fight" ?  Just send a standard git formatted patch
> to the email address with an additional KernelVersion: tag.  It's
> not at all hard.

You are linux kernel maintainer. Start acting as one. It is not at all
hard.

Alternatively, just remove KernelVersion: checking in  your
scripts.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] sa1111: Prevent deadlock in resume path

[Marek Vasut]

Dne ?t 10. ?ervna 2010 15:56:10 Pavel Machek napsal(a):
> On Wed 2010-05-26 21:18:24, Russell King - ARM Linux wrote:
> > On Wed, May 26, 2010 at 09:15:57PM +0200, Marek Vasut wrote:
> > > Dne St 26. kv?tna 2010 21:14:25 Russell King - ARM Linux napsal(a):
> > > > On Wed, May 26, 2010 at 09:11:44PM +0200, Marek Vasut wrote:
> > > > > This patch reorganises the sa1111_resume() function in a manner the
> > > > > spinlock happens after calling the sa1111_wake(). This fixes two
> > > > > bugs:
> > > > > 
> > > > > 1) This function called sa1111_wake() which tried to claim the same
> > > > > spinlock
> > > > > 
> > > > >    the sa1111_resume() already claimed. This would result in
> > > > >    certain deadlock.
> > > > >    
> > > > >    Original idea for this part: Russell King
> > > > >    <linux@arm.linux.org.uk>
> > > > > 
> > > > > 2) The function didn't unlock the spinlock in case the chip didn't
> > > > > report
> > > > > 
> > > > >    correct ID.
> > > > >    
> > > > >    Original idea for this part: Julia Lawall <julia@diku.dk>
> > > > > 
> > > > > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > > > 
> > > > Yea, good enough.
> > > 
> > > You want me to fight your patch tracking system or will you just merge
> > > it into your tree ?
> > 
> > What do you mean "fight" ?  Just send a standard git formatted patch
> > to the email address with an additional KernelVersion: tag.  It's
> > not at all hard.
> 
> You are linux kernel maintainer. Start acting as one. It is not at all
> hard.
> 
> Alternatively, just remove KernelVersion: checking in  your
> scripts.
> 
> 									Pavel

Even though my problem was fixed (yeah, sorry Russell, I didn't know the patch 
tracker was updated to support git-send-email), I have a question though.

I believe that Kernel-version is unnecessary. Isn't this information already 
encoded in the git-send-emailed patch?

btw. guys, before another burning discussion emerges, if you two plan to make 
your fights into a new sports discipline, I won't mind taking the role of a 
spectator ;-) Some possible criteria would be a length of the replying email or 
number of rough words ;-)

[PATCH] sa1111: Prevent deadlock in resume path

[Russell King - ARM Linux]

On Thu, Jun 10, 2010 at 11:49:12PM +0200, Marek Vasut wrote:
> Even though my problem was fixed (yeah, sorry Russell, I didn't know
> the patch tracker was updated to support git-send-email), I have a
> question though.
> 
> I believe that Kernel-version is unnecessary. Isn't this information already 
> encoded in the git-send-emailed patch?

It's there so that gnu patches can be accepted, and the additional
information does help when applying hard to apply patches, particularly
if either the patch has aged or was generated against an old kernel
version.

Not only that, but it was once critical when we had 2.4 and 2.5/2.6
kernel patches going via the patch system.

Last point to make - and it's in similar vain to the KernelVersion
tag - is that it would be useful if people would tag pure fixes which
should be applied to previous kernels with a "Cc: <stable@kernel.org>"
tag along-side the existing sign-offs.  That allows the stable
maintainers to automatically pick up on these patches when they're
merged into mainline.

[PATCH] sa1111: Prevent deadlock in resume path

[Catalin Marinas]

On Thu, 2010-06-10 at 14:56 +0100, Pavel Machek wrote:
> On Wed 2010-05-26 21:18:24, Russell King - ARM Linux wrote:
> > On Wed, May 26, 2010 at 09:15:57PM +0200, Marek Vasut wrote:
> > > You want me to fight your patch tracking system or will you just merge it into
> > > your tree ?
> >
> > What do you mean "fight" ?  Just send a standard git formatted patch
> > to the email address with an additional KernelVersion: tag.  It's
> > not at all hard.
> 
> You are linux kernel maintainer. Start acting as one. It is not at all
> hard.
> 
> Alternatively, just remove KernelVersion: checking in  your
> scripts.

BTW (and I'm not complaining about Russell's patch system here), how can
we get the linux-arm-kernel list subscribed to patchwork.kernel.org?
That would be useful for grabbing RFC/CFT patches.

-- 
Catalin

[PATCH] sa1111: Prevent deadlock in resume path

[Eric Miao]

On Fri, Jun 11, 2010 at 8:22 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Thu, 2010-06-10 at 14:56 +0100, Pavel Machek wrote:
>> On Wed 2010-05-26 21:18:24, Russell King - ARM Linux wrote:
>> > On Wed, May 26, 2010 at 09:15:57PM +0200, Marek Vasut wrote:
>> > > You want me to fight your patch tracking system or will you just merge it into
>> > > your tree ?
>> >
>> > What do you mean "fight" ? ?Just send a standard git formatted patch
>> > to the email address with an additional KernelVersion: tag. ?It's
>> > not at all hard.
>>
>> You are linux kernel maintainer. Start acting as one. It is not at all
>> hard.
>>
>> Alternatively, just remove KernelVersion: checking in ?your
>> scripts.
>
> BTW (and I'm not complaining about Russell's patch system here), how can
> we get the linux-arm-kernel list subscribed to patchwork.kernel.org?
> That would be useful for grabbing RFC/CFT patches.
>

CC'ed Jeremy Kerr.

Jeremy,

Is it OK to add linux-arm-kernel?