ACPI work on aic7xxx

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

drivers/scsi/aic7xxx/aic7xxx.h +15 -0

@@ -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 */

drivers/scsi/aic7xxx/aic7xxx.h +6 -0

+{
+	int rval;
+	int device_state = dev->current_state;
+	struct ahc_softc *ahc =
+		ahc_find_softc((struct ahc_softc *)pci_get_drvdata(dev));
+

Quoted 12 lines

drivers/scsi/aic7xxx/aic7xxx_osm_pci.c +12 -0

+#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*/;

drivers/scsi/aic7xxx/aic7xxx_osm_pci.c line 260

Quoted 5 lines

drivers/scsi/aic7xxx/aic7xxx_osm_pci.c +5 -0

+                        break;
+        }
+#else
+	device_state = state;
+#endif

Can you kill #if 0 code?

Quoted 1 line

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.

Hi!

Quoted 2 lines

Quoted 1 line

Can you kill #if 0 code?

Quoted 1 line

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.

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.

Quoted 5 lines · Benjamin Herrenschmidt

2 comments here:

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

Quoted 3 lines

- 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

Quoted 9 lines

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 2 lines · Benjamin Herrenschmidt

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

Did more code reading. I assume the value passed to pci_dev->suspend is going to be one of:

enum {
PM_SUSPEND_ON,
PM_SUSPEND_STANDBY,
PM_SUSPEND_MEM,
PM_SUSPEND_DISK,
PM_SUSPEND_MAX,
};

And the value passed to pci_set_power_state will continue to be 0..3 (except enumerated)?

Note regarding aix7xxx, we also need proper hooks in the SCSI stack to block the queue correctly etc... in the same way we do on IDE. I didn't have time to look into this yet.

Quoted 3 lines · Benjamin Herrenschmidt

Note regarding aix7xxx, we also need proper hooks in the SCSI stack to block the queue correctly etc... in the same way we do on IDE. I didn't have time to look into this yet.

Here's what we currently do, aic7xxx_core.c - looks like it attempts to quiesce, and then refuse to suspend if we happen to be busy. This is a little messy because it's done in the suspend call rather than the save_state call, therefore resume will still be called if this routine returns an error code, which will reinitialize the device when we didn't really need to.

int
ahc_suspend(struct ahc_softc *ahc)
{

         ahc_pause_and_flushwork(ahc);

         if (LIST_FIRST(&ahc->pending_scbs) != NULL) {
                 ahc_unpause(ahc);
                 return (EBUSY);
         }

#ifdef AHC_TARGET_MODE
         /*
          * XXX What about ATIOs that have not yet been serviced?
          * Perhaps we should just refuse to be suspended if we
          * are acting in a target role.
          */
         if (ahc->pending_device != NULL) {
                 ahc_unpause(ahc);
                 return (EBUSY);
         }
#endif
         ahc_shutdown(ahc);
         return (0);
}

save_state is a nonsense, didn't we kill it ? queue quiescing must be done by the upper level, which is a bit nasty with things like md & multipath... basically, the low level driver must have a way to notify it's functional parent (as opposed to it's bus parent) that it's going to sleep, and any path using this low level driver must then be quiesced, the parent must resume only when all the drivers it relies on are back up.

Quoted 1 line · Benjamin Herrenschmidt

save_state is a nonsense, didn't we kill it ?

queue quiescing must be

Quoted 7 lines

done by the upper level, which is a bit nasty with things like md & multipath... basically, the low level driver must have a way to notify it's functional parent (as opposed to it's bus parent) that it's going to sleep, and any path using this low level driver must then be quiesced, the parent must resume only when all the drivers it relies on are back up.

Isn't sysfs supposed to take care of this for us? IOW, I shouldn't have to call up to the SCSI midlayer from pcidev->suspend in order to notify it of a suspend, the midlayer should call the driver before we ever try to suspend. This may become important some day when the upper layers need to decide which order to bring pci devices down

Looking in /sys/devices shows that sysfs already knows that 'host0' is a child of a SCSI PCI device.

$ ls 
/sys/devices/pci0000\:00/0000\:00\:1e.0/0000\:02\:02.0/host0/0\:0\:0\:0/
block   detach_state    model  queue_depth  rev         state    type
delete  device_blocked  power  rescan       scsi_level  timeout  vendor

sysfs only takes care about the bus hierarchy as far as suspend/resume is concerned (which is the only sane way to do it imho)

No, the ordering cannot be dictated by the upper layer, but by the physical bus hierarchy. The low level driver gets the suspend callback and need to notify the parent. The md/multipath must keep track that one of the device it relies on is going away and thus block the queues.

That is at least for machine suspend/resume.

Quoted 1 line

Looking in /sys/devices shows that sysfs already knows that 'host0' is a child of a SCSI PCI device.

Yes, but the PM herarchy is the bus hierarchy, I don't see a simple way of going through both in this case ...

Quoted 2 lines · Benjamin Herrenschmidt

sysfs only takes care about the bus hierarchy as far as suspend/resume is concerned (which is the only sane way to do it imho)

I saw comments in one of the PCI IDE driver pcidev->suspend routines that say "we don't need to iterate over the list of drives, sysfs does that for us."

Quoted 5 lines

No, the ordering cannot be dictated by the upper layer, but by the physical bus hierarchy. The low level driver gets the suspend callback and need to notify the parent. The md/multipath must keep track that one of the device it relies on is going away and thus block the queues.

Quoted 1 line

That is at least for machine suspend/resume.

We're talking past each other. I'm saying you take into consideration the physical bus hierarchy: PCI bus x is a parent of SCSI bus y which is a parent of SCSI disk drive z. Suspend disk z, with involvement from the block layer and scsi midlayer, before even calling the actual pcidev->suspend routine on the SCSI bus adapter. Shouldn't require more than minimal LLD involvement.

Quoted 6 lines

Quoted 2 lines

Looking in /sys/devices shows that sysfs already knows that 'host0' is a child of a SCSI PCI device.

Yes, but the PM herarchy is the bus hierarchy, I don't see a simple way of going through both in this case ...

In the case of IDE, IDE is registered as a bus_type and has generic suspend code for the whole bus that is unrelated to the pcidev. The PIIX IDE (Intel chipsets) PCI pcidev struct doesn't even have suspend and resume callbacks filled in, but it works fine!

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

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.

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/

Quoted 3 lines · Benjamin Herrenschmidt

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.

Quoted 6 lines

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.

Quoted 6 lines

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?

Quoted 7 lines · Nathan Bryant

Quoted 3 lines · Benjamin Herrenschmidt

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 !

Quoted 16 lines

Quoted 6 lines

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.

Quoted 6 lines

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/

Quoted 4 lines

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.

Quoted 10 lines

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 }

Quoted 4 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

Quoted 3 lines · Nathan Bryant

Benjamin Herrenschmidt wrote:

Quoted 5 lines

Quoted 3 lines

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 !

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.

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

Quoted 6 lines

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....