Patch to make ymfpci legacy address 16 bits

Patch to make ymfpci legacy address 16 bits

[Pete Zaitcev]

Hi:

I found that every time I run a 2.4 on my laptop, APM locks up
the machine. Apparently, legacy YMF code enabled decoding of
10 bits of I/O address. A call to APM BIOS touched that and
somehow the system locked up.

If Pavel Roskin, Daisuke Nagano or someone else do not mind,
I want this in stock kernel.

-- Pete

--- linux-2.4.4/drivers/sound/ymfpci.c	Thu Apr 26 22:17:27 2001
+++ linux-2.4.4-niph/drivers/sound/ymfpci.c	Tue May  8 16:46:58 2001
@@ -2059,9 +2059,10 @@
 	}
 
 	if (mpuio >= 0 || oplio >= 0) {
-		v = 0x003e;
+		/* 0x0020: 1 - 10 bits of I/O address decoded, 0 - 16 bits. */
+		v = 0x001e;
 		pci_write_config_word(pcidev, PCIR_LEGCTRL, v);
-	
+
 		switch (pcidev->device) {
 		case PCI_DEVICE_ID_YAMAHA_724:
 		case PCI_DEVICE_ID_YAMAHA_740:

Re: Patch to make ymfpci legacy address 16 bits

[Pavel Roskin]

Hi, Pete!

Next time you are asking my opinion please cc: me, so that I can quote
you.

Yes, I think you have fixed a terrible bug in ymfpci. Decoding only 10-bit
addresses is extremely dangerous, considering that only 388-38b is
reserved, while 788-78b etc are not.

In order to get your patch accepted sooner please use symbolic constants
and better indentation. I don't want to steal your credits by doing it for
you :-)

If you want to play further with APM and ymfpci, I made a stub for proper
apm support in the ymfpci driver. It's available here:

http://www.red-bean.com/~proski/linux/ymfpci_pm.diff

You may need to save some data in memory when the system goes to suspend
and restore them afterwards. I believe that the PCI config space should be
saved by BIOS. Everything else is the responsibility of the driver.

If I find a similar problem in ALSA it will be reported with cc: to you.

-- 
Regards,
Pavel Roskin

Re: Patch to make ymfpci legacy address 16 bits

[Jeff Garzik]

Pavel Roskin wrote:
> If you want to play further with APM and ymfpci, I made a stub for proper
> apm support in the ymfpci driver. It's available here:
> 
> http://www.red-bean.com/~proski/linux/ymfpci_pm.diff

Why not use pci_driver::{suspend,resume} ?


> You may need to save some data in memory when the system goes to suspend
> and restore them afterwards. I believe that the PCI config space should be
> saved by BIOS. Everything else is the responsibility of the driver.

In ACPI land the kernel should save and restore the PCI device config
space and the PCI bus config space.  It is probably that similar is
necessary under APM.

	Jeff


-- 
Jeff Garzik      | Game called on account of naked chick
Building 1024    |
MandrakeSoft     |

Re: Patch to make ymfpci legacy address 16 bits

[Pavel Roskin]

Hi, Jeff!

Thanks for your very (!!!) fast response :-)

> > http://www.red-bean.com/~proski/linux/ymfpci_pm.diff
>
> Why not use pci_driver::{suspend,resume} ?

I'm just a bit conservative. There are several drivers that don't use this
mechanism, notably trident and maestro. Do you think it's safe to switch
all sound drivers to the mechanism you are proposing?

I'm worried about a comment in maestro.c:

                /*
                 * we'd also like to find out about
                 * power level changes because some biosen
                 * do mean things to the maestro when they
                 * change their power state.
                 */

If we switch to pci_driver::{suspend,resume}, will it ever be possible to
add support for any messages other than PM_SUSPEND and PM_RESUME? Probably
yes, but only in the PCI driver dispatches them.

By the way, I don't like the way how pm_unregister_all() is used in
several sound drivers. If a unit is removed its power manager callback
remains registered until the module is unloaded.

> In ACPI land the kernel should save and restore the PCI device config
> space and the PCI bus config space.  It is probably that similar is
> necessary under APM.

I have never seen any sound driver doing that. I also know that PCI
settings are saved by some BIOSes on some hardware.

I'm sorry if I put it wrong. Perhaps I should have added a few IMHO.

PS. The 0x20 bit in 0x40 is not set if I load CVS ALSA drivers, even if I
set it before loading the driver. It's a problem with the kernel driver
only and should be fixed IMHO.

-- 
Regards,
Pavel Roskin

Re: Patch to make ymfpci legacy address 16 bits

[Jeff Garzik]

Pavel Roskin wrote:
> 
> Hi, Jeff!
> 
> Thanks for your very (!!!) fast response :-)
> 
> > > http://www.red-bean.com/~proski/linux/ymfpci_pm.diff
> >
> > Why not use pci_driver::{suspend,resume} ?
> 
> I'm just a bit conservative. There are several drivers that don't use this
> mechanism, notably trident and maestro. Do you think it's safe to switch
> all sound drivers to the mechanism you are proposing?
> 
> I'm worried about a comment in maestro.c:
> 
>                 /*
>                  * we'd also like to find out about
>                  * power level changes because some biosen
>                  * do mean things to the maestro when they
>                  * change their power state.
>                  */
> 
> If we switch to pci_driver::{suspend,resume}, will it ever be possible to
> add support for any messages other than PM_SUSPEND and PM_RESUME? Probably
> yes, but only in the PCI driver dispatches them.

