| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <s5h8ww2r9qg.wl%tiwai@suse.de>
Date: Wed, 16 Jul 2008 01:47:03 +0200
From: Takashi Iwai <tiwai@...e.de>
To: Thadeu Lima de Souza Cascardo <cascardo@...aslivre.org>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Wait for all codecs to be ready if doing a cold reset
At Wed, 9 Jul 2008 14:26:38 -0300,
Thadeu Lima de Souza Cascardo wrote:
>
> On Wed, Jul 09, 2008 at 08:23:12PM +0200, Takashi Iwai wrote:
> > At Tue, 8 Jul 2008 14:31:22 -0300,
> > Thadeu Lima de Souza Cascardo wrote:
> > >
> > > On Wed, Jul 09, 2008 at 05:07:17PM +0200, Takashi Iwai wrote:
> > > > At Tue, 8 Jul 2008 13:59:32 -0300,
> > > > Thadeu Lima de Souza Cascardo wrote:
> > > > >
> > > > > On Tue, Jul 08, 2008 at 12:16:35PM +0200, Takashi Iwai wrote:
> > > > > > At Mon, 7 Jul 2008 14:36:55 -0300,
> > > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > >
> > > > > > > On Mon, Jul 07, 2008 at 06:39:09PM +0200, Takashi Iwai wrote:
> > > > > > > > At Sun, 6 Jul 2008 14:15:56 -0300,
> > > > > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > > > >
> > > > > > > > > If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
> > > > > > > > > initializing the codecs. While resuming, it does not wait for all codecs
> > > > > > > > > to be active. Sound card does not work after a resume without it,
> > > > > > > > > however. This patch fixes it.
> > > > > > > >
> > > > > > > > Thanks for the patch.
> > > > > > > >
> > > > > > > > But, I still don't figure out why this is needed.
> > > > > > > > In the else block (with the comment "resume phase"), you can find the
> > > > > > > > loop to wait for the all *probed* codecs. The difference with the
> > > > > > > > code you moved is that it checks only the bits corresponding to the
> > > > > > > > already probed codecs. In other words, the other bits should be
> > > > > > > > irrelevant with the hardware.
> > > > > > > >
> > > > > > > > I guess it's not about the loop but the additional 1/4 sec delay that
> > > > > > > > did fix the resume on your device. Please check how is the status
> > > > > > > > bits and whether it passed the loop in the middle.
> > > > > > > >
> > > > > > > >
> > > > > > > > Takashi
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > The 1/4 sec delay came in my mind as one of the possible reasons, and
> > > > > > > that's why I've made some tests. status and nstatus are 0x200, while
> > > > > > > codec_isr_bits is 0x300. The loop waits for the status register give us
> > > > > > > 0x300 as the active codecs, instead of the only one probed. Since the
> > > > > > > cold reset in the case of a power saving cleans up all codec registers,
> > > > > > > it is needed that we wait for all codecs again (like in the probe case).
> > > > > >
> > > > > > You loaded the modem driver as well?
> > > > > > If so, what happens if you unload modem driver?
> > > > > >
> > > > > >
> > > > > > thanks,
> > > > > >
> > > > > > Takashi
> > > > > >
> > > > >
> > > > > The modem driver has always been loaded. Unloading it with my patch
> > > > > works OK.
> > > >
> > > > Well, I meant to unload the modem driver *without* your patch.
> > > > That is, does it the unmodified version work if you unload the modem
> > > > driver beforehand (e.g. adding it to blacklist)?
> > > >
> > > >
> > > > thanks,
> > > >
> > > > Takashi
> > > >
> > >
> > > I blacklisted snd_intel8x0m, rebooted, it was not loaded, sound worked.
> > > Suspended, resumed, sound didn't work, as usual.
> >
> > Thanks for checking.
> >
> > I still don't understand why you need to wait for all codecs.
> > Certainly the secondary one is the modem codec, and it has nothing to
> > do with the audio codec...
> >
> > I don't mind to apply the patch as is since it's harmless except for
> > the extra delay. But, the real question is whether it's the codec
> > probe or just another delay.
> >
> > Note that not all devices have codecs filling all slots like yours.
> > On the later ICH, it has three slots while usually two of them are
> > used. So, on these hardwares, your code results always in just a 25ms
> > delay.
> >
> > Anyway, could you give your sign-off?
> >
> >
> > thanks,
> >
> > Takashi
>
>
> Maybe we could treat this as a quirk in my device, which is 8086:2485?
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@...aslivre.org>
Thanks.
Meanwhile, I reconsidered about this problem. I think the check of
the whole "active" codec slots can be done better in a way like the
following. Could you give it a try?
If this still doesn't work, I suspect it's really the matter of
additional delay.
thanks,
Takashi
---
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 048d99e..a282c7c 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -404,6 +404,7 @@ struct intel8x0 {
unsigned int *codec_bit;
unsigned int codec_isr_bits;
unsigned int codec_ready_bits;
+ unsigned int codec_init_bits;
spinlock_t reg_lock;
@@ -2278,7 +2279,7 @@ static void do_ali_reset(struct intel8x0 *chip)
static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
{
unsigned long end_time;
- unsigned int cnt, status, nstatus;
+ unsigned int cnt, status;
/* put logic to right state */
/* first clear status bits */
@@ -2344,20 +2345,15 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
status |= igetdword(chip, ICHREG(GLOB_STA)) &
chip->codec_isr_bits;
}
-
+ chip->codec_init_bits = status;
} else {
/* resume phase */
- int i;
- status = 0;
- for (i = 0; i < chip->ncodecs; i++)
- if (chip->ac97[i])
- status |= chip->codec_bit[chip->ac97_sdin[i]];
/* wait until all the probed codecs are ready */
end_time = jiffies + HZ;
do {
- nstatus = igetdword(chip, ICHREG(GLOB_STA)) &
+ status = igetdword(chip, ICHREG(GLOB_STA)) &
chip->codec_isr_bits;
- if (status == nstatus)
+ if (status == chip->codec_init_bits)
break;
schedule_timeout_uninterruptible(1);
} while (time_after_eq(end_time, jiffies));
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists