Re: [PATCH v8 04/23] scsi: sd: Differentiate system and runtime start/stop management

[Damien Le Moal <dlemoal@kernel.org>]

On 10/16/23 21:39, Phillip Susi wrote:
> Damien Le Moal <dlemoal@kernel.org> 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