* [BUG] unsafe reset in ac97_codec.c
@ 2004-02-03 15:42 Liam Girdwood
2004-02-04 21:24 ` Jeff Garzik
0 siblings, 1 reply; 6+ messages in thread
From: Liam Girdwood @ 2004-02-03 15:42 UTC (permalink / raw)
To: Jeff Garzik, Linux Kernel
Hi
Does anyone know why ac97_codec.c does a register reset of the codec
during device probing. i.e.
/* probing AC97 codec, AC97 2.0 says that bit 15 of register 0x00
(reset) should
* be read zero.
*
* FIXME: is the following comment outdated? -jgarzik
* Probing of AC97 in this way is not reliable, it is not even SAFE !!
*/
codec->codec_write(codec, AC97_RESET, 0L);
IMO, this is unsafe because it can also reset the codec into it's
default power state which can be "power down". This is not normally a
problem for PC's, but some battery powered devices have the default
codec state as "power down" to conserve power.
Was this introduced as a workaround for some buggy device ?
If no one objects I'll submit a patch.
Liam
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] unsafe reset in ac97_codec.c
2004-02-03 15:42 [BUG] unsafe reset in ac97_codec.c Liam Girdwood
@ 2004-02-04 21:24 ` Jeff Garzik
2004-02-05 16:31 ` Liam Girdwood
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Garzik @ 2004-02-04 21:24 UTC (permalink / raw)
To: Liam Girdwood; +Cc: Linux Kernel, Alan Cox
Liam Girdwood wrote:
> /* probing AC97 codec, AC97 2.0 says that bit 15 of register 0x00
> (reset) should
> * be read zero.
> *
> * FIXME: is the following comment outdated? -jgarzik
> * Probing of AC97 in this way is not reliable, it is not even SAFE !!
> */
> codec->codec_write(codec, AC97_RESET, 0L);
>
>
> IMO, this is unsafe because it can also reset the codec into it's
> default power state which can be "power down". This is not normally a
> problem for PC's, but some battery powered devices have the default
> codec state as "power down" to conserve power.
>
> Was this introduced as a workaround for some buggy device ?
> If no one objects I'll submit a patch.
In general it's important for Linux to be able to reset a device
reliable. Where in Other Operating Systems one must reboot the
computer, Linux users can just re-load the driver quite often.
So I think there are two comments here:
* I can certainly see -probing- being unreliable (but not necessarily reset)
* If the default state for some devices is power-down, the driver should
be aware of that -anyway-, and we should power up on startup or on-demand.
Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] unsafe reset in ac97_codec.c
2004-02-04 21:24 ` Jeff Garzik
@ 2004-02-05 16:31 ` Liam Girdwood
2004-02-05 17:59 ` Alan Cox
0 siblings, 1 reply; 6+ messages in thread
From: Liam Girdwood @ 2004-02-05 16:31 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Linux Kernel, Alan Cox
On Wed, 2004-02-04 at 21:24, Jeff Garzik wrote:
> In general it's important for Linux to be able to reset a device
> reliable. Where in Other Operating Systems one must reboot the
> computer, Linux users can just re-load the driver quite often.
>
> So I think there are two comments here:
>
> * I can certainly see -probing- being unreliable (but not necessarily reset)
I agree, but I think we need to be aware of the codec type before we do
a register reset. This type of codec is now becoming popular in PDA's.
>
> * If the default state for some devices is power-down, the driver should
> be aware of that -anyway-, and we should power up on startup or on-demand.
>
I can see another problem with the current probe implementation.
Currently it sends the register reset command without first checking the
codec ready bit. This assumes that the AC97 link is up and completely
working before probe is called.
I would like to suggest some changes to probe:-
1. We have a new flag AC97_DEFAULT_POWER_OFF in ac97_codec_ids[] to mark
codecs that have a default power state of "off".
2. Probe checks the codec ready bit (or waits 10uS) first before doing
anything else.
3. Probe reads the codec ID (hardwired codec registers) and if the codec
is of type AC97_DEFAULT_POWER_OFF then goes to step 6. In this case the
code that calls probe will have done a warm reset to wake the codec in
the first place.
4. Send register reset to codec.
5. wait for codec ready bit.
6. carry on as original probe, by reading register AC97_RESET
I'll implement this if it's acceptable as I can test it on both types of
codec.
Liam
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] unsafe reset in ac97_codec.c
2004-02-05 16:31 ` Liam Girdwood
@ 2004-02-05 17:59 ` Alan Cox
2004-02-26 15:55 ` Liam Girdwood
0 siblings, 1 reply; 6+ messages in thread
From: Alan Cox @ 2004-02-05 17:59 UTC (permalink / raw)
To: Liam Girdwood; +Cc: Jeff Garzik, Linux Kernel Mailing List
On Iau, 2004-02-05 at 16:31, Liam Girdwood wrote:
> I agree, but I think we need to be aware of the codec type before we do
> a register reset. This type of codec is now becoming popular in PDA's.
Sometimes we can't even find out but yes I agree
> I can see another problem with the current probe implementation.
> Currently it sends the register reset command without first checking the
> codec ready bit. This assumes that the AC97 link is up and completely
> working before probe is called.
It is (in theory) the job of the calling driver to ensure AC97 is up
before doing the reset part.
> I'll implement this if it's acceptable as I can test it on both types of
> codec.
Sounds right to me
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] unsafe reset in ac97_codec.c
2004-02-05 17:59 ` Alan Cox
@ 2004-02-26 15:55 ` Liam Girdwood
2004-02-29 18:38 ` Jeff Garzik
0 siblings, 1 reply; 6+ messages in thread
From: Liam Girdwood @ 2004-02-26 15:55 UTC (permalink / raw)
To: Alan Cox; +Cc: Jeff Garzik, Linux Kernel Mailing List
[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]
Hi
I've attached a patch against 2.4.25 that now checks for the codec type
before doing the AC97 register reset.
Changes:-
o Added AC97_DEFAULT_POWER_OFF to ac97_codec_ids[]
o ac97_probe now checks hardwired codec ID's before sending a reset
o Added initial support for WM9713 AC97 codec.
Liam
On Thu, 2004-02-05 at 17:59, Alan Cox wrote:
> On Iau, 2004-02-05 at 16:31, Liam Girdwood wrote:
> > I agree, but I think we need to be aware of the codec type before we do
> > a register reset. This type of codec is now becoming popular in PDA's.
>
> Sometimes we can't even find out but yes I agree
>
> > I can see another problem with the current probe implementation.
> > Currently it sends the register reset command without first checking the
> > codec ready bit. This assumes that the AC97 link is up and completely
> > working before probe is called.
>
> It is (in theory) the job of the calling driver to ensure AC97 is up
> before doing the reset part.
>
> > I'll implement this if it's acceptable as I can test it on both types of
> > codec.
>
> Sounds right to me
>
>
> ________________________________________________________________________
> This email has been scanned for all viruses by the MessageLabs Email
> Security System.
> ________________________________________________________________________
>
[-- Attachment #2: ac_power.diff.gz --]
[-- Type: application/x-gzip, Size: 1772 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-02-29 18:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-02-03 15:42 [BUG] unsafe reset in ac97_codec.c Liam Girdwood
2004-02-04 21:24 ` Jeff Garzik
2004-02-05 16:31 ` Liam Girdwood
2004-02-05 17:59 ` Alan Cox
2004-02-26 15:55 ` Liam Girdwood
2004-02-29 18:38 ` Jeff Garzik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox