[patch] ACPI work on aic7xxx

[patch] ACPI work on aic7xxx

[Nathan Bryant]

[-- Attachment #1: Type: text/plain, Size: 2003 bytes --]

Attached is a patch against 2.6.8-rc that supplies some more of the 
missing pieces of ACPI support for the aic7xxx driver.

--- Background and current driver status:
The 6.2.36 driver in current mainline 2.6 kernels contains the OS- and 
bus- neutral portions of the suspend/resume callbacks (ahc_resume() in 
aic7xxx_core.c and ahc_pci_resume() in aic7xxx_pci.c.) These stubs are 
mostly concerned with the card's own registers, and don't perform the 
requisite linux-specific calls to save and restore PCI config space and 
re-enable the card slot. It also appears that these callbacks were being 
developed by eyeball and were never tested. (Of course, not many people 
could test it since ACPI core wasn't in a usable state until recent 2.6)

As a consequence, after a resume the card I/O space is not visible under 
the current driver, and the driver would panic the kernel with the "Loop 
1" message. (See 
http://marc.theaimsgroup.com/?l=linux-scsi&m=102710764330862&w=2) This 
patch fixes that part. We now renable the slot and interrupts properly, 
and then call the previously-implemented resume routines (with just some 
obvious fixes) to attempt to reinitialize the card.

--- Where we are now:
It still doesn't work. The driver complains, "scsi0: Someone reset 
channel A" repeatedly. This message is coming from the driver's 
interrupt handler, so either the card registers are improperly 
reinitialized in some way, or the driver's state engine is not clearing 
the interrupt status, or we're somehow causing the reset to occur again 
and again. I'm not sure how much further I can take this without a data 
book or some help figuring out the card and driver state machines. I 
guess either the card's state machine or the driver's state machine has 
been driven insane, but I'm not sure what direction to look in.

This patch also includes a small fix for a bad merge, as suggested by 
Pavel Machec. See 
http://marc.theaimsgroup.com/?l=linux-scsi&m=108306129820558&w=2


Nathan Bryant

[-- Attachment #2: aic7xxx_acpi.patch --]
[-- Type: text/plain, Size: 6030 bytes --]

diff -urN linux-2.6.7-1.492.backup/drivers/scsi/aic7xxx/aic7xxx.h linux-2.6.7-1.492/drivers/scsi/aic7xxx/aic7xxx.h
--- linux-2.6.7-1.492.backup/drivers/scsi/aic7xxx/aic7xxx.h	2004-06-16 01:19:29.000000000 -0400
+++ linux-2.6.7-1.492/drivers/scsi/aic7xxx/aic7xxx.h	2004-07-19 23:27:51.000000000 -0400
@@ -1109,6 +1109,8 @@
 
 	uint16_t	 	  user_discenable;/* Disconnection allowed  */
 	uint16_t		  user_tagenable;/* Tagged Queuing allowed */
+
+	u32                      PciState[64]; /* save PCI state to this area */
 };
 
 TAILQ_HEAD(ahc_softc_tailq, ahc_softc);
diff -urN linux-2.6.7-1.492.backup/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c linux-2.6.7-1.492/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
--- linux-2.6.7-1.492.backup/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c	2004-07-15 14:23:17.000000000 -0400
+++ linux-2.6.7-1.492/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c	2004-07-20 00:34:30.000000000 -0400
@@ -54,6 +54,10 @@
 static int	ahc_linux_pci_reserve_mem_region(struct ahc_softc *ahc,
 						 u_long *bus_addr,
 						 uint8_t **maddr);
+#ifdef CONFIG_PM
+static int	ahc_linux_pci_suspend(struct pci_dev *dev, u32 state);
+static int	ahc_linux_pci_resume(struct pci_dev *dev);
+#endif
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,0)
 static void	ahc_linux_pci_dev_remove(struct pci_dev *pdev);
 
@@ -76,6 +80,10 @@
 	.name		= "aic7xxx",
 	.probe		= ahc_linux_pci_dev_probe,
 	.remove		= ahc_linux_pci_dev_remove,
+#ifdef CONFIG_PM
+	.suspend	= ahc_linux_pci_suspend,
+	.resume		= ahc_linux_pci_resume,
+#endif
 	.id_table	= ahc_linux_pci_id_table
 };
 
@@ -225,6 +233,72 @@
 #endif
 }
 
+#ifdef CONFIG_PM
+int ahc_linux_pci_suspend(struct pci_dev *dev, u32 state)
+{
+	int rval;
+	u32 device_state;
+	struct ahc_softc *ahc =
+		ahc_find_softc((struct ahc_softc *)pci_get_drvdata(dev));
+
+#if 0
+        switch(state)
+        {
+                case 1: /* S1 */
+                        device_state=1; /* D1 */;
+                        break;
+                case 3: /* S3 */
+                case 4: /* S4 */
+                        device_state=3; /* D3 */;
+                        break;
+                default:
+                        return -EAGAIN /*FIXME*/;
+                        break;
+        }
+#else
+	device_state = state;
+#endif
+
+        printk(KERN_INFO
+        "aic7xxx: pci-suspend: pdev=0x%p, slot=%s, Entering operating state [D%d]\n",
+                dev, pci_name(dev), device_state);
+
+	rval = ahc->bus_suspend(ahc);
+	if (rval != 0)
+		return rval;
+
+	pci_save_state(dev, ahc->PciState);
+	pci_disable_device(dev);
+	pci_set_power_state(dev, device_state);
+	return 0;
+}
+
+int ahc_linux_pci_resume(struct pci_dev *dev)
+{
+	int rval;
+	int device_state = dev->current_state;
+	struct ahc_softc *ahc =
+		ahc_find_softc((struct ahc_softc *)pci_get_drvdata(dev));
+
+        printk(KERN_INFO
+        "aic7xxx: pci-resume: pdev=0x%p, slot=%s, Previous operating state [D%d]\n",
+                dev, pci_name(dev), device_state);
+
+        pci_set_power_state(dev, AHC_POWER_STATE_D0);
+        pci_restore_state(dev, ahc->PciState);
+        pci_enable_device(dev);
+        pci_set_master(dev);
+
+	rval = ahc->bus_resume(ahc);
+#ifdef AHC_DEBUG
+	if (ahc_debug & AHC_SHOW_MISC) {
+		ahc_dump_card_state(ahc);
+	}
+#endif
+	return rval;
+}
+#endif /* CONFIG_PM */
+
 void
 ahc_linux_pci_exit(void)
 {
diff -urN linux-2.6.7-1.492.backup/drivers/scsi/aic7xxx/aic7xxx_pci.c linux-2.6.7-1.492/drivers/scsi/aic7xxx/aic7xxx_pci.c
--- linux-2.6.7-1.492.backup/drivers/scsi/aic7xxx/aic7xxx_pci.c	2004-06-16 01:18:57.000000000 -0400
+++ linux-2.6.7-1.492/drivers/scsi/aic7xxx/aic7xxx_pci.c	2004-07-19 23:32:21.000000000 -0400
@@ -834,8 +834,8 @@
 	ahc_pci_write_config(ahc->dev_softc, DEVCONFIG, devconfig, /*bytes*/4);
 
 	/* Ensure busmastering is enabled */
-	command = ahc_pci_read_config(ahc->dev_softc, PCIR_COMMAND, /*bytes*/2);
-	command |= PCIM_CMD_BUSMASTEREN;
+        command = ahc_pci_read_config(ahc->dev_softc, PCIR_COMMAND, /*bytes*/2);
+        command |= PCIM_CMD_BUSMASTEREN;
 
 	ahc_pci_write_config(ahc->dev_softc, PCIR_COMMAND, command, /*bytes*/2);
 
@@ -2090,21 +2090,18 @@
 static int
 ahc_pci_resume(struct ahc_softc *ahc)
 {
-
-	ahc_power_state_change(ahc, AHC_POWER_STATE_D0);
-
 	/*
 	 * We assume that the OS has restored our register
 	 * mappings, etc.  Just update the config space registers
 	 * that the OS doesn't know about and rely on our chip
 	 * reset handler to handle the rest.
 	 */
-	ahc_pci_write_config(ahc->dev_softc, DEVCONFIG, /*bytes*/4,
-			     ahc->bus_softc.pci_softc.devconfig);
-	ahc_pci_write_config(ahc->dev_softc, PCIR_COMMAND, /*bytes*/1,
-			     ahc->bus_softc.pci_softc.command);
-	ahc_pci_write_config(ahc->dev_softc, CSIZE_LATTIME, /*bytes*/1,
-			     ahc->bus_softc.pci_softc.csize_lattime);
+	ahc_pci_write_config(ahc->dev_softc, DEVCONFIG,
+			     ahc->bus_softc.pci_softc.devconfig, /*bytes*/4);
+	ahc_pci_write_config(ahc->dev_softc, PCIR_COMMAND,
+			     ahc->bus_softc.pci_softc.command, /*bytes*/1);
+	ahc_pci_write_config(ahc->dev_softc, CSIZE_LATTIME,
+			     ahc->bus_softc.pci_softc.csize_lattime, /*bytes*/1);
 	if ((ahc->flags & AHC_HAS_TERM_LOGIC) != 0) {
 		struct	seeprom_descriptor sd;
 		u_int	sxfrctl1;
--- linux-2.6.7-1.486/drivers/scsi/aic7xxx/aic7xxx_osm.c.orig	2004-07-14 18:45:39.695469069 -0400
+++ linux-2.6.7-1.486/drivers/scsi/aic7xxx/aic7xxx_osm.c	2004-07-14 18:46:55.160968771 -0400
@@ -2295,7 +2295,7 @@
 	sprintf(current->comm, "ahc_dv_%d", ahc->unit);
 #else
 	daemonize("ahc_dv_%d", ahc->unit);
-	current->flags |= PF_FREEZE;
+	current->flags |= PF_NOFREEZE;
 #endif
 	unlock_kernel();
 
--- linux-2.6.7-1.486/drivers/scsi/aic7xxx/aic79xx_osm.c.orig	2004-07-14 18:46:09.524239111 -0400
+++ linux-2.6.7-1.486/drivers/scsi/aic7xxx/aic79xx_osm.c	2004-07-14 18:47:09.707290430 -0400
@@ -2591,7 +2591,7 @@
 	sprintf(current->comm, "ahd_dv_%d", ahd->unit);
 #else
 	daemonize("ahd_dv_%d", ahd->unit);
-	current->flags |= PF_FREEZE;
+	current->flags |= PF_NOFREEZE;
 #endif
 	unlock_kernel();
 

Re: [patch] ACPI work on aic7xxx

[Pavel Machek]

Hi!

> It still doesn't work. The driver complains, "scsi0: Someone reset 
> channel A" repeatedly. This message is coming from the driver's 
> interrupt handler, so either the card registers are improperly 
> reinitialized in some way, or the driver's state engine is not clearing 
> the interrupt status, or we're somehow causing the reset to occur again 
> and again. I'm not sure how much further I can take this without a data 
> book or some help figuring out the card and driver state machines. I 
> guess either the card's state machine or the driver's state machine has 
> been driven insane, but I'm not sure what direction to look in.
> 
> This patch also includes a small fix for a bad merge, as suggested by 
> Pavel Machec. See 
> http://marc.theaimsgroup.com/?l=linux-scsi&m=108306129820558&w=2

It looks better tha what was there (modulo whitespace)

								Pavel

> diff -urN linux-2.6.7-1.492.backup/drivers/scsi/aic7xxx/aic7xxx.h linux-2.6.7-1.492/drivers/scsi/aic7xxx/aic7xxx.h
> --- linux-2.6.7-1.492.backup/drivers/scsi/aic7xxx/aic7xxx.h	2004-06-16 01:19:29.000000000 -0400
> +++ linux-2.6.7-1.492/drivers/scsi/aic7xxx/aic7xxx.h	2004-07-19 23:27:51.000000000 -0400
> @@ -1109,6 +1109,8 @@
>  
>  	uint16_t	 	  user_discenable;/* Disconnection allowed  */
>  	uint16_t		  user_tagenable;/* Tagged Queuing allowed */
> +
> +	u32                      PciState[64]; /* save PCI state to this area */
>  };
>  
>  TAILQ_HEAD(ahc_softc_tailq, ahc_softc);

Space-vs-tabs are inconsistent here.

> @@ -225,6 +233,72 @@
>  #endif
>  }
>  
> +#ifdef CONFIG_PM
> +int ahc_linux_pci_suspend(struct pci_dev *dev, u32 state)
> +{
> +	int rval;
> +	u32 device_state;
> +	struct ahc_softc *ahc =
> +		ahc_find_softc((struct ahc_softc *)pci_get_drvdata(dev));
> +
> +#if 0
> +        switch(state)
> +        {
> +                case 1: /* S1 */
> +                        device_state=1; /* D1 */;
> +                        break;
> +                case 3: /* S3 */
> +                case 4: /* S4 */
> +                        device_state=3; /* D3 */;
> +                        break;
> +                default:
> +                        return -EAGAIN /*FIXME*/;
> +                        break;
> +        }
> +#else
> +	device_state = state;
> +#endif

Can you kill #if 0 code?

> +{
> +	int rval;
> +	int device_state = dev->current_state;
> +	struct ahc_softc *ahc =
> +		ahc_find_softc((struct ahc_softc *)pci_get_drvdata(dev));
> +
> +        printk(KERN_INFO
> +        "aic7xxx: pci-resume: pdev=0x%p, slot=%s, Previous operating state [D%d]\n",
> +                dev, pci_name(dev), device_state);
> +
> +        pci_set_power_state(dev, AHC_POWER_STATE_D0);
> +        pci_restore_state(dev, ahc->PciState);
> +        pci_enable_device(dev);
> +        pci_set_master(dev);
> +
> +	rval = ahc->bus_resume(ahc);

Heavy whitespace damage here, and in following hunks.

								Pavel
-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: [patch] ACPI work on aic7xxx

[Nathan Bryant]

Pavel Machek wrote:

>>+#if 0
>>+        switch(state)
>>+        {
>>+                case 1: /* S1 */
>>+                        device_state=1; /* D1 */;
>>+                        break;
>>+                case 3: /* S3 */
>>+                case 4: /* S4 */
>>+                        device_state=3; /* D3 */;
>>+                        break;
>>+                default:
>>+                        return -EAGAIN /*FIXME*/;
>>+                        break;
>>+        }
>>+#else
>>+	device_state = state;
>>+#endif
> 
> 
> Can you kill #if 0 code?

Yes. This is a work in progress. Interestingly, the ifdef'd-out code was 
pasted from mptbase.c in the MPT Fusion driver. If it's broken here, 
it's probably broken there -- seems the state parameter passed to the 
pci resume callback is intended to be a PCI D state, not an ACPI S 
state. Can somebody confirm or deny? The kernel is actually passing 
state 2 (D2) to the driver when I enter ACPI S3, so presumably the same 
failure could happen to fusion.

device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Pavel Machek]

Hi!

> >Can you kill #if 0 code?
> 
> Yes. This is a work in progress. Interestingly, the ifdef'd-out code was 
> pasted from mptbase.c in the MPT Fusion driver. If it's broken here, 
> it's probably broken there -- seems the state parameter passed to the 
> pci resume callback is intended to be a PCI D state, not an ACPI S 
> state. Can somebody confirm or deny? The kernel is actually passing 
> state 2 (D2) to the driver when I enter ACPI S3, so presumably the same 
> failure could happen to fusion.

I'm no longer sure what should be passed there... We'll probably need
to turn it into enum... Actually swsusp code in -mm actually passes
value from enum, and mainline swsusp code passes 0/3. 

								Pavel
-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Nathan Bryant]

Pavel Machek wrote:
> Hi!
> 
> 
>>>Can you kill #if 0 code?
>>
>>Yes. This is a work in progress. Interestingly, the ifdef'd-out code was 
>>pasted from mptbase.c in the MPT Fusion driver. If it's broken here, 
>>it's probably broken there -- seems the state parameter passed to the 
>>pci resume callback is intended to be a PCI D state, not an ACPI S 
>>state. Can somebody confirm or deny? The kernel is actually passing 
>>state 2 (D2) to the driver when I enter ACPI S3, so presumably the same 
>>failure could happen to fusion.
> 
> 
> I'm no longer sure what should be passed there... We'll probably need
> to turn it into enum... Actually swsusp code in -mm actually passes
> value from enum, and mainline swsusp code passes 0/3. 
> 
> 								Pavel

Seems to me, aside from whether it's an enum or not, it should represent 
a D-state not an ACPI S-state. Some platforms (Power Mac?) probably 
implement PCI power management but not in an ACPI way.

Re: device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Benjamin Herrenschmidt]

> Seems to me, aside from whether it's an enum or not, it should represent 
> a D-state not an ACPI S-state. Some platforms (Power Mac?) probably 
> implement PCI power management but not in an ACPI way.

2 comments here:

 - The low level bus state (PCI D state for example) and the "linux"
state should be 2 different entities.

 - For PCI, we probably want a hook so the arch can implement it's own
version of pci_set_power_state() so that ACPI can use it's own trickery
there.

Ben.

Re: device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Nathan Bryant]

Benjamin Herrenschmidt wrote:

> 2 comments here:
> 
>  - The low level bus state (PCI D state for example) and the "linux"
> state should be 2 different entities.
> 
>  - For PCI, we probably want a hook so the arch can implement it's own
> version of pci_set_power_state() so that ACPI can use it's own trickery
> there.

Ok, so the takeaway message for driver writers is to treat the 
pci_dev->suspend() state parameter as an opaque value as far as 
possible, and just pass it on to the other layers

Re: device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Benjamin Herrenschmidt]

On Tue, 2004-07-20 at 14:34, Nathan Bryant wrote:
> Benjamin Herrenschmidt wrote:
> 
> > 2 comments here:
> > 
> >  - The low level bus state (PCI D state for example) and the "linux"
> > state should be 2 different entities.
> > 
> >  - For PCI, we probably want a hook so the arch can implement it's own
> > version of pci_set_power_state() so that ACPI can use it's own trickery
> > there.
> 
> Ok, so the takeaway message for driver writers is to treat the 
> pci_dev->suspend() state parameter as an opaque value as far as 
> possible, and just pass it on to the other layers

NO ! The exact opposite in fact. I'll work on cleaning that up and
write some doco this week with Pavel.

Ben.

Re: device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Pavel Machek]

Hi!

> > > 2 comments here:
> > > 
> > >  - The low level bus state (PCI D state for example) and the "linux"
> > > state should be 2 different entities.
> > > 
> > >  - For PCI, we probably want a hook so the arch can implement it's own
> > > version of pci_set_power_state() so that ACPI can use it's own trickery
> > > there.
> > 
> > Ok, so the takeaway message for driver writers is to treat the 
> > pci_dev->suspend() state parameter as an opaque value as far as 
> > possible, and just pass it on to the other layers
> 
> NO ! The exact opposite in fact. I'll work on cleaning that up and
> write some doco this week with Pavel.

Agreed. I do not have strong opinion about Sx or Dx, but it should
be enum or equivalent so it is hard to get wrong. [And not "really
easy to get wrong because noone knows for sure", as it is now].

								Pavel
-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Nathan Bryant]

Benjamin Herrenschmidt wrote:

> That's different, because the disks are actually registered as
> "struct device" childs of the bus, and thus get proper suspend/resume
> callbacks.

Ok, Seems like we rely on the BIOS a lot, here.

> Oh sure, the disks are in the loop, the problem happens with multipath
> and such which "breaks" the bus hiearchy somewhat. The queue management
> is part of the "functional" hierarchy (read: block layer) on top of
> SCSI disks, thus the disks will be the one getting the suspend callback,
> but they have to "notify" their functional parent (block layer, md, ...)
> to properly get the queues stopped.

