On 6/15/23 23:50, Alan Stern wrote:
> On Thu, Jun 15, 2023 at 05:33:26PM +0900, Damien Le Moal wrote:
>> When an ATA port is resumed from sleep, the port is reset and a power
>> management request issued to libata EH to reset the port and rescanning
>> the device(s) attached to the port. Device rescanning is done by
>> scheduling an ata_scsi_dev_rescan() work, which will execute
>> scsi_rescan_device().
>>
>> However, scsi_rescan_device() takes the generic device lock, which is
>> also taken by dpm_resume() when the SCSI device is resumed as well. If
>> a device rescan execution starts before the completion of the SCSI
>> device resume, the rcu locking used to refresh the cached VPD pages of
>> the device, combined with the generic device locking from
>> scsi_rescan_device() and from dpm_resume() can cause a deadlock.
>>
>> Avoid this situation by changing struct ata_port scsi_rescan_task to be
>> a delayed work instead of a simple work_struct. ata_scsi_dev_rescan() is
>> modified to check if the SCSI device associated with the ATA device that
>> must be rescanned is not suspended. If the SCSI device is still
>> suspended, ata_scsi_dev_rescan() returns early and reschedule itself for
>> execution after an arbitrary delay of 5ms.
>
> I don't understand the nature of the relationship between the ATA port
> and the corresponding SCSI device. Maybe you could explain it more
> fully, if you have time.
For ata devices, the parent -> child relationship is:
ata_host (the adapter) -> ata_port -> ata_link -> ata_device (HDD, SSD or ATAPI
CD/DVD)
For scsi devices representing ATA devices, it is:
ata_port -> scsi_host -> scsi_target -> scsi_device -> scsi_disk (or gendisk for
a CD/DVD)
When devices are scanned, libata will create ports and create a scsi_host for
each port, and a scsi device for each ata_device found on the link(s) for the
port. There is no direct relationship between an ata_device (the HDD or SSD) and
its scsi_device/scsi_disk (the device used to issue commands). The PM operations
we have are for ata_port and scsi_device. For the scsi device, the operations
are actually defined per device type, so in the scsi_disk driver (sd) for HDDs
and SSDs.
> But in any case, this approach seems like a layering violation. Why not
The layering violation is I think only with the direct reference to the scsi
device power.is_suspended field, which is definitely not pretty. But there are
some other drivers doing something similar:
$ git grep "power\.is_suspended" | grep -v drivers/base/power/main.c
drivers/gpu/drm/i915/display/intel_display_power_well.c: if
(!dev_priv->drm.dev->power.is_suspended)
drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c: if
(!dwmac->dev->power.is_suspended) {
drivers/platform/surface/surface_acpi_notify.c: if (d->dev->power.is_suspended) {
All the other code (ata calling scsi) is normal per the SCSI-to-ata translation
needed (all ata devices are represented as scsi devices in Linux, following the
SAT=scsi ATA translation specifications).
> instead call a SCSI utility routine to set a "needs_rescan" flag in the
> scsi_device structure? Then scsi_device_resume() could automatically
> call scsi_rescan_device() -- or rather an internal version that assumes
> the device lock is already held -- if the flag is set. Or it could
Yes, ideally, that is what we should do. Such fix is however more involved, and
so I prefer not to push for this right now as a fix for the regression at hand.
But I will definitively look into this.
> queue a non-delayed work routine to do this. (Is it important to have
> the rescan finish before userspace starts up and tries to access the ATA
> device again?)
>
> That, combined with a guaranteed order of resuming, would do what you
> want, right?
Yes. But again more fixes needed:
1) libata uses its error handling path to reset a port on resume and probe the
links again. The devices found are then revalidated (IDENTIFY command, reading
log pages etc). This call to EH is triggered in the pm->resume operation, but EH
runs as an asynchronous task. So the port->resume may complete before EH is
done. We need to change the EH call to be synchronous in ->resume
2) We need to remove triggering the task that does scsi_rescan_device() in EH
and move that trigger to scsi_device ->resume.
3) Modify scsi_device ->resume() to call scsi_rescan_device()
Safely doing (3) requires synchronization between ata_port->resume and
scsi_device->resume. We can do that by adding a device link between ata_device
and scsi_device. Doing so, the scsi device becomes the grandchild of the
ata_port and we are guaranteed that its ->resume will happen only once the ata
port ->resume is done.
That will also improve things as we will be able to rescan the scsi device (and
thus catch any change to the device) *before* the device ->resume operation
re-enables issuing user commands by un-quiescing the device request queue.
As you can see, that is all beyond a quick fix for a regression... Will work on
this.
Cheers.
>
> Alan Stern
--
Damien Le Moal
Western Digital Research
