On 2020-08-28 18:12, Alan Stern wrote: > On Fri, Aug 28, 2020 at 05:51:03PM -0700, Bart Van Assche wrote: >> On 2020-08-28 08:37, Alan Stern wrote: >>> There may be an even simpler solution: Ensure that SCSI domain >>> validation is mutually exclusive with runtime PM. It's already mutually >>> exclusive with system PM, so this makes sense. >>> >>> What do you think of the patch below? >>> >>> Alan Stern >>> >>> >>> Index: usb-devel/drivers/scsi/scsi_transport_spi.c >>> =================================================================== >>> --- usb-devel.orig/drivers/scsi/scsi_transport_spi.c >>> +++ usb-devel/drivers/scsi/scsi_transport_spi.c >>> @@ -1001,7 +1001,7 @@ spi_dv_device(struct scsi_device *sdev) >>> * Because this function and the power management code both call >>> * scsi_device_quiesce(), it is not safe to perform domain validation >>> * while suspend or resume is in progress. Hence the >>> - * lock/unlock_system_sleep() calls. >>> + * lock/unlock_system_sleep() and scsi_autopm_get/put_device() calls. >>> */ >>> lock_system_sleep(); >>> >>> @@ -1018,10 +1018,13 @@ spi_dv_device(struct scsi_device *sdev) >>> if (unlikely(!buffer)) >>> goto out_put; >>> >>> + if (scsi_autopm_get_device(sdev)) >>> + goto out_free; >>> + >>> /* We need to verify that the actual device will quiesce; the >>> * later target quiesce is just a nice to have */ >>> if (unlikely(scsi_device_quiesce(sdev))) >>> - goto out_free; >>> + goto out_autopm_put; >>> >>> scsi_target_quiesce(starget); >>> >>> @@ -1041,6 +1044,8 @@ spi_dv_device(struct scsi_device *sdev) >>> >>> spi_initial_dv(starget) = 1; >>> >>> + out_autopm_put: >>> + scsi_autopm_put_device(sdev); >>> out_free: >>> kfree(buffer); >>> out_put: >> >> Hi Alan, >> >> I think this is only a part of the solution. scsi_target_quiesce() invokes >> scsi_device_quiesce() and that function in turn calls blk_set_pm_only(). So >> I think that the above is not sufficient to fix the deadlock mentioned in >> my previous email. > > Sorry, it sounds like you misinterpreted my preceding email. I meant to > suggest that the patch above should be considered _instead of_ the patch > that introduced BLK_MQ_REQ_PM. So blk_queue_enter() would remain > unchanged, using the BLK_MQ_REQ_PREEMPT flag to decide whether or not to > postpone new requests. Thus the deadlock you're concerned about would > not arise. Hi Alan, Thanks for the clarification. I agree that the above patch should serialize SCSI DV and runtime PM. I'm fine with the above patch. Thanks, Bart.
