On 11/4/23 03:05, Phillip Susi wrote: > Sorry for the delay, I've been out sick all week. > > Damien Le Moal <dlemoal@xxxxxxxxxx> writes: >> Sure, but I think we can skip the spinup only and only if the drive has PUIS >> enabled and reported a complete IDENTIFY: in this case, we can skip the SET >> FEATURES command that spins up the drive to leave the drive spun down. Some more >> code will be needed though so that we track that the drive is spun sdown because >> of PUIS and not because of STANDBY IMMEDIATE, so that ata_dev_power_set_active() >> issues a SET FEATURES to spinup the drive instead of the regular VERIFY command. > > I don't see why the revalidation MUST be performed at system resume. Revalidation is needed to make sure that the device settings did not change, or that the device itself did not change. This must be done before we start using the drive again on a system resume or on a port runtime resume. The current code always revalidates the devices attached to a port on system resume, unless the port was runtime suspended. If the port was runtime suspended, then revalidation will be done on runtime resume. The main issue here which make things messy is the fact that the PM ops are defined for the port, not for the device. Separating things would be cleaner, but I think that is extremely hard to do considering the cases where we have multiple devices per port. For these cases, setting the link speed etc may depend on all the devices responding. So leaving some devices sleeping may be very dangerous to do. > The patch I've been working on leaves the error flags and also sets the > DFLAG_SLEEPING flag on the disk when it's PuiS. Then later when the > disk is accessed, the SLEEPING flag schedules another round of eh, which > resets the link, and revalidates the disk, and this time the SET > FEATURES command is used to wake the drive. I'll post the patch in a > few hours when I get home. I commented on that. Your patch has a deadlock path. > The basic case of postponing the wakeup of the drive until the disk is > later accessed seems to work fine. Unfortunately, that later access > comes quickly either in the form of a CHECK POWER CONDITION from > udisks2.service, or FLUSH CACHE from the filesystem. These commands > don't cause the drive to spin up when it is in ata STANDBY mode, but do > with runtime suspend. I'm leaning towards modifying the blk_queue to > allow the driver to register a method to be called on a request when the > block device is runtime suspended to possibly handle the request without > activating the block device first. udisks2 issues passthrough commands, no ? We do not parse these currently, and we will *not* start doing that. For other operations like sync etc which end up as a FLUSH CACHE command, or any other command that require medium access, triggering EH when we prepare these commands in the submission path is not nice. Not even sure that can work. That may need some significant changes to work. >> Note: I am still not able to see runtime suspend automatically suspend the ata >> port if I enable a scsi disk autosuspend_delay_ms. That does not happen for me. >> In fact, I cannot even read a ata port autosuspend delay: >> >> cat /sys/class/ata_port/ata2/power/autosuspend_delay_ms >> cat: /sys/class/ata_port/ata2/power/autosuspend_delay_ms: Input/output error > > Right, there is no activity timer on the port. > >> But I have: >> >> cat /sys/class/ata_port/ata2/power/control >> auto > > If that is set to auto then it seems to auto suspend once all children > have gone to runtime suspend. For my internal ports, they only have a > single child, so when the drive enters runtime suspend, so does the > port. For the eSATA PMP dock, BOTH drives have to suspend before the > port will. Well, I do not see that happening. "cat /sys/class/ata_port/ataX/power/runtime_status" always says "unsupported" on my system. So not sure how you can get that... The only "link" between a scsi disk device and an ata_port device is created by libata with the RPM_ACTIVE flag which runs pm_runtime_get_sync() on the supplier, that is, the ata_port. So I do not see how runtime suspend could ever work for a port as-is. The fact that you seem to be seeing a different behavior than me kind of indicates that something is still broken with the runtime port management. Not sure what exactly. Could you send me your kernel config please ? I would like to see if using that I get a different behavior. Also, what are your ata adapters (vendor/model) ? -- Damien Le Moal Western Digital Research
