[PATCH 1/1] megaraid_sas: correct an info message
This was apparently forgotten in
894169db1 ("scsi: megaraid_sas: Use 63-bit DMA addressing").
Signed-off-by: Tomas Henzl
---
drivers/scsi/megaraid/megaraid_sas_base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
b/drivers/scsi/megaraid/megaraid_sas_base.c
index f7bdd7833..a6b1824cc 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -6236,7 +6236,7 @@ megasas_set_dma_mask(struct megasas_instance *instance)
instance->consistent_mask_64bit = true;
dev_info(&pdev->dev, "%s bit DMA mask and %s bit consistent mask\n",
-((*pdev->dev.dma_mask == DMA_BIT_MASK(64)) ? "63" : "32"),
+((*pdev->dev.dma_mask == DMA_BIT_MASK(63)) ? "63" : "32"),
(instance->consistent_mask_64bit ? "63" : "32"));
return 0;
--
2.20.1
Re: [PATCH v1 1/1] scsi: Synchronize request queue PM status only on successful resume
On Wed, 2019-01-02 at 14:25 +0800, stanley@mediatek.com wrote:
> From: Stanley Chu
>
> The commit 356fd2663cff ("scsi: Set request queue runtime PM status
> back to active on resume") fixed up the inconsistent RPM status between
> request queue and device. However changing request queue RPM status
> shall be done only on successful resume, otherwise status may be still
> inconsistent as below,
>
> Request queue: RPM_ACTIVE
> Device: RPM_SUSPENDED
>
> This ends up soft lockup because requests can be submitted to
> underlying devices but those devices and their required resource
> are not resumed.
>
> Signed-off-by: Stanley Chu
Please add "Fixes:" and "Cc: stable" tags and also Cc the author of commit
356fd2663cff.
> ---
> drivers/scsi/scsi_pm.c | 24 ++--
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index a2b4179..eff3e59 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -82,6 +82,20 @@ static int scsi_dev_type_resume(struct device *dev,
> pm_runtime_disable(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> +
> + /*
> + * Forcibly set runtime PM status of request queue to "active"
> + * to make sure we can again get requests from the queue
> + * (see also blk_pm_peek_request()).
> + *
> + * The resume hook will correct runtime PM status of the disk.
> + */
> + if (!err && scsi_is_sdev_device(dev)) {
> + struct scsi_device *sdev = to_scsi_device(dev);
> +
> + if (sdev->request_queue->dev)
> + blk_set_runtime_active(sdev->request_queue);
> + }
What makes you think that the sdev->request_queue->dev test is necessary? The
scsi_dev_type_resume() function is only called after blk_pm_runtime_init() has
finished so I don't think that test is necessary.
Additionally, since the above code occurs inside a block controlled by an
"if (err == 0)" statement, I think the !err test is redundant and should be
left out.
Thanks,
Bart.
Re: [PATCH v1 1/1] scsi: Synchronize request queue PM status only on successful resume
Hi Bart,
On Wed, 2019-01-02 at 08:15 -0800, Bart Van Assche wrote:
> On Wed, 2019-01-02 at 14:25 +0800, stanley@mediatek.com wrote:
> > From: Stanley Chu
> >
> > The commit 356fd2663cff ("scsi: Set request queue runtime PM status
> > back to active on resume") fixed up the inconsistent RPM status between
> > request queue and device. However changing request queue RPM status
> > shall be done only on successful resume, otherwise status may be still
> > inconsistent as below,
> >
> > Request queue: RPM_ACTIVE
> > Device: RPM_SUSPENDED
> >
> > This ends up soft lockup because requests can be submitted to
> > underlying devices but those devices and their required resource
> > are not resumed.
> >
> > Signed-off-by: Stanley Chu
>
> Please add "Fixes:" and "Cc: stable" tags and also Cc the author of commit
> 356fd2663cff.
Sure. Thanks for remind.
>
>
> > ---
> > drivers/scsi/scsi_pm.c | 24 ++--
> > 1 file changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> > index a2b4179..eff3e59 100644
> > --- a/drivers/scsi/scsi_pm.c
> > +++ b/drivers/scsi/scsi_pm.c
> > @@ -82,6 +82,20 @@ static int scsi_dev_type_resume(struct device *dev,
> > pm_runtime_disable(dev);
> > pm_runtime_set_active(dev);
> > pm_runtime_enable(dev);
> > +
> > + /*
> > +* Forcibly set runtime PM status of request queue to "active"
> > +* to make sure we can again get requests from the queue
> > +* (see also blk_pm_peek_request()).
> > +*
> > +* The resume hook will correct runtime PM status of the disk.
> > +*/
> > + if (!err && scsi_is_sdev_device(dev)) {
> > + struct scsi_device *sdev = to_scsi_device(dev);
> > +
> > + if (sdev->request_queue->dev)
> > + blk_set_runtime_active(sdev->request_queue);
> > + }
>
> What makes you think that the sdev->request_queue->dev test is necessary? The
> scsi_dev_type_resume() function is only called after blk_pm_runtime_init() has
> finished so I don't think that test is necessary.
We found NULL sdev->request_queue->dev may be dereferenced during below
system resume flow,
scsi_bus_resume_common()
=> async_schedule_domain(async_sdev_resume)
And then async_sdev_resume() is invoked asynchronously,
async_sdev_resume()
=> scsi_dev_type_resume(dev, do_scsi_resume)
=> blk_set_runtime_active(sdev->request_queue)
If a SCSI device does not have upper layer driver (like SCSI disk), it
may not be applied blk_pm_runtime_init() invoked by sd_probe() while
this SCSI device is added.
For example, some SCSI devices (like UFS Boot W-LUN) are added
explicitly in __scsi_add_device() by ufshcd_scsi_add_wlus() first and
thus sd_probe() for them is skipped because they are already visible.
For those SCSI devices, null sdev->request_queue->dev will be
dereferenced in blk_set_runtime_active() during above system resume
flow, therefore we add a null pointer checking for this case.
The same issue also happens on those SCSI devices before this patch as
below system resume flow while devices are already runtime-suspended.
scsi_bus_resume_common()
=> blk_set_runtime_active(to_scsi_device(dev)->request_queue)
>
> Additionally, since the above code occurs inside a block controlled by an
> "if (err == 0)" statement, I think the !err test is redundant and should be
> left out.
Sorry this is my code merge defect.
"err" here shall be returned value from pm_runtime_set_active().
I will fix it in v2.
>
> Thanks,
>
> Bart.
>
> ___
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
[PATCH v2 0/1] scsi: Synchronize request queue PM status only on successful resume
From: Stanley Chu Stanley Chu (1): scsi: Synchronize request queue PM status only on successful resume drivers/scsi/scsi_pm.c | 26 +++--- 1 file changed, 15 insertions(+), 11 deletions(-) -- 2.18.0
[PATCH v2] scsi: Synchronize request queue PM status only on successful resume
The commit 356fd2663cff ("scsi: Set request queue runtime PM status
back to active on resume") fixed up the inconsistent RPM status between
request queue and device. However changing request queue RPM status
shall be done only on successful resume, otherwise status may be still
inconsistent as below,
Request queue: RPM_ACTIVE
Device: RPM_SUSPENDED
This ends up soft lockup because requests can be submitted to
underlying devices but those devices and their required resource
are not resumed.
[PATCH v2 1/1] scsi: Synchronize request queue PM status only on successful resume
From: Stanley Chu
The commit 356fd2663cff ("scsi: Set request queue runtime PM status
back to active on resume") fixed up the inconsistent RPM status between
request queue and device. However changing request queue RPM status
shall be done only on successful resume, otherwise status may be still
inconsistent as below,
Request queue: RPM_ACTIVE
Device: RPM_SUSPENDED
This ends up soft lockup because requests can be submitted to
underlying devices but those devices and their required resource
are not resumed.
Fixes: 356fd2663cff ("scsi: Set request queue runtime PM status
back to active on resume")
Cc: sta...@vger.kernel.org
Signed-off-by: Stanley Chu
---
drivers/scsi/scsi_pm.c | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index a2b4179bfdf7..7639df91b110 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -80,8 +80,22 @@ static int scsi_dev_type_resume(struct device *dev,
if (err == 0) {
pm_runtime_disable(dev);
- pm_runtime_set_active(dev);
+ err = pm_runtime_set_active(dev);
pm_runtime_enable(dev);
+
+ /*
+* Forcibly set runtime PM status of request queue to "active"
+* to make sure we can again get requests from the queue
+* (see also blk_pm_peek_request()).
+*
+* The resume hook will correct runtime PM status of the disk.
+*/
+ if (!err && scsi_is_sdev_device(dev)) {
+ struct scsi_device *sdev = to_scsi_device(dev);
+
+ if (sdev->request_queue->dev)
+ blk_set_runtime_active(sdev->request_queue);
+ }
}
return err;
@@ -140,16 +154,6 @@ static int scsi_bus_resume_common(struct device *dev,
else
fn = NULL;
- /*
-* Forcibly set runtime PM status of request queue to "active" to
-* make sure we can again get requests from the queue (see also
-* blk_pm_peek_request()).
-*
-* The resume hook will correct runtime PM status of the disk.
-*/
- if (scsi_is_sdev_device(dev) && pm_runtime_suspended(dev))
- blk_set_runtime_active(to_scsi_device(dev)->request_queue);
-
if (fn) {
async_schedule_domain(fn, dev, &scsi_sd_pm_domain);
--
2.18.0
Re: [PATCH] scsi: associate bio write hint with WRITE CDB
On Wed, Dec 26, 2018 at 12:15:04PM +0800, Randall Huang wrote:
> In SPC-3, WRITE(10)/(16) support grouping function.
> Let's associate bio write hint with group number for
> enabling StreamID or Turbo Write feature.
>
> Signed-off-by: Randall Huang
> ---
> drivers/scsi/sd.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 4b49cb67617e..28bfa9ed2b54 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1201,7 +1201,12 @@ static int sd_setup_read_write_cmnd(struct scsi_cmnd
> *SCpnt)
> SCpnt->cmnd[11] = (unsigned char) (this_count >> 16) & 0xff;
> SCpnt->cmnd[12] = (unsigned char) (this_count >> 8) & 0xff;
> SCpnt->cmnd[13] = (unsigned char) this_count & 0xff;
> - SCpnt->cmnd[14] = SCpnt->cmnd[15] = 0;
> + if (rq_data_dir(rq) == WRITE) {
> + SCpnt->cmnd[14] = rq->bio->bi_write_hint & 0x3f;
> + } else {
> + SCpnt->cmnd[14] = 0;
> + }
No need for braces here.
But what I'm more worried about is devices not recognizing the feature
throwing up on the field. Can you check what SBC version first
references these or come up with some other decently smart conditional?
Maybe Martin has a good idea, too.
