On 10/16/23 21:39, Phillip Susi wrote: > Damien Le Moal <dlemoal@xxxxxxxxxx> writes: > >> Yes, correct, but this does not create any issues in practice beside the >> undesired disk spinup. > > The issue it creates is the opposite of that: it breaks the desired spin > down. After some period of inactivity, the disk should be suspended, > but after a system resume, the kernel thinks that it already is, and so > won't suspend it again. That one should be fixable, though it I do not see an elegant method to do it. It would be easy with ugly code, e.g. tweaking the scsi device runtime pm state from libata... Not great. > >> Fixing that is not trivial because using runtime suspend/resume on the SCSI disk >> is just that, it will affect *only* the SCSI disk and not the ATA device and its >> port. In other words, a runtime suspend of the SCSI disk will spin down the >> drive but it will not runtime suspend the ATA port. So if you suspend >> the > > I tested this last week and it appeared to work. I enabled runtime pm > on the disk, as well as the ata port, and as soon as the disk suspended, > the port did as well. Never saw that in my tests when enabling runtime pm on the scsi disk only. Which is the important point here: there is no propagation of the suspend state down to the device parent it seems. > >> system, on resume, the ATA port will not be runtime suspended and so it will be >> resumed. The SCSI disk will not be resumed, but the ATA port resume will have >> spun up the disk, which we do not really want in that case. > > Right, I would rather the disk stay asleep if it has PuiS enabled, and > I'm working on a patch for that. In the process of doing that though, I > noticed that despite waking the disk up, it does not inform runtime pm > about that. But there are no runtime PM operations defined for ATA devices, only for ports. So not sure that matters... I am probably still missing something about runtime PM and devices ancestry. I focused a lot on system suspend/resume to fix the issues. runtime suspend/resume is next. > >> I am looking into this. Again, that is not a trivial fix. The other thing to >> notice here is that ATA port runtime suspend/resume is in fact broken: it does >> not track accesses to the device(s) connected to the port. And given that more >> than one device may be connected to a port, we need PM runtime reference >> counting to be done for this to work correctly. That is >> missing. Solutions are: > > Again, it seems to me that the child reference counting IS working. I am not sure of that, especially with cases of ATA ports with multiple disks (e.g. pmp or IDE). > >> fix everything or simply do not support ATA port runtime suspend/resume (i.e. >> remove code doing it). I am leaning toward the latter as it seems that no one >> actually noticed these issues because no one is actually using ATA port runtime >> suspend/resume... > > Probably nobody is using it yes, but that doesn't mean we shouldn't try > to get it working. It would be nice to have the drive go into deep > SLEEP instead of standby, as well as suspend the ata port, and possibly > even the whole AHCI controller rather than relying on the old APM drive > internal auto standby mode. > -- Damien Le Moal Western Digital Research
