Re: [alsa-devel] PNP_DRIVER_RES_DISABLE breaks swsusp at least withsnd_cs4236

[Rene Herman]

On 10-01-08 08:58, Jaroslav Kysela wrote:

> On Thu, 10 Jan 2008, Rene Herman wrote:
>
> > On 09-01-08 23:43, Ondrej Zary wrote:
> >
> > Jaroslav -- in your role as ISA-PnP maintainer and Bjorn, in yours as 
> > having been foollish enough to touch PnP recently:
> >
> > > as hibernation (swsusp) started to work with my CPU, I found that my Turtle 
> > > Beach Malibu stops working after resume from hibernation. It's caused by fact 
> > > that the card is not enabled on the pnp layer during resume - and thus card 
> > > registers are inaccessible (reads return FFs, writes go nowhere).
> > >
> > > During resume, pnp_bus_resume() in drivers/pnp/driver.c is called for each pnp 
> > > device. This function calls pnp_start_dev() only when the 
> > > PNP_DRIVER_RES_DO_NOT_CHANGE bit is NOT seting pnp_drv->flags. But the cs4236 
> > > driver in sound/isa/cs423x/cs4236.c explicitly sets the .flags to 
> > > PNP_DRIVER_RES_DISABLE - it's value is 3 and that includes 
> > > PNP_DRIVER_RES_DO_NOT_CHANGE bit.
> > Ehm. Isn't that a bit unexpected:
> >
> > #define PNP_DRIVER_RES_DO_NOT_CHANGE    0x0001  /* do not change the state 
> > of the device */
> > #define PNP_DRIVER_RES_DISABLE          0x0003  /* ensure the device is 
> > disabled */
> >
> > I'd say that disabling is changing, so isn't this just a braino where 
> > someone meant to write 2 instead of 3?
>
> It's irrelevant. I think that condition in driver.c in suspend and resume 
> callbacks is invalid, because PNP_DRIVER_RES_DO_NOT_CHANGE means that 
> resources should not be changed in the pnp core but only in the driver, 
> but in suspend/resume process are resources preserved, so the condition 
> should be removed.

I see a PnP resume _consists_ of setting the resources so I can see the test 
implementation wise, but yes, conceptually it seems this test shouldn't be 
present. So just like the attached then?

Pierre, what was the idea here? Added the other SOBs to the CC as well...

> Author of this code is:
>
> author Pierre Ossman <drzeus-list@xxxxxxxxx> Tue, 29 Nov 2005 09:09:32 +0100
> committer Jaroslav Kysela <perex@xxxxxxx> Tue, 03 Jan 2006 12:31:30 +0100
>
>     [ALSA] [PATCH] alsa: Improved PnP suspend support
>
>     Also use the PnP functions to start/stop the devices during the suspend so
>     that drivers will not have to duplicate this code.
>
>     Cc: Adam Belay <ambx1@xxxxxxxxxx>
>     Cc: Jaroslav Kysela <perex@xxxxxxx>
>     Cc: Takashi Iwai <tiwai@xxxxxxx>
>
>     Signed-off-by: Pierre Ossman <drzeus@xxxxxxxxx>
>     Signed-off-by: Andrew Morton <akpm@xxxxxxxx>
>     Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>

Rene.

diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index a262762..12a1645 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -161,8 +161,7 @@ static int pnp_bus_suspend(struct device *dev, pm_message_t state)
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
+	if (pnp_can_disable(pnp_dev)) {
 		error = pnp_stop_dev(pnp_dev);
 		if (error)
 			return error;
@@ -185,14 +184,17 @@ static int pnp_bus_resume(struct device *dev)
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
+	if (pnp_can_write(pnp_dev)) {
 		error = pnp_start_dev(pnp_dev);
 		if (error)
 			return error;
 	}
 
-	if (pnp_drv->resume)
-		return pnp_drv->resume(pnp_dev);
+	if (pnp_drv->resume) {
+		error = pnp_drv->resume(pnp_dev);
+		if (error)
+			return error;
+	}
 
 	return 0;
 }