Damien Le Moal <dlemoal@xxxxxxxxxx> writes: > Correct, if it succeeds, no need to do it again. The problem with clearing the > flag though is that ATA_EH_SET_ACTIVE is for the device and that is set only if > ATA_PFLAG_RESUMING is set, but that one is for the port. So if resume for the > device succeeds, you can clear the ATA_PFLAG_RESUMING flag *only* if there is > only a single link/device for that port. If not, other devices on the port may > need a retry so we cannot clear ATA_PFLAG_RESUMING. Rather than clear ATA_PFLAG_RESUMING, I was thinking of keeping my previus change to specify ATA_EH_SET_ACTIVE in the request_pm path rather than by setting it based on ATA_PFLAG_RESUMING, but just adding a check to see if the VERIFY fails, and if so, set ATA_EH_SET_ACTIVE again. > Ah, now I think I understand: is it your patch that prevents resuming a drive if > it has PUIS on ? If yes, then sure, the verify command to spin-up the drive will > indeed fail immediately if the drive is in standby from PUIS, since getting out > of standby state driven by PUIS requires a set features spinup. So... with your > patch, things will not work. Right... unless you also apply this patch to make sure that ATA_EH_SET_ACTIVE isn't turned on again after it is cleared when PuiS is detected. > If PUIS is not enabled, the only thing wasted is a check power mode command. If > the drive confirms that it is active, verify command is not issued. Ahh, right... I forgot you did add a CHECK POWER MODE first. > With PUIS enabled though, if you leave the drive in standby mode, when/how do > you actually wake it up ? Initially I just set the ATA_DFLAG_SLEEPING flag as if the drive had been put into SLEEP mode, which triggers EH to wake it up when the drive is accessed. I have since switched to actually putting the drive to SLEEP mode when PuiS is detected since it will save a little more power than leaving it in STANDBY. > Scratching my head about this, I think that doing this cleanly should be > possible if: > 1) The dive gives complete identify data when that command is issued with the > drive in standby state driven by PUIS. I think you can do it even if the IDENTIFY data is incomplete, as long as a revalidate_and_attach is done eventually, when waking the drive up. > 2) The call to ata_dev_configure() executed by ata EH started from system resume > does not spinup the device if requested not to (libata module & sysfs parameter > can do that). But I think this requires that the drive be instead put into a > state equivalent to *runtime* suspend, that is, with the scsi disk associated > with the device must also be in runtime suspend state. Yea, I was trying to make it work with runtime suspend before, but you indicated that the current device hierarchy seems to make that impossible, so I put that aside for now. Currently runtime pm thinks the drive is running even though it isn't, but that isn't any different than when you hdparm -y or hdparm -Y, or hdparm -S and let the drive decide to auto standby. Eventually I'll try to get the runtime pm sorted but I figured I'd try to get the basic concept working first. > With that, it would only be a matter of adding a device flag to remember that > the drive is in "PUIS stnadby" instead of regular standby, and then have > ata_dev_power_set_active() use a set feature spinup command instead of a verify. > Drive spinup will then be cleanly driven by accesses to the scsi disk, similarly > to a regular runtime suspend. Right now I'm using the SLEEP flag to do this, and when the disk is accessed, it triggers a round of EH that does the revalidate_and_attach and in the process, issues the SET FEATURES command to wake the drive. > With such changes, everything would be cleaner and safer and all work as > expected. The exception will be drives that do not give complete identify data > when PUIS is on. For these, it is too risky to not wake them up to get the full > information first. > > Do you want to try to tackle this ? If you do not feel like it, I can give it a > try too. I've already got it working ;) I think I sent you an earlier version of the patch a few weeks ago. I'll post my whole series tonight, after I fix the case of retrying the VERIFY if it fails.