All right, I can see where that's probably the easiest solution.

> IDE sort-of does that internally, by generating a special request that
> goes down the queue (in order to be properly ordered with whatever
> is pending in the queue, including pending tagged commands if any),
> and the "toplevel" IDE handling will stop processing the queue once
> that request got past, but it's a hackery that at this point is quite
> specific to drivers/ide/

The queued special command trick does mostly the same thing as the 
scsi_device_quiesce(), right? As far as I can tell, the only difference 
is that we attempt to drain the queue (and sometimes leave error 
handlers running) and they just pause it at some mostly sane stopping point.

Now correct me if I'm wrong, but it seems that with what we understand 
now, we are in a position that we could decide to make SCSI layer do the 
following on suspend, in this order:

scsi_device_quiesce();
{ LLD pause and flush work; aic7xxx will run the completion queue at 
this point }
blk_stop_queue();
{ LLD reset registers and shutdown, etc }

And once the MD problem is solved we will also need to do another call 
before blk_stop_queue().

Am I missing anything?

Re: device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Benjamin Herrenschmidt]

On Sat, 2004-07-24 at 20:19, Nathan Bryant wrote:
> Benjamin Herrenschmidt wrote:
> 
> > That's different, because the disks are actually registered as
> > "struct device" childs of the bus, and thus get proper suspend/resume
> > callbacks.
> 
> Ok, Seems like we rely on the BIOS a lot, here.

