release() locking

release() locking

[Udo A. Steinberg]

Hi Andrew,

According to Linus' 2.5.1-pre changelog, the release locking changes
introduced in -pre5 are your work. Those changes, however, seem to
break the keyboard driver:

keyboard: Timeout - AT keyboard not present?(f4)

Other people (i.e. Mike Galbraith) have been experiencing the same.

Do you have an updated patch which fixes those issues? -pre6 still
contains the same stuff as -pre5 and if it's broken then Linus should
probably back it out.

Regards,
Udo.

Re: release() locking

[Andrew Morton]

"Udo A. Steinberg" wrote:
> 
> Hi Andrew,
> 
> According to Linus' 2.5.1-pre changelog, the release locking changes
> introduced in -pre5 are your work. Those changes, however, seem to
> break the keyboard driver:
> 
> keyboard: Timeout - AT keyboard not present?(f4)
> 
> Other people (i.e. Mike Galbraith) have been experiencing the same.

wasntmeididntdoit

> Do you have an updated patch which fixes those issues? -pre6 still
> contains the same stuff as -pre5 and if it's broken then Linus should
> probably back it out.

My patch simply fixed a few things like holding spinlocks across
sleeping functions, forgetting to release locks when returning
from functions, etc.

Yes, others have suggested that the whole lot should be reverted,
for several reasons.  However it looks like that won't happen, so we
need to debug the present code.  But it works for me.

I can review the code, see if anything stands out.  But it'll probably
require someone who can reproduce it to be able to fix it.

-

Re: release() locking

[Udo A. Steinberg]

Andrew Morton wrote:

> Yes, others have suggested that the whole lot should be reverted,
> for several reasons.  However it looks like that won't happen, so we
> need to debug the present code.  But it works for me.
> 
> I can review the code, see if anything stands out.  But it'll probably
> require someone who can reproduce it to be able to fix it.

That would be good. I can reliably reproduce the problem, so if you
want me to try out some patches, just send them here.

Regards,
Udo.

Re: release() locking

[David C. Hansen]

Andrew Morton wrote:

>"Udo A. Steinberg" wrote:
>
>>Hi Andrew,
>>
>>According to Linus' 2.5.1-pre changelog, the release locking changes
>>introduced in -pre5 are your work. Those changes, however, seem to
>>break the keyboard driver:
>>
>>keyboard: Timeout - AT keyboard not present?(f4)
>>
>>Other people (i.e. Mike Galbraith) have been experiencing the same.
>>
>wasntmeididntdoit
>
>>Do you have an updated patch which fixes those issues? -pre6 still
>>contains the same stuff as -pre5 and if it's broken then Linus should
>>probably back it out.
>>

I'm responsible for the release locking changes.  But, I don't think 
that the problems are a result of those changes.  There have been some 
other patches that might have caused the problem.  Take a look at this 
thread:

http://marc.theaimsgroup.com/?l=linux-kernel&m=100745930928683&w=2

Jens Axboe posted a patch.  I asked him:
 > So, what was the actual problem?
bio_alloc() not waiting on the reserved pool for free entries, even
though __GFP_WAIT was set. No need for __GFP_IO in that case too.

Udo, did you apply the patch that Jens sent?

Re: release() locking

[Udo A. Steinberg]

"David C. Hansen" wrote:

> I'm responsible for the release locking changes.  But, I don't think
> that the problems are a result of those changes.  There have been some
> other patches that might have caused the problem.  Take a look at this
> thread:

> Jens Axboe posted a patch.  I asked him:
> [...]

The patch that Jens posted was the fix for the oops I posted. The oops
was related to the bio stuff and not to the release() locking. I only
mentioned the keyboard stuff because I initially didn't know if there
was any connection between that and the oops.

> Udo, did you apply the patch that Jens sent?

Yes - and it cured the oops (the one in the thread you refer to), however
the keyboard problems are still there.

I guess there's something wrong with the changes you made, and it only
shows with the modifications that Andrew made - and since he says he
only fixed some bits of the code, the broken bits must have been there
before.

Regards,
Udo.

Re: release() locking

[Andrew Morton]

"Udo A. Steinberg" wrote:
> 
> I guess there's something wrong with the changes you made, and it only
> shows with the modifications that Andrew made - and since he says he
> only fixed some bits of the code, the broken bits must have been there
> before.

Maybe so.  Can you identify the exact kernel version at which
the problem started?

Re: release() locking

[David C. Hansen]

Andrew Morton wrote:

>"Udo A. Steinberg" wrote:
>
>>I guess there's something wrong with the changes you made, and it only
>>shows with the modifications that Andrew made - and since he says he
>>only fixed some bits of the code, the broken bits must have been there
>>before.
>>
>Maybe so.  Can you identify the exact kernel version at which
>the problem started?
>
The release() functions patch went into pre3.  It looks like Jens' bio 
changes went into pre4.  

Re: release() locking

[Udo A. Steinberg]

"David C. Hansen" wrote:
> 
> Andrew Morton wrote:

> >Maybe so.  Can you identify the exact kernel version at which
> >the problem started?

Yes. I tried the entire 2.5.1-pre series:

-pre1 and -pre2 are fine.
-pre3 doesn't compile out of the box and with 3 trivial compile fixes to
      pc_keyb.c shows the problem.

So anything including and after -pre3 is broken wrt. locking.

> The release() functions patch went into pre3.  It looks like Jens' bio
> changes went into pre4.

Like I said before - it has nothing to do with Jens' bio stuff. Also the
fixes done by Andrew have nothing to do with it.
Moreover, if I back out the changes to pc_keyb.c, the problem goes away.

Regards,
Udo.

Re: release() locking

[Andrew Morton]

"Udo A. Steinberg" wrote:
> 
> "David C. Hansen" wrote:
> >
> > Andrew Morton wrote:
> 
> > >Maybe so.  Can you identify the exact kernel version at which
> > >the problem started?
> 
> Yes. I tried the entire 2.5.1-pre series:
> 
> -pre1 and -pre2 are fine.
> -pre3 doesn't compile out of the box and with 3 trivial compile fixes to
>       pc_keyb.c shows the problem.
> 

Hum.  send_data() requires that local interrupts be enabled.

Does this fix it?

--- linux-2.5.1-pre6/drivers/char/pc_keyb.c	Thu Dec  6 20:44:21 2001
+++ 25/drivers/char/pc_keyb.c	Fri Dec  7 15:31:34 2001
@@ -1090,6 +1090,7 @@ static int open_aux(struct inode * inode
 		spin_unlock_irqrestore(&aux_count_lock, flags);
 		return -EBUSY;
 	}
+	spin_unlock_irqrestore(&aux_count_lock, flags);
 	kbd_write_command_w(KBD_CCMD_MOUSE_ENABLE);	/* Enable the
 							   auxiliary port on
 							   controller. */
@@ -1099,7 +1100,6 @@ static int open_aux(struct inode * inode
 	mdelay(2);			/* Ensure we follow the kbc access delay rules.. */
 
 	send_data(KBD_CMD_ENABLE);	/* try to workaround toshiba4030cdt problem */
-	spin_unlock_irqrestore(&aux_count_lock, flags);
 	return 0;
 }

Re: release() locking

[Udo A. Steinberg]

Andrew Morton wrote:
> 
> Hum.  send_data() requires that local interrupts be enabled.
> 
> Does this fix it?

[Patch snipped]

Yes, that fixes it. Thanks! Please submit it to Linus for inclusion
in pre7.

Regards,
Udo.