On 11/7/23 22:45, Phillip Susi wrote:
> Damien Le Moal <dlemoal@xxxxxxxxxx> writes:
>
>> 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.
>
> I don't see any checks for whether the port was previously runtime
> suspended or not. It looks to me like it unconditionally revalidates
> all devices on system resume, which is why I'm trying to patch it to
> defer that until runtime resume.
The current code for port system resume looks like this:
static int ata_port_pm_resume(struct device *dev)
{
if (!pm_runtime_suspended(dev))
ata_port_resume(to_ata_port(dev), PMSG_RESUME, true);
return 0;
}
So if the port was runtime suspended before the system was suspended, it will
not be resumed and there will be no device revalidation on system resume.
But as we know, runtime suspending a port does not work...
>> 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.
>
> Ahh, right.... you mean for PATA right? Does anyone still have any of
> those these days? :)
Yes. Many embedded systems and people using old PCs still use that.
And that is not the only case. Port multiplier is the other major one since
multiple drives share the same link. In both cases, the drive "think" it is
talking to its own port, but the port is used to manage several devices. One
consequence of this is that the link speed for the port MUST be limited to the
lowest link speed of all devices. And to discover that, one has to probe all 15
ports of the PMP to discover the devices attached... Hence the fact that
bypassing revalidation being a really bad idea.
>> 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.
>
> Isn't the NORMAL time eh is triggered is in response to normal IO being
> submitted? Power management is kind of the auxiliary use of eh.
EH == error handler. So normally EH triggers only when there are errors. The
other case EH is used is for port scan, device revalidation, etc. Any internal
task that libata needs to do. The reason for using EH for these cases is that
libata internally issued commands are non-NCQ commands, which cannot be mixed
with the NCQ commands that result from the uppoer layers requests (block layer
and scsi disk). Using EH allows stopping these commands for EH to operate
exclusively on the device (or port). That is why suspend/resume use EH.
> I had another patch in my old series that had libata fake several
> commands rather than waking up the drive from SLEEP. That included
> IDENTIFY, FLUSH CACHE, STANDBY1_NOW, and CHECK POWER CONDITION. Those were needed to
> keep udisks or smartd from waking up the drive.
Patching udisk and smartd would be a lot more sensible. An application using
passthrough to talk to a drive without first checking if the drive is actually
active is not a sensible thing to do. Especially considering that runtime
autosuspend is a user initiated setup, systemd/udisk/smartd should respect that
setup and not attempt to poke at a disk that is sleeping.
--
Damien Le Moal
Western Digital Research