How so ? not at all !

> > Oh sure, the disks are in the loop, the problem happens with multipath
> > and such which "breaks" the bus hiearchy somewhat. The queue management
> > is part of the "functional" hierarchy (read: block layer) on top of
> > SCSI disks, thus the disks will be the one getting the suspend callback,
> > but they have to "notify" their functional parent (block layer, md, ...)
> > to properly get the queues stopped.
> 
> All right, I can see where that's probably the easiest solution.
> 
> > IDE sort-of does that internally, by generating a special request that
> > goes down the queue (in order to be properly ordered with whatever
> > is pending in the queue, including pending tagged commands if any),
> > and the "toplevel" IDE handling will stop processing the queue once
> > that request got past, but it's a hackery that at this point is quite
> > specific to drivers/ide/
> 
> The queued special command trick does mostly the same thing as the 
> scsi_device_quiesce(), right? As far as I can tell, the only difference 
> is that we attempt to drain the queue (and sometimes leave error 
> handlers running) and they just pause it at some mostly sane stopping point.

I don't know, I have to look more closely at the SCSI code.

> Now correct me if I'm wrong, but it seems that with what we understand 
> now, we are in a position that we could decide to make SCSI layer do the 
> following on suspend, in this order:
> 
> scsi_device_quiesce();
> { LLD pause and flush work; aic7xxx will run the completion queue at 
> this point }
> blk_stop_queue();
> { LLD reset registers and shutdown, etc }
> 
> And once the MD problem is solved we will also need to do another call 
> before blk_stop_queue().
> 
> Am I missing anything?

We need to issue the stuff from the low level driver (like aix7xxx) or
the disk, that is sd, but we should make sure sg etc... also properly
call the stuff, actually, look at IDE, I defined the special power
request to act as a state machine once down the queue so the ide layer
acts differently for disks, cdroms, etc... by sending appropriate
commands like standby for disks.

Ben.

Re: device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Andre Hedrick]

Ben,

Remember to prevent suspend "BRAIN DAMAGE", scsi has host-hba
dma-queue-rings to manage.  I have thought about this for a while and have
considered a glass of scotch or two ease the pain.

SCSI != IDE

IDE is a simple child by comparison.

SCSI is painful because the hardware specifically prevents raw low-level
access to the bus-phase.

Cheers,

Andre Hedrick
LAD Storage Consulting Group

On Sun, 25 Jul 2004, Benjamin Herrenschmidt wrote:

