[BUG] unsafe reset in ac97_codec.c

[BUG] unsafe reset in ac97_codec.c

[Liam Girdwood]

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

Re: [BUG] unsafe reset in ac97_codec.c

[Jeff Garzik]

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

Re: [BUG] unsafe reset in ac97_codec.c

[Liam Girdwood]

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

Re: [BUG] unsafe reset in ac97_codec.c

[Alan Cox]

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

Re: [BUG] unsafe reset in ac97_codec.c

[Liam Girdwood]

[-- 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 --]

Re: [BUG] unsafe reset in ac97_codec.c

[Jeff Garzik]

Patch looks good to me.

You need to use bit operations, not equality, to check codec->flags, but 
I can fix that up.

	Jeff