[PATCH v2] intel8x0m: wait a bit before warm reset check

[PATCH v2] intel8x0m: wait a bit before warm reset check

[Paul Bolle]

At every resume a laptop I use prints this message (at KERN_ERR level):
    ALSA sound/pci/intel8x0m.c:904: AC'97 warm reset still in progress? [0x2]

The thing to note here is that 0x2 corresponds to ICH_AC97COLD. Ie, what
seems to be happening is that the register involved indicated a warm
reset for some time (as the ICH_AC97WARM bit was set) but by the time
the warning is printed, and that same register is checked again, that
bit is already cleared and only the ICH_AC97COLD bit is still set.

It turns out a warm reset needs some time to settle, but it is currently
checked right away. The test therefore fails the first time it is done
and schedule_timeout_uninterruptible() will be called. Once we return
from that jiffies is already (far) past end_time on this laptop, so we
exit the loop, print a warning, and exit the function while the warm
reset actually succeeded.

One way to fix this is to call udelay() after writing to the register
involved. A handful of tests suggests 500 usecs is a safe value. (This
might punish the "finish cold reset" case, but on this laptop such a
cold reset apparently never happens, so I can't say for sure.)

While we're at it drop the extra single tick from end_time, as it looks
rather silly.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
0) v1 was called " intel8x0m: schedule timeout before warm reset check".

1) A udelay of 250 usecs always failed while 300 usecs always worked. So
500 usecs might be a bit cautious.

 sound/pci/intel8x0m.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/sound/pci/intel8x0m.c b/sound/pci/intel8x0m.c
index 13cec1e..62fad24 100644
--- a/sound/pci/intel8x0m.c
+++ b/sound/pci/intel8x0m.c
@@ -894,7 +894,8 @@ static int snd_intel8x0m_ich_chip_init(struct intel8x0m *chip, int probing)
 	/* finish cold or do warm reset */
 	cnt |= (cnt & ICH_AC97COLD) == 0 ? ICH_AC97COLD : ICH_AC97WARM;
 	iputdword(chip, ICHREG(GLOB_CNT), cnt);
-	end_time = (jiffies + (HZ / 4)) + 1;
+	udelay(500);
+	end_time = jiffies + HZ / 4;
 	do {
 		if ((igetdword(chip, ICHREG(GLOB_CNT)) & ICH_AC97WARM) == 0)
 			goto __ok;
-- 
1.7.4.1

Re: [PATCH v2] intel8x0m: wait a bit before warm reset check

[Jesper Juhl]

On Sun, 6 Mar 2011, Paul Bolle wrote:

> At every resume a laptop I use prints this message (at KERN_ERR level):
>     ALSA sound/pci/intel8x0m.c:904: AC'97 warm reset still in progress? [0x2]
> 
> The thing to note here is that 0x2 corresponds to ICH_AC97COLD. Ie, what
> seems to be happening is that the register involved indicated a warm
> reset for some time (as the ICH_AC97WARM bit was set) but by the time
> the warning is printed, and that same register is checked again, that
> bit is already cleared and only the ICH_AC97COLD bit is still set.
> 
> It turns out a warm reset needs some time to settle, but it is currently
> checked right away. The test therefore fails the first time it is done
> and schedule_timeout_uninterruptible() will be called. Once we return
> from that jiffies is already (far) past end_time on this laptop, so we
> exit the loop, print a warning, and exit the function while the warm
> reset actually succeeded.
> 
> One way to fix this is to call udelay() after writing to the register
> involved. A handful of tests suggests 500 usecs is a safe value. (This
> might punish the "finish cold reset" case, but on this laptop such a
> cold reset apparently never happens, so I can't say for sure.)
> 
> While we're at it drop the extra single tick from end_time, as it looks
> rather silly.
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
> 0) v1 was called " intel8x0m: schedule timeout before warm reset check".
> 
> 1) A udelay of 250 usecs always failed while 300 usecs always worked. So
> 500 usecs might be a bit cautious.
> 

Wouldn't it be nice to put these notes in a comment in the code so that 
future readers don't have to scratch their heads and wonder where the 
udelay() with some magical constant comes from?

Also, if udelay(300) always worked in your tests, wouldn't udelay(400) be 
more than enough to provide a nice fat safety margin (33% more than 
"always works")? Are there no official documents that specify how long 
this is expected to take that we could base this delay on instead of just 
testing on one system?

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html

Re: [PATCH v2] intel8x0m: wait a bit before warm reset check

[Takashi Iwai]

At Sun, 6 Mar 2011 01:12:26 +0100 (CET),
Jesper Juhl wrote:
> 
> On Sun, 6 Mar 2011, Paul Bolle wrote:
> 
> > At every resume a laptop I use prints this message (at KERN_ERR level):
> >     ALSA sound/pci/intel8x0m.c:904: AC'97 warm reset still in progress? [0x2]
> > 
> > The thing to note here is that 0x2 corresponds to ICH_AC97COLD. Ie, what
> > seems to be happening is that the register involved indicated a warm
> > reset for some time (as the ICH_AC97WARM bit was set) but by the time
> > the warning is printed, and that same register is checked again, that
> > bit is already cleared and only the ICH_AC97COLD bit is still set.
> > 
> > It turns out a warm reset needs some time to settle, but it is currently
> > checked right away. The test therefore fails the first time it is done
> > and schedule_timeout_uninterruptible() will be called. Once we return
> > from that jiffies is already (far) past end_time on this laptop, so we
> > exit the loop, print a warning, and exit the function while the warm
> > reset actually succeeded.
> > 
> > One way to fix this is to call udelay() after writing to the register
> > involved. A handful of tests suggests 500 usecs is a safe value. (This
> > might punish the "finish cold reset" case, but on this laptop such a
> > cold reset apparently never happens, so I can't say for sure.)
> > 
> > While we're at it drop the extra single tick from end_time, as it looks
> > rather silly.
> > 
> > Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> > ---
> > 0) v1 was called " intel8x0m: schedule timeout before warm reset check".
> > 
> > 1) A udelay of 250 usecs always failed while 300 usecs always worked. So
> > 500 usecs might be a bit cautious.
> > 
> 
> Wouldn't it be nice to put these notes in a comment in the code so that 
> future readers don't have to scratch their heads and wonder where the 
> udelay() with some magical constant comes from?

That'd be better, indeed.

> Also, if udelay(300) always worked in your tests, wouldn't udelay(400) be 
> more than enough to provide a nice fat safety margin (33% more than 
> "always works")?

Well...  I think you know how meaningless to quote a percentage
representation in such a case...

> Are there no official documents that specify how long 
> this is expected to take that we could base this delay on instead of just 
> testing on one system?

Heh, can you really trust such a document? ;)

It's basically a workaround for a specific codec chip that doesn't set
the bit reflecting to the right status after resume/power-up.  That
is, it's just for the codec chip on Paul's machine.  Others work well
without this delay.

Also, as mentioned earlier, it's better to use usleep_range() than
udelay().  There is no hard upper-limit in this case, and any longer
sleep works.


thanks,

Takashi

Re: [PATCH v2] intel8x0m: wait a bit before warm reset check

[Paul Bolle]

On Mon, 2011-03-07 at 10:55 +0100, Takashi Iwai wrote:
> Also, as mentioned earlier, it's better to use usleep_range() than
> udelay().  There is no hard upper-limit in this case, and any longer
> sleep works.

So something like, say, usleep_range(500, 1000) should be OK?


Paul

Re: [PATCH v2] intel8x0m: wait a bit before warm reset check

[Takashi Iwai]

At Mon, 07 Mar 2011 11:36:34 +0100,
Paul Bolle wrote:
> 
> On Mon, 2011-03-07 at 10:55 +0100, Takashi Iwai wrote:
> > Also, as mentioned earlier, it's better to use usleep_range() than
> > udelay().  There is no hard upper-limit in this case, and any longer
> > sleep works.
> 
> So something like, say, usleep_range(500, 1000) should be OK?

Yes.  It can be longer (as it's no path with critical time-out).


Takashi