> On Sat, 2004-07-24 at 20:19, Nathan Bryant wrote:
> > Benjamin Herrenschmidt wrote:
> > 
> > > That's different, because the disks are actually registered as
> > > "struct device" childs of the bus, and thus get proper suspend/resume
> > > callbacks.
> > 
> > Ok, Seems like we rely on the BIOS a lot, here.
> 
> How so ? not at all !
> 
> > > Oh sure, the disks are in the loop, the problem happens with multipath
> > > and such which "breaks" the bus hiearchy somewhat. The queue management
> > > is part of the "functional" hierarchy (read: block layer) on top of
> > > SCSI disks, thus the disks will be the one getting the suspend callback,
> > > but they have to "notify" their functional parent (block layer, md, ...)
> > > to properly get the queues stopped.
> > 
> > All right, I can see where that's probably the easiest solution.
> > 
> > > IDE sort-of does that internally, by generating a special request that
> > > goes down the queue (in order to be properly ordered with whatever
> > > is pending in the queue, including pending tagged commands if any),
> > > and the "toplevel" IDE handling will stop processing the queue once
> > > that request got past, but it's a hackery that at this point is quite
> > > specific to drivers/ide/
> > 
> > The queued special command trick does mostly the same thing as the 
> > scsi_device_quiesce(), right? As far as I can tell, the only difference 
> > is that we attempt to drain the queue (and sometimes leave error 
> > handlers running) and they just pause it at some mostly sane stopping point.
> 
> I don't know, I have to look more closely at the SCSI code.
> 
> > Now correct me if I'm wrong, but it seems that with what we understand 
> > now, we are in a position that we could decide to make SCSI layer do the 
> > following on suspend, in this order:
> > 
> > scsi_device_quiesce();
> > { LLD pause and flush work; aic7xxx will run the completion queue at 
> > this point }
> > blk_stop_queue();
> > { LLD reset registers and shutdown, etc }
> > 
> > And once the MD problem is solved we will also need to do another call 
> > before blk_stop_queue().
> > 
> > Am I missing anything?
> 
> We need to issue the stuff from the low level driver (like aix7xxx) or
> the disk, that is sd, but we should make sure sg etc... also properly
> call the stuff, actually, look at IDE, I defined the special power
> request to act as a state machine once down the queue so the ide layer
> acts differently for disks, cdroms, etc... by sending appropriate
> commands like standby for disks.
> 
> Ben.
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Nathan Bryant]

Benjamin Herrenschmidt wrote:
> On Sat, 2004-07-24 at 20:19, Nathan Bryant wrote:
> 
>>Benjamin Herrenschmidt wrote:
>>
>>
>>>That's different, because the disks are actually registered as
>>>"struct device" childs of the bus, and thus get proper suspend/resume
>>>callbacks.
>>
>>Ok, Seems like we rely on the BIOS a lot, here.
> 
> 
> How so ? not at all !

For suspend/resume and also initialization on bootup. We're not saving 
the chip state for PIIX so I assume we're hoping that ACPI does it for us

> We need to issue the stuff from the low level driver (like aix7xxx) or
> the disk, that is sd, but we should make sure sg etc... also properly
> call the stuff, actually, look at IDE, I defined the special power
> request to act as a state machine once down the queue so the ide layer
> acts differently for disks, cdroms, etc... by sending appropriate
> commands like standby for disks.

There's another one - synchronize cache or disable write back cache on 
the drive....

Re: device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Benjamin Herrenschmidt]

> For suspend/resume and also initialization on bootup. We're not saving 
> the chip state for PIIX so I assume we're hoping that ACPI does it for us

ah, that part, yes, well, we hope ;) though it may just come back up
in the right state for normal PIO access, and we do restore the DMA
state by calling dma_check again in the ide-disk wakeup code.

> > We need to issue the stuff from the low level driver (like aix7xxx) or
> > the disk, that is sd, but we should make sure sg etc... also properly
> > call the stuff, actually, look at IDE, I defined the special power
> > request to act as a state machine once down the queue so the ide layer
> > acts differently for disks, cdroms, etc... by sending appropriate
> > commands like standby for disks.
> 
> There's another one - synchronize cache or disable write back cache on 
> the drive....

Yes, whatever. STANDBYNOW1 is enough on IDE (it does sync. the cache),
though I don't think we "restore" the state of the write back cache,
so that could be a good idea to add too :)

Ben
 

Re: device_suspend() levels [was Re: [patch] ACPI work on aic7xxx]

[Benjamin Herrenschmidt]

On Mon, 2004-07-26 at 03:32, Andre Hedrick wrote:
> Ben,
> 
> Remember to prevent suspend "BRAIN DAMAGE", scsi has host-hba
> dma-queue-rings to manage.  I have thought about this for a while and have
> considered a glass of scotch or two ease the pain.
> 
> SCSI != IDE
> 
> IDE is a simple child by comparison.
> 
> SCSI is painful because the hardware specifically prevents raw low-level
> access to the bus-phase.

Hi Andre !

That shouldn't be a problem in our case though, but the scsi layer more
complicated command queuing mecanism is...

Ben.