Tejun, The patch that I sent does invoke EH, both for resume and suspend. I am calling ata_pci_device_suspend as the runtime suspend method. This will call ata_host_suspend which will end up calling ata_host_request_pm, which sets some flags including ATA_PFLAG_PM_PENDING and basically schedules the error handling (ata_port_schedule_eh). Isn't that what is needed? The runtime resume that I use, does the same - schedules error handling with ATA_EH_RESET. Is there any reason that the system suspend and resume methods can't be used for runtime suspend and resume? I mean sure, we probably don't need the ata_pci_device_do_suspend part since the runtime PM will take care of that, but other than that it looks like we should just be able to do a ata_host_suspend during runtime suspend. Thanks, Priyanka On Mon, Jan 3, 2011 at 9:31 AM, Tejun Heo <tj@xxxxxxxxxx> wrote: > Hello, Alan. > > On Mon, Jan 03, 2011 at 10:15:40AM -0500, Alan Stern wrote: >> > Oh I see. You wanna put the controller into sleep. You'll have to >> > implement EH actions for it and invoke EH to do it. The problem is >> > that for it to work in generic manner, the operations need to be >> > synchronized with other accesses to the hardware and going through EH >> > is the easiest way to achieve that. Something like the following >> > would work. >> >> Are you talking about the usual SCSI error handler or something else >> specific to ATA drivers? > > I'm talking specifically about libata EH. > >> Runtime suspend should not occur unless all the child devices (i.e., >> the attached drives) are already suspended. This means it is not >> necessary to synchronize any operations, since there shouldn't be any >> other accesses to the hardware going on. Therefore there is no need to >> use the error handler. > > Well, it's not that simple I'm afraid. EH actions are asynchronous. > Even if all the downstream devices are suspended, PHY events can > happen any time and EH could be active. Hmmm... a delta but it would > make more sense to put only the controller into hot sleep while > leaving the disk alone for rotating devices. > > Also, on resume, as the controller was out, libata needs to do full > revalidation & reconfiguration. There's no way to avoid EH. > >> > * EH kicks in. Mark the port inactive so that further commands >> > processing won't happen (it would probably be nice if this is >> > something the PM framework can guarantee but I don't think it's done >> > that way, is it?) >> >> Why must the port be marked inactive? If any further commands need to >> be processed then they should cause the controller to be resumed, at >> which point the port will be active again. > > Then, at least we would need to plug EH because commands aren't the > only intiation vector. > >> > * EH skips all other EH actions and put the controller in sleep. >> >> The PCI core already takes care of putting device into D3. The EH >> doesn't need to do it. And there shouldn't be any other EH actions >> pending. > > So, it behaves differently from the usual suspend/resume? We have > .suspend callback which puts the controller into D3. Are you saying > for runtime PM that isn't necessary? If so, wouldn't it be better to > unify behaviors between the two paths? > >> > On runtime resume >> > >> > * Tell EH to kick in and wake up the controller. >> >> The PCI core already puts devices back into D0. > > I still can't see how this would work without low level driver's help. > Who's gonna reconfigure the controller? Or are controllers supposed > to maintain all configurations across D3(hot) sleep? > >> > * EH kicks in. Powers up the controller and revalidates all the >> > attached devices. >> >> At this point the attached devices should still be suspended. >> Presumably you would not want to revalidate them until their own >> runtime resumes occur. > > Controller resume involves SATA bus resets. As there can be multiple > devices per bus, you can't really divide things that neatly. The > hardware isn't designed that way. > >> > So, it's gonna take a bit more code than posted. >> >> On the contrary, there should be almost no new code needed. In fact, >> calling pm_runtime_put_noidle() in the probe routine and >> pm_runtime_get_noresume() in the remove routine should be almost good >> enough. > > Yeah, that would be nice. I'm quite doubtful that would work as well > as you would expect it to tho. > > Thanks. > > -- > tejun > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
