Hi Bart,
Resent, typo fixed.
On 2020-11-15 04:57, Bart Van Assche wrote:
On 11/12/20 10:30 PM, Can Guo wrote:
If block layer runtime PM is enabled for one SCSI device, then there
is
no need to forcibly change the SCSI device and its request queue's
runtime
PM status to active in scsi_dev_type_resume(), since block layer PM
shall
resume the SCSI device on the demand of bios.
Please change "along" into "alone" in the subject of this patch (if
that
is what you meant).
Aha, sorry, a typo here.
+ if (scsi_is_sdev_device(dev)) {
+ struct scsi_device *sdev;
+ sdev = to_scsi_device(dev);
A minor comment: I think that "struct scsi_device *sdev =
to_scsi_device(dev);" fits on a single line.
Sure.
+ * If block layer runtime PM is enabled for the SCSI device,
+ * let block layer PM handle its runtime PM routines.
Please change "its runtime PM routines" into "runtime resume" or
similar. I think that will make the comment more clear.
Yes, thanks.
+ if (sdev->request_queue->dev)
+ return err;
+ }
The 'dev' member only exists in struct request_queue if CONFIG_PM=y so
the above won't compile if CONFIG_PM=n. How about adding a function in
include/linux/blk-pm.h to check whether or not runtime PM has been
enabled?
You are right.
Otherwise this patch looks good to me.
Actually, I am thinking about removing all the pm_runtime_set_active()
codes in both scsi_bus_resume_common() and scsi_dev_type_resume() - we
don't need to forcibly set the runtime PM status to RPM_ACTIVE for
either
SCSI host/target or SCSI devices.
Whenever we access one SCSI device, either block layer or somewhere in
the path (e.g. throgh sg IOCTL, sg_open() calls
scsi_autopm_get_device())
should runtime resume the device first, and the runtime PM framework
makes
sure device's parent (and its parent's parent and so on)gets resumed as
well.
Thus, the pm_runtime_set_active() seems redundant. What do you think?
Thanks,
Can Guo.
Thanks,
Bart.