Basically the PCI core should implement what PM is necessary, because
eventually struct pci_driver will become a more generic struct driver. 
When that happens, drivers using pci_driver::{suspend,resume} will
automagically work under the new system.  To answer your question, the
PCI[/driver] core should support messages other than PM_{SUSPEND,RESUME}
if they need to be supported.  Right now I mostly see suspend/resume
implemented and nothing else, so that is a very straightforward
conversion to the new PCI API.

Why does maestro.c not use my suggestion?  Because it doesn't use struct
pci_driver.
Why does trident.c not use my suggestion?  Only because noone has
written and tested the patch for it yet :)  It uses struct pci_driver
and should be updated to use ::suspend/resume.

> > In ACPI land the kernel should save and restore the PCI device config
> > space and the PCI bus config space.  It is probably that similar is
> > necessary under APM.
> 
> I have never seen any sound driver doing that. I also know that PCI
> settings are saved by some BIOSes on some hardware.

That's because full PM support is still very much a work in progress :) 
The linux-pm-devel sourceforge list has carried recent discussions, as
has linux-kernel.  Patrick Mochel(sp?) @ Transmeta has posted a
hackers-only patch which incorporates my PCI PM work as well as his own
ACPI work to do suspend and resume.

-- 
Jeff Garzik      | Game called on account of naked chick
Building 1024    |
MandrakeSoft     |

Re: Patch to make ymfpci legacy address 16 bits

[Pavel Roskin]

Hi, Jeff!

> Basically the PCI core should implement what PM is necessary, because
> eventually struct pci_driver will become a more generic struct driver.

I just wanted to make sure that you don't expect any problems if we go
this way.

> Why does maestro.c not use my suggestion?  Because it doesn't use struct
> pci_driver.

I see. It's not a pure PCI driver. I wonder what happens if some other
driver becomes "impure" e.g. by adding PCMCIA support.

> Why does trident.c not use my suggestion?  Only because noone has
> written and tested the patch for it yet :)  It uses struct pci_driver
> and should be updated to use ::suspend/resume.

First part (writing the patch) is done:

http://www.red-bean.com/~proski/linux/trident_pm.diff

I only know that it compiles. I have no hardware I can test it on. Please
don't apply until tested!

-- 
Regards,
Pavel Roskin

Re: Patch to make ymfpci legacy address 16 bits

[Zach Brown]

On Wed, May 09, 2001 at 05:08:15PM -0400, Jeff Garzik wrote:

> Why does maestro.c not use my suggestion?  Because it doesn't use struct
> pci_driver.

I finally found an able hacker with maestro hardware with power
management.  He not only fixed the nasty pm races that were causing
channel corruption, but he seems willing to move it towards the proper
pci_driver APIs as well.

So don't worry Jeff, we'll fix the evil maestro.c driver yet :) :)

- z

Re: Patch to make ymfpci legacy address 16 bits

[Albert D. Cahalan]

Jeff Garzik writes:
> Pavel Roskin wrote:

>> You may need to save some data in memory when the system goes
>> to suspend and restore them afterwards. I believe that the PCI
>> config space should be saved by BIOS. Everything else is the
>> responsibility of the driver.
>
> In ACPI land the kernel should save and restore the PCI device
> config space and the PCI bus config space.  It is probably that
> similar is necessary under APM.

When you write "the kernel", do you mean the driver or generic
code? I hope you mean the driver, because I have this:

1. the device looks normal at power on
2. the driver pokes a device-specific config register
3. the config space header changes from type 0 to type 1

(The class code does NOT indicate PCI-to-PCI bridge.
You could say this is like CardBus but much weirder)

If the kernel saves type 1 header data, cuts power using
motherboard features, restores power, and then tries to
restore type 1 header data into a type 0 header... the
system will be well and truly screwed IMHO.

Re: Patch to make ymfpci legacy address 16 bits

[mirabilos]

> When you write "the kernel", do you mean the driver or generic
> code? I hope you mean the driver, because I have this:
> 
> 1. the device looks normal at power on
> 2. the driver pokes a device-specific config register
> 3. the config space header changes from type 0 to type 1
> 
> (The class code does NOT indicate PCI-to-PCI bridge.
> You could say this is like CardBus but much weirder)
> 
> If the kernel saves type 1 header data, cuts power using
> motherboard features, restores power, and then tries to
> restore type 1 header data into a type 0 header... the
> system will be well and truly screwed IMHO.

This reminds me of being unable to use my sound card under Windoze
after stand-by, suspend etc. (NB Win2k hibernate worked, suspend
I had disabled then).
Seems as not only linux has this kind of problems...
I don't remember _which_ card it was but I am quite sure it was
an old OPTi MAD16 Pro.

-mirabilos
-- 
EA F0 FF 00 F0 #$@%CARRIER LOST