* [PATCH 1/9] block: Fix a race in the runtime power management code
2020-09-06 1:22 [PATCH 0/9] Rework runtime suspend and SCSI domain validation Bart Van Assche
@ 2020-09-06 1:22 ` Bart Van Assche
2020-09-06 1:22 ` [PATCH 2/9] ide: Do not set the RQF_PREEMPT flag for sense requests Bart Van Assche
` (9 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2020-09-06 1:22 UTC (permalink / raw)
To: Jens Axboe, Martin K . Petersen, James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern,
Bart Van Assche, Stanley Chu, Ming Lei, Rafael J . Wysocki,
stable, Can Guo
With the current implementation the following race can happen:
* blk_pre_runtime_suspend() calls blk_freeze_queue_start() and
blk_mq_unfreeze_queue().
* blk_queue_enter() calls blk_queue_pm_only() and that function returns
true.
* blk_queue_enter() calls blk_pm_request_resume() and that function does
not call pm_request_resume() because the queue runtime status is
RPM_ACTIVE.
* blk_pre_runtime_suspend() changes the queue status into RPM_SUSPENDING.
Fix this race by changing the queue runtime status into RPM_SUSPENDING
before switching q_usage_counter to atomic mode.
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Acked-by: Stanley Chu <stanley.chu@mediatek.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: stable <stable@vger.kernel.org>
Fixes: 986d413b7c15 ("blk-mq: Enable support for runtime power management")
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
block/blk-pm.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/block/blk-pm.c b/block/blk-pm.c
index b85234d758f7..17bd020268d4 100644
--- a/block/blk-pm.c
+++ b/block/blk-pm.c
@@ -67,6 +67,10 @@ int blk_pre_runtime_suspend(struct request_queue *q)
WARN_ON_ONCE(q->rpm_status != RPM_ACTIVE);
+ spin_lock_irq(&q->queue_lock);
+ q->rpm_status = RPM_SUSPENDING;
+ spin_unlock_irq(&q->queue_lock);
+
/*
* Increase the pm_only counter before checking whether any
* non-PM blk_queue_enter() calls are in progress to avoid that any
@@ -89,15 +93,14 @@ int blk_pre_runtime_suspend(struct request_queue *q)
/* Switch q_usage_counter back to per-cpu mode. */
blk_mq_unfreeze_queue(q);
- spin_lock_irq(&q->queue_lock);
- if (ret < 0)
+ if (ret < 0) {
+ spin_lock_irq(&q->queue_lock);
+ q->rpm_status = RPM_ACTIVE;
pm_runtime_mark_last_busy(q->dev);
- else
- q->rpm_status = RPM_SUSPENDING;
- spin_unlock_irq(&q->queue_lock);
+ spin_unlock_irq(&q->queue_lock);
- if (ret)
blk_clear_pm_only(q);
+ }
return ret;
}
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 2/9] ide: Do not set the RQF_PREEMPT flag for sense requests
2020-09-06 1:22 [PATCH 0/9] Rework runtime suspend and SCSI domain validation Bart Van Assche
2020-09-06 1:22 ` [PATCH 1/9] block: Fix a race in the runtime power management code Bart Van Assche
@ 2020-09-06 1:22 ` Bart Van Assche
2020-09-06 1:22 ` [PATCH 3/9] scsi: Pass a request queue pointer to __scsi_execute() Bart Van Assche
` (8 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2020-09-06 1:22 UTC (permalink / raw)
To: Jens Axboe, Martin K . Petersen, James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern,
Bart Van Assche, David S . Miller, Can Guo, Stanley Chu,
Ming Lei, Rafael J . Wysocki
RQF_PREEMPT is used for two different purposes in the legacy IDE code:
1. To mark power management requests.
2. To mark requests that should preempt another request. An (old)
explanation of that feature is as follows:
"The IDE driver in the Linux kernel normally uses a series of busywait
delays during its initialization. When the driver executes these
busywaits, the kernel does nothing for the duration of the wait. The
time spent in these waits could be used for other initialization
activities, if they could be run concurrently with these waits.
More specifically, busywait-style delays such as udelay() in module
init functions inhibit kernel preemption because the Big Kernel Lock
is held, while yielding APIs such as schedule_timeout() allow preemption.
This is true because the kernel handles the BKL specially and releases
and reacquires it across reschedules allowed by the current thread.
This IDE-preempt specification requires that the driver eliminate these
busywaits and replace them with a mechanism that allows other work to
proceed while the IDE driver is initializing."
Since I haven't found an implementation of (2), do not set the PREEMPT
flag for sense requests. This patch causes sense requests to be
postponed while a drive is suspended instead of being submitted to
ide_queue_rq().
If it would ever be necessary to restore the IDE PREEMPT functionality,
that can be done by introducing a new flag in struct ide_request.
This patch is a first step towards removing the PREEMPT flag from the
block layer.
Cc: David S. Miller <davem@davemloft.net>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Can Guo <cang@codeaurora.org>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/ide/ide-atapi.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 2162bc80f09e..013ad33fbbc8 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -223,7 +223,6 @@ void ide_prep_sense(ide_drive_t *drive, struct request *rq)
sense_rq->rq_disk = rq->rq_disk;
sense_rq->cmd_flags = REQ_OP_DRV_IN;
ide_req(sense_rq)->type = ATA_PRIV_SENSE;
- sense_rq->rq_flags |= RQF_PREEMPT;
req->cmd[0] = GPCMD_REQUEST_SENSE;
req->cmd[4] = cmd_len;
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 3/9] scsi: Pass a request queue pointer to __scsi_execute()
2020-09-06 1:22 [PATCH 0/9] Rework runtime suspend and SCSI domain validation Bart Van Assche
2020-09-06 1:22 ` [PATCH 1/9] block: Fix a race in the runtime power management code Bart Van Assche
2020-09-06 1:22 ` [PATCH 2/9] ide: Do not set the RQF_PREEMPT flag for sense requests Bart Van Assche
@ 2020-09-06 1:22 ` Bart Van Assche
2020-09-06 1:22 ` [PATCH 4/9] scsi: Rework scsi_mq_alloc_queue() Bart Van Assche
` (7 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2020-09-06 1:22 UTC (permalink / raw)
To: Jens Axboe, Martin K . Petersen, James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern,
Bart Van Assche, Can Guo, Stanley Chu, Ming Lei,
Rafael J . Wysocki
This patch does not change any functionality but makes a later patch easier
to read.
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Can Guo <cang@codeaurora.org>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/scsi/scsi_lib.c | 12 +++++-------
include/scsi/scsi_device.h | 8 ++++----
2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7affaaf8b98e..760976f27634 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -221,7 +221,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
/**
* __scsi_execute - insert request and wait for the result
- * @sdev: scsi device
+ * @q: queue to insert the request into
* @cmd: scsi command
* @data_direction: data direction
* @buffer: data buffer
@@ -237,7 +237,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
* Returns the scsi_cmnd result field if a command was executed, or a negative
* Linux error code if we didn't get that far.
*/
-int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+int __scsi_execute(struct request_queue *q, const unsigned char *cmd,
int data_direction, void *buffer, unsigned bufflen,
unsigned char *sense, struct scsi_sense_hdr *sshdr,
int timeout, int retries, u64 flags, req_flags_t rq_flags,
@@ -247,15 +247,13 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
struct scsi_request *rq;
int ret = DRIVER_ERROR << 24;
- req = blk_get_request(sdev->request_queue,
- data_direction == DMA_TO_DEVICE ?
+ req = blk_get_request(q, data_direction == DMA_TO_DEVICE ?
REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
if (IS_ERR(req))
return ret;
rq = scsi_req(req);
- if (bufflen && blk_rq_map_kern(sdev->request_queue, req,
- buffer, bufflen, GFP_NOIO))
+ if (bufflen && blk_rq_map_kern(q, req, buffer, bufflen, GFP_NOIO))
goto out;
rq->cmd_len = COMMAND_SIZE(cmd[0]);
@@ -268,7 +266,7 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
/*
* head injection *required* here otherwise quiesce won't work
*/
- blk_execute_rq(req->q, NULL, req, 1);
+ blk_execute_rq(q, NULL, req, 1);
/*
* Some devices (USB mass-storage in particular) may transfer
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index bc5909033d13..48c80793915e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -437,7 +437,7 @@ extern const char *scsi_device_state_name(enum scsi_device_state);
extern int scsi_is_sdev_device(const struct device *);
extern int scsi_is_target_device(const struct device *);
extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
-extern int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+extern int __scsi_execute(struct request_queue *q, const unsigned char *cmd,
int data_direction, void *buffer, unsigned bufflen,
unsigned char *sense, struct scsi_sense_hdr *sshdr,
int timeout, int retries, u64 flags,
@@ -448,9 +448,9 @@ extern int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
({ \
BUILD_BUG_ON((sense) != NULL && \
sizeof(sense) != SCSI_SENSE_BUFFERSIZE); \
- __scsi_execute(sdev, cmd, data_direction, buffer, bufflen, \
- sense, sshdr, timeout, retries, flags, rq_flags, \
- resid); \
+ __scsi_execute(sdev->request_queue, cmd, data_direction, \
+ buffer, bufflen, sense, sshdr, timeout, retries, \
+ flags, rq_flags, resid); \
})
static inline int scsi_execute_req(struct scsi_device *sdev,
const unsigned char *cmd, int data_direction, void *buffer,
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 4/9] scsi: Rework scsi_mq_alloc_queue()
2020-09-06 1:22 [PATCH 0/9] Rework runtime suspend and SCSI domain validation Bart Van Assche
` (2 preceding siblings ...)
2020-09-06 1:22 ` [PATCH 3/9] scsi: Pass a request queue pointer to __scsi_execute() Bart Van Assche
@ 2020-09-06 1:22 ` Bart Van Assche
2020-09-06 1:22 ` [PATCH 5/9] scsi: Do not wait for a request in scsi_eh_lock_door() Bart Van Assche
` (6 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2020-09-06 1:22 UTC (permalink / raw)
To: Jens Axboe, Martin K . Petersen, James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern,
Bart Van Assche, Can Guo, Stanley Chu, Ming Lei,
Rafael J . Wysocki
Do not modify sdev->request_queue. Remove the sdev->request_queue
assignment. That assignment is superfluous because scsi_mq_alloc_queue()
only has one caller and that caller calls scsi_mq_alloc_queue() as follows:
sdev->request_queue = scsi_mq_alloc_queue(sdev);
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Can Guo <cang@codeaurora.org>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/scsi/scsi_lib.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 760976f27634..9e3c2930ce40 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1854,14 +1854,15 @@ static const struct blk_mq_ops scsi_mq_ops = {
struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev)
{
- sdev->request_queue = blk_mq_init_queue(&sdev->host->tag_set);
- if (IS_ERR(sdev->request_queue))
+ struct request_queue *q = blk_mq_init_queue(&sdev->host->tag_set);
+
+ if (IS_ERR(q))
return NULL;
- sdev->request_queue->queuedata = sdev;
- __scsi_init_queue(sdev->host, sdev->request_queue);
- blk_queue_flag_set(QUEUE_FLAG_SCSI_PASSTHROUGH, sdev->request_queue);
- return sdev->request_queue;
+ q->queuedata = sdev;
+ __scsi_init_queue(sdev->host, q);
+ blk_queue_flag_set(QUEUE_FLAG_SCSI_PASSTHROUGH, q);
+ return q;
}
int scsi_mq_setup_tags(struct Scsi_Host *shost)
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 5/9] scsi: Do not wait for a request in scsi_eh_lock_door()
2020-09-06 1:22 [PATCH 0/9] Rework runtime suspend and SCSI domain validation Bart Van Assche
` (3 preceding siblings ...)
2020-09-06 1:22 ` [PATCH 4/9] scsi: Rework scsi_mq_alloc_queue() Bart Van Assche
@ 2020-09-06 1:22 ` Bart Van Assche
2020-09-06 16:03 ` Alan Stern
2020-09-06 1:22 ` [PATCH 6/9] scsi_transport_spi: Make spi_execute() accept a request queue pointer Bart Van Assche
` (5 subsequent siblings)
10 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2020-09-06 1:22 UTC (permalink / raw)
To: Jens Axboe, Martin K . Petersen, James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern,
Bart Van Assche, Can Guo, Stanley Chu, Ming Lei,
Rafael J . Wysocki
It is not guaranteed that a request is available when scsi_eh_lock_door()
is called. Hence pass the BLK_MQ_REQ_NOWAIT flag to blk_get_request().
This patch has a second purpose, namely preventing that
scsi_eh_lock_door() deadlocks if sdev->request_queue is frozen and if a
SCSI command is submitted to a SCSI device through a second request queue
that has not been frozen.
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Can Guo <cang@codeaurora.org>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/scsi/scsi_error.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index b197291c631a..f7604d930e3c 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1979,7 +1979,8 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
struct request *req;
struct scsi_request *rq;
- req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN, 0);
+ req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN,
+ BLK_MQ_REQ_NOWAIT);
if (IS_ERR(req))
return;
rq = scsi_req(req);
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 5/9] scsi: Do not wait for a request in scsi_eh_lock_door()
2020-09-06 1:22 ` [PATCH 5/9] scsi: Do not wait for a request in scsi_eh_lock_door() Bart Van Assche
@ 2020-09-06 16:03 ` Alan Stern
0 siblings, 0 replies; 17+ messages in thread
From: Alan Stern @ 2020-09-06 16:03 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Martin K . Petersen, James E . J . Bottomley,
linux-block, Christoph Hellwig, linux-scsi, Can Guo, Stanley Chu,
Ming Lei, Rafael J . Wysocki
On Sat, Sep 05, 2020 at 06:22:15PM -0700, Bart Van Assche wrote:
> It is not guaranteed that a request is available when scsi_eh_lock_door()
> is called. Hence pass the BLK_MQ_REQ_NOWAIT flag to blk_get_request().
> This patch has a second purpose, namely preventing that
> scsi_eh_lock_door() deadlocks if sdev->request_queue is frozen and if a
> SCSI command is submitted to a SCSI device through a second request queue
> that has not been frozen.
I think it would help readers understand the reason for doing this if
you mentioned that scsi_eh_lock_door() is the only place in the SCSI
error handler that calls blk_get_request().
Also mention that an upcoming patch in this series requires the error
handler to work okay even when the device's request queue is frozen.
(That's what the second sentence of your description is getting at, but
the connection is not clear.)
Alan Stern
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 6/9] scsi_transport_spi: Make spi_execute() accept a request queue pointer
2020-09-06 1:22 [PATCH 0/9] Rework runtime suspend and SCSI domain validation Bart Van Assche
` (4 preceding siblings ...)
2020-09-06 1:22 ` [PATCH 5/9] scsi: Do not wait for a request in scsi_eh_lock_door() Bart Van Assche
@ 2020-09-06 1:22 ` Bart Van Assche
2020-09-06 1:22 ` [PATCH 7/9] scsi_transport_spi: Freeze request queues instead of quiescing Bart Van Assche
` (4 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2020-09-06 1:22 UTC (permalink / raw)
To: Jens Axboe, Martin K . Petersen, James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern,
Bart Van Assche, James Bottomley, Woody Suwalski, Can Guo,
Stanley Chu, Ming Lei, Rafael J . Wysocki
Passing a request queue pointer to spi_execute() instead of a SCSI device
pointer will allow a later patch to associate two request queues with a
SCSI device. Additionally, instead of assuming that the device state is
SDEV_QUIESCE before domain validation starts, read the device state. This
patch does not change any functionality but makes a later patch easier to
read.
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Woody Suwalski <terraluna977@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Can Guo <cang@codeaurora.org>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/scsi/scsi_transport_spi.c | 69 ++++++++++++++++---------------
1 file changed, 36 insertions(+), 33 deletions(-)
diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
index f3d5b1bbd5aa..959990f66865 100644
--- a/drivers/scsi/scsi_transport_spi.c
+++ b/drivers/scsi/scsi_transport_spi.c
@@ -104,7 +104,7 @@ static int sprint_frac(char *dest, int value, int denom)
return result;
}
-static int spi_execute(struct scsi_device *sdev, const void *cmd,
+static int spi_execute(struct request_queue *q, const void *cmd,
enum dma_data_direction dir,
void *buffer, unsigned bufflen,
struct scsi_sense_hdr *sshdr)
@@ -117,7 +117,7 @@ static int spi_execute(struct scsi_device *sdev, const void *cmd,
sshdr = &sshdr_tmp;
for(i = 0; i < DV_RETRIES; i++) {
- result = scsi_execute(sdev, cmd, dir, buffer, bufflen, sense,
+ result = __scsi_execute(q, cmd, dir, buffer, bufflen, sense,
sshdr, DV_TIMEOUT, /* retries */ 1,
REQ_FAILFAST_DEV |
REQ_FAILFAST_TRANSPORT |
@@ -620,13 +620,14 @@ enum spi_compare_returns {
/* This is for read/write Domain Validation: If the device supports
* an echo buffer, we do read/write tests to it */
static enum spi_compare_returns
-spi_dv_device_echo_buffer(struct scsi_device *sdev, u8 *buffer,
- u8 *ptr, const int retries)
+spi_dv_device_echo_buffer(struct scsi_device *sdev, struct request_queue *q,
+ u8 *buffer, u8 *ptr, const int retries)
{
int len = ptr - buffer;
int j, k, r, result;
unsigned int pattern = 0x0000ffff;
struct scsi_sense_hdr sshdr;
+ enum scsi_device_state sdev_state = sdev->sdev_state;
const char spi_write_buffer[] = {
WRITE_BUFFER, 0x0a, 0, 0, 0, 0, 0, len >> 8, len & 0xff, 0
@@ -671,11 +672,10 @@ spi_dv_device_echo_buffer(struct scsi_device *sdev, u8 *buffer,
}
for (r = 0; r < retries; r++) {
- result = spi_execute(sdev, spi_write_buffer, DMA_TO_DEVICE,
+ result = spi_execute(q, spi_write_buffer, DMA_TO_DEVICE,
buffer, len, &sshdr);
if(result || !scsi_device_online(sdev)) {
-
- scsi_device_set_state(sdev, SDEV_QUIESCE);
+ scsi_device_set_state(sdev, sdev_state);
if (scsi_sense_valid(&sshdr)
&& sshdr.sense_key == ILLEGAL_REQUEST
/* INVALID FIELD IN CDB */
@@ -693,9 +693,9 @@ spi_dv_device_echo_buffer(struct scsi_device *sdev, u8 *buffer,
}
memset(ptr, 0, len);
- spi_execute(sdev, spi_read_buffer, DMA_FROM_DEVICE,
- ptr, len, NULL);
- scsi_device_set_state(sdev, SDEV_QUIESCE);
+ spi_execute(q, spi_read_buffer, DMA_FROM_DEVICE, ptr, len,
+ NULL);
+ scsi_device_set_state(sdev, sdev_state);
if (memcmp(buffer, ptr, len) != 0)
return SPI_COMPARE_FAILURE;
@@ -706,11 +706,12 @@ spi_dv_device_echo_buffer(struct scsi_device *sdev, u8 *buffer,
/* This is for the simplest form of Domain Validation: a read test
* on the inquiry data from the device */
static enum spi_compare_returns
-spi_dv_device_compare_inquiry(struct scsi_device *sdev, u8 *buffer,
- u8 *ptr, const int retries)
+spi_dv_device_compare_inquiry(struct scsi_device *sdev, struct request_queue *q,
+ u8 *buffer, u8 *ptr, const int retries)
{
int r, result;
const int len = sdev->inquiry_len;
+ enum scsi_device_state sdev_state = sdev->sdev_state;
const char spi_inquiry[] = {
INQUIRY, 0, 0, 0, len, 0
};
@@ -718,11 +719,11 @@ spi_dv_device_compare_inquiry(struct scsi_device *sdev, u8 *buffer,
for (r = 0; r < retries; r++) {
memset(ptr, 0, len);
- result = spi_execute(sdev, spi_inquiry, DMA_FROM_DEVICE,
- ptr, len, NULL);
+ result = spi_execute(q, spi_inquiry, DMA_FROM_DEVICE, ptr, len,
+ NULL);
if(result || !scsi_device_online(sdev)) {
- scsi_device_set_state(sdev, SDEV_QUIESCE);
+ scsi_device_set_state(sdev, sdev_state);
return SPI_COMPARE_FAILURE;
}
@@ -742,9 +743,10 @@ spi_dv_device_compare_inquiry(struct scsi_device *sdev, u8 *buffer,
}
static enum spi_compare_returns
-spi_dv_retrain(struct scsi_device *sdev, u8 *buffer, u8 *ptr,
- enum spi_compare_returns
- (*compare_fn)(struct scsi_device *, u8 *, u8 *, int))
+spi_dv_retrain(struct scsi_device *sdev, struct request_queue *q, u8 *buffer,
+ u8 *ptr, enum spi_compare_returns
+ (*compare_fn)(struct scsi_device *, struct request_queue *,
+ u8 *, u8 *, int))
{
struct spi_internal *i = to_spi_internal(sdev->host->transportt);
struct scsi_target *starget = sdev->sdev_target;
@@ -754,7 +756,7 @@ spi_dv_retrain(struct scsi_device *sdev, u8 *buffer, u8 *ptr,
for (;;) {
int newperiod;
- retval = compare_fn(sdev, buffer, ptr, DV_LOOPS);
+ retval = compare_fn(sdev, q, buffer, ptr, DV_LOOPS);
if (retval == SPI_COMPARE_SUCCESS
|| retval == SPI_COMPARE_SKIP_TEST)
@@ -800,7 +802,8 @@ spi_dv_retrain(struct scsi_device *sdev, u8 *buffer, u8 *ptr,
}
static int
-spi_dv_device_get_echo_buffer(struct scsi_device *sdev, u8 *buffer)
+spi_dv_device_get_echo_buffer(struct scsi_device *sdev,
+ struct request_queue *q, u8 *buffer)
{
int l, result;
@@ -824,8 +827,8 @@ spi_dv_device_get_echo_buffer(struct scsi_device *sdev, u8 *buffer)
* (reservation conflict, device not ready, etc) just
* skip the write tests */
for (l = 0; ; l++) {
- result = spi_execute(sdev, spi_test_unit_ready, DMA_NONE,
- NULL, 0, NULL);
+ result = spi_execute(q, spi_test_unit_ready, DMA_NONE, NULL, 0,
+ NULL);
if(result) {
if(l >= 3)
@@ -836,8 +839,8 @@ spi_dv_device_get_echo_buffer(struct scsi_device *sdev, u8 *buffer)
}
}
- result = spi_execute(sdev, spi_read_buffer_descriptor,
- DMA_FROM_DEVICE, buffer, 4, NULL);
+ result = spi_execute(q, spi_read_buffer_descriptor, DMA_FROM_DEVICE,
+ buffer, 4, NULL);
if (result)
/* Device has no echo buffer */
@@ -847,7 +850,8 @@ spi_dv_device_get_echo_buffer(struct scsi_device *sdev, u8 *buffer)
}
static void
-spi_dv_device_internal(struct scsi_device *sdev, u8 *buffer)
+spi_dv_device_internal(struct scsi_device *sdev, struct request_queue *q,
+ u8 *buffer)
{
struct spi_internal *i = to_spi_internal(sdev->host->transportt);
struct scsi_target *starget = sdev->sdev_target;
@@ -859,7 +863,7 @@ spi_dv_device_internal(struct scsi_device *sdev, u8 *buffer)
DV_SET(offset, 0);
DV_SET(width, 0);
- if (spi_dv_device_compare_inquiry(sdev, buffer, buffer, DV_LOOPS)
+ if (spi_dv_device_compare_inquiry(sdev, q, buffer, buffer, DV_LOOPS)
!= SPI_COMPARE_SUCCESS) {
starget_printk(KERN_ERR, starget, "Domain Validation Initial Inquiry Failed\n");
/* FIXME: should probably offline the device here? */
@@ -875,9 +879,8 @@ spi_dv_device_internal(struct scsi_device *sdev, u8 *buffer)
if (i->f->set_width && max_width) {
i->f->set_width(starget, 1);
- if (spi_dv_device_compare_inquiry(sdev, buffer,
- buffer + len,
- DV_LOOPS)
+ if (spi_dv_device_compare_inquiry(sdev, q, buffer, buffer + len,
+ DV_LOOPS)
!= SPI_COMPARE_SUCCESS) {
starget_printk(KERN_ERR, starget, "Wide Transfers Fail\n");
i->f->set_width(starget, 0);
@@ -946,7 +949,7 @@ spi_dv_device_internal(struct scsi_device *sdev, u8 *buffer)
DV_SET(width, max_width);
/* Do the read only INQUIRY tests */
- spi_dv_retrain(sdev, buffer, buffer + sdev->inquiry_len,
+ spi_dv_retrain(sdev, q, buffer, buffer + sdev->inquiry_len,
spi_dv_device_compare_inquiry);
/* See if we actually managed to negotiate and sustain DT */
if (i->f->get_dt)
@@ -958,7 +961,7 @@ spi_dv_device_internal(struct scsi_device *sdev, u8 *buffer)
* negotiated DT */
if (len == -1 && spi_dt(starget))
- len = spi_dv_device_get_echo_buffer(sdev, buffer);
+ len = spi_dv_device_get_echo_buffer(sdev, q, buffer);
if (len <= 0) {
starget_printk(KERN_INFO, starget, "Domain Validation skipping write tests\n");
@@ -970,7 +973,7 @@ spi_dv_device_internal(struct scsi_device *sdev, u8 *buffer)
len = SPI_MAX_ECHO_BUFFER_SIZE;
}
- if (spi_dv_retrain(sdev, buffer, buffer + len,
+ if (spi_dv_retrain(sdev, q, buffer, buffer + len,
spi_dv_device_echo_buffer)
== SPI_COMPARE_SKIP_TEST) {
/* OK, the stupid drive can't do a write echo buffer
@@ -1030,7 +1033,7 @@ spi_dv_device(struct scsi_device *sdev)
starget_printk(KERN_INFO, starget, "Beginning Domain Validation\n");
- spi_dv_device_internal(sdev, buffer);
+ spi_dv_device_internal(sdev, sdev->request_queue, buffer);
starget_printk(KERN_INFO, starget, "Ending Domain Validation\n");
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 7/9] scsi_transport_spi: Freeze request queues instead of quiescing
2020-09-06 1:22 [PATCH 0/9] Rework runtime suspend and SCSI domain validation Bart Van Assche
` (5 preceding siblings ...)
2020-09-06 1:22 ` [PATCH 6/9] scsi_transport_spi: Make spi_execute() accept a request queue pointer Bart Van Assche
@ 2020-09-06 1:22 ` Bart Van Assche
2020-09-09 3:51 ` Bart Van Assche
2020-09-06 1:22 ` [PATCH 8/9] block, scsi, ide: Only process PM requests if rpm_status != RPM_ACTIVE Bart Van Assche
` (3 subsequent siblings)
10 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2020-09-06 1:22 UTC (permalink / raw)
To: Jens Axboe, Martin K . Petersen, James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern,
Bart Van Assche, James Bottomley, Woody Suwalski, Can Guo,
Stanley Chu, Ming Lei, Rafael J . Wysocki
Instead of quiescing the request queues involved in domain validation,
freeze these. As a result, the struct request_queue pm_only member is no
longer set during domain validation. That will allow to modify
scsi_execute() such that it stops setting the BLK_MQ_REQ_PREEMPT flag.
Three additional changes in this patch are that scsi_mq_alloc_queue() is
exported, that scsi_device_quiesce() is no longer exported and that
scsi_target_{quiesce,resume}() have been changed into
scsi_target_{freeze,unfreeze}().
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Woody Suwalski <terraluna977@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Can Guo <cang@codeaurora.org>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/scsi/scsi_lib.c | 22 +++++-----
drivers/scsi/scsi_priv.h | 2 +
drivers/scsi/scsi_transport_spi.c | 72 ++++++++++++++++++++-----------
include/scsi/scsi_device.h | 6 +--
4 files changed, 61 insertions(+), 41 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9e3c2930ce40..0b0cb727e699 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1864,6 +1864,7 @@ struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev)
blk_queue_flag_set(QUEUE_FLAG_SCSI_PASSTHROUGH, q);
return q;
}
+EXPORT_SYMBOL_GPL(scsi_mq_alloc_queue);
int scsi_mq_setup_tags(struct Scsi_Host *shost)
{
@@ -2538,7 +2539,6 @@ scsi_device_quiesce(struct scsi_device *sdev)
return err;
}
-EXPORT_SYMBOL(scsi_device_quiesce);
/**
* scsi_device_resume - Restart user issued commands to a quiesced device.
@@ -2567,30 +2567,30 @@ void scsi_device_resume(struct scsi_device *sdev)
EXPORT_SYMBOL(scsi_device_resume);
static void
-device_quiesce_fn(struct scsi_device *sdev, void *data)
+device_freeze_fn(struct scsi_device *sdev, void *data)
{
- scsi_device_quiesce(sdev);
+ blk_mq_freeze_queue(sdev->request_queue);
}
void
-scsi_target_quiesce(struct scsi_target *starget)
+scsi_target_freeze(struct scsi_target *starget)
{
- starget_for_each_device(starget, NULL, device_quiesce_fn);
+ starget_for_each_device(starget, NULL, device_freeze_fn);
}
-EXPORT_SYMBOL(scsi_target_quiesce);
+EXPORT_SYMBOL(scsi_target_freeze);
static void
-device_resume_fn(struct scsi_device *sdev, void *data)
+device_unfreeze_fn(struct scsi_device *sdev, void *data)
{
- scsi_device_resume(sdev);
+ blk_mq_unfreeze_queue(sdev->request_queue);
}
void
-scsi_target_resume(struct scsi_target *starget)
+scsi_target_unfreeze(struct scsi_target *starget)
{
- starget_for_each_device(starget, NULL, device_resume_fn);
+ starget_for_each_device(starget, NULL, device_unfreeze_fn);
}
-EXPORT_SYMBOL(scsi_target_resume);
+EXPORT_SYMBOL(scsi_target_unfreeze);
/**
* scsi_internal_device_block_nowait - try to transition to the SDEV_BLOCK state
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index d12ada035961..6b9203df84c8 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -95,6 +95,8 @@ extern int scsi_mq_setup_tags(struct Scsi_Host *shost);
extern void scsi_mq_destroy_tags(struct Scsi_Host *shost);
extern void scsi_exit_queue(void);
extern void scsi_evt_thread(struct work_struct *work);
+extern int scsi_device_quiesce(struct scsi_device *sdev);
+extern void scsi_device_resume(struct scsi_device *sdev);
struct request_queue;
struct request;
diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
index 959990f66865..f0ef9ab008c5 100644
--- a/drivers/scsi/scsi_transport_spi.c
+++ b/drivers/scsi/scsi_transport_spi.c
@@ -997,59 +997,79 @@ void
spi_dv_device(struct scsi_device *sdev)
{
struct scsi_target *starget = sdev->sdev_target;
+ struct request_queue *q2;
u8 *buffer;
const int len = SPI_MAX_ECHO_BUFFER_SIZE*2;
/*
- * 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.
+ * Because this function creates a new request queue that is not
+ * visible to the rest of the system, this function must be serialized
+ * against suspend, resume and runtime power management. Hence the
+ * lock/unlock_system_sleep() and scsi_autopm_{get,put}_device()
+ * calls.
*/
lock_system_sleep();
+ if (scsi_autopm_get_device(sdev))
+ goto unlock_system_sleep;
+
if (unlikely(spi_dv_in_progress(starget)))
- goto unlock;
+ goto put_autopm;
if (unlikely(scsi_device_get(sdev)))
- goto unlock;
-
- spi_dv_in_progress(starget) = 1;
+ goto put_autopm;
buffer = kzalloc(len, GFP_KERNEL);
if (unlikely(!buffer))
- goto out_put;
-
- /* 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;
-
- scsi_target_quiesce(starget);
+ goto put_sdev;
spi_dv_pending(starget) = 1;
+
mutex_lock(&spi_dv_mutex(starget));
+ if (unlikely(spi_dv_in_progress(starget)))
+ goto clear_pending;
+
+ spi_dv_in_progress(starget) = 1;
starget_printk(KERN_INFO, starget, "Beginning Domain Validation\n");
- spi_dv_device_internal(sdev, sdev->request_queue, buffer);
+ q2 = scsi_mq_alloc_queue(sdev);
+
+ if (q2) {
+ /*
+ * Freeze the target such that no other subsystem can submit
+ * SCSI commands to 'sdev'. Submitting SCSI commands through
+ * q2 may trigger the SCSI error handler. The SCSI error
+ * handler must be able to handle a frozen sdev->request_queue
+ * and must also use blk_mq_rq_from_pdu(q2)->q instead of
+ * sdev->request_queue if it would be necessary to access q2
+ * directly.
+ */
+ scsi_target_freeze(starget);
+ spi_dv_device_internal(sdev, q2, buffer);
+ blk_cleanup_queue(q2);
+ scsi_target_unfreeze(starget);
+ }
starget_printk(KERN_INFO, starget, "Ending Domain Validation\n");
- mutex_unlock(&spi_dv_mutex(starget));
- spi_dv_pending(starget) = 0;
-
- scsi_target_resume(starget);
-
spi_initial_dv(starget) = 1;
+ spi_dv_in_progress(starget) = 0;
+
+clear_pending:
+ spi_dv_pending(starget) = 0;
+ mutex_unlock(&spi_dv_mutex(starget));
- out_free:
kfree(buffer);
- out_put:
- spi_dv_in_progress(starget) = 0;
+
+put_sdev:
scsi_device_put(sdev);
-unlock:
+
+put_autopm:
+ scsi_autopm_put_device(sdev);
+
+unlock_system_sleep:
unlock_system_sleep();
}
EXPORT_SYMBOL(spi_dv_device);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 48c80793915e..891ffebadebe 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -422,10 +422,8 @@ extern struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
extern void sdev_evt_send(struct scsi_device *sdev, struct scsi_event *evt);
extern void sdev_evt_send_simple(struct scsi_device *sdev,
enum scsi_device_event evt_type, gfp_t gfpflags);
-extern int scsi_device_quiesce(struct scsi_device *sdev);
-extern void scsi_device_resume(struct scsi_device *sdev);
-extern void scsi_target_quiesce(struct scsi_target *);
-extern void scsi_target_resume(struct scsi_target *);
+extern void scsi_target_freeze(struct scsi_target *);
+extern void scsi_target_unfreeze(struct scsi_target *);
extern void scsi_scan_target(struct device *parent, unsigned int channel,
unsigned int id, u64 lun,
enum scsi_scan_mode rescan);
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 7/9] scsi_transport_spi: Freeze request queues instead of quiescing
2020-09-06 1:22 ` [PATCH 7/9] scsi_transport_spi: Freeze request queues instead of quiescing Bart Van Assche
@ 2020-09-09 3:51 ` Bart Van Assche
0 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2020-09-09 3:51 UTC (permalink / raw)
To: Jens Axboe, Martin K . Petersen, James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern,
James Bottomley, Woody Suwalski, Can Guo, Stanley Chu, Ming Lei,
Rafael J . Wysocki
On 2020-09-05 18:22, Bart Van Assche wrote:
> Instead of quiescing the request queues involved in domain validation,
> freeze these. As a result, the struct request_queue pm_only member is no
> longer set during domain validation. That will allow to modify
> scsi_execute() such that it stops setting the BLK_MQ_REQ_PREEMPT flag.
> Three additional changes in this patch are that scsi_mq_alloc_queue() is
> exported, that scsi_device_quiesce() is no longer exported and that
> scsi_target_{quiesce,resume}() have been changed into
> scsi_target_{freeze,unfreeze}().
Hi Woody,
Do you still own a setup on which SPI DV can be tested?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 8/9] block, scsi, ide: Only process PM requests if rpm_status != RPM_ACTIVE
2020-09-06 1:22 [PATCH 0/9] Rework runtime suspend and SCSI domain validation Bart Van Assche
` (6 preceding siblings ...)
2020-09-06 1:22 ` [PATCH 7/9] scsi_transport_spi: Freeze request queues instead of quiescing Bart Van Assche
@ 2020-09-06 1:22 ` Bart Van Assche
2020-09-06 1:22 ` [PATCH 9/9] block: Do not accept any requests while suspended Bart Van Assche
` (2 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2020-09-06 1:22 UTC (permalink / raw)
To: Jens Axboe, Martin K . Petersen, James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern,
Bart Van Assche, Can Guo, Stanley Chu, Ming Lei,
Rafael J . Wysocki, Martin Kepplinger
Instead of submitting all SCSI commands submitted with scsi_execute() to
a SCSI device if rpm_status != RPM_ACTIVE, only submit RQF_PM (power
management requests) if rpm_status != RPM_ACTIVE. Remove flag
RQF_PREEMPT since it is no longer necessary.
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Can Guo <cang@codeaurora.org>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
block/blk-core.c | 6 +++---
block/blk-mq-debugfs.c | 1 -
block/blk-mq.c | 4 ++--
drivers/ide/ide-io.c | 3 +--
drivers/ide/ide-pm.c | 2 +-
drivers/scsi/scsi_lib.c | 27 ++++++++++++++-------------
include/linux/blk-mq.h | 4 ++--
include/linux/blkdev.h | 6 +-----
8 files changed, 24 insertions(+), 29 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 093649bd252e..c5a2f8173b1f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -420,11 +420,11 @@ EXPORT_SYMBOL(blk_cleanup_queue);
/**
* blk_queue_enter() - try to increase q->q_usage_counter
* @q: request queue pointer
- * @flags: BLK_MQ_REQ_NOWAIT and/or BLK_MQ_REQ_PREEMPT
+ * @flags: BLK_MQ_REQ_NOWAIT and/or BLK_MQ_REQ_PM
*/
int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
{
- const bool pm = flags & BLK_MQ_REQ_PREEMPT;
+ const bool pm = flags & BLK_MQ_REQ_PM;
while (true) {
bool success = false;
@@ -629,7 +629,7 @@ struct request *blk_get_request(struct request_queue *q, unsigned int op,
struct request *req;
WARN_ON_ONCE(op & REQ_NOWAIT);
- WARN_ON_ONCE(flags & ~(BLK_MQ_REQ_NOWAIT | BLK_MQ_REQ_PREEMPT));
+ WARN_ON_ONCE(flags & ~(BLK_MQ_REQ_NOWAIT | BLK_MQ_REQ_PM));
req = blk_mq_alloc_request(q, op, flags);
if (!IS_ERR(req) && q->mq_ops->initialize_rq_fn)
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 645b7f800cb8..135eeb7a5056 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -296,7 +296,6 @@ static const char *const rqf_name[] = {
RQF_NAME(MIXED_MERGE),
RQF_NAME(MQ_INFLIGHT),
RQF_NAME(DONTPREP),
- RQF_NAME(PREEMPT),
RQF_NAME(FAILED),
RQF_NAME(QUIET),
RQF_NAME(ELVPRIV),
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 4abb71459f94..09900b94931f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -292,8 +292,8 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
rq->mq_hctx = data->hctx;
rq->rq_flags = 0;
rq->cmd_flags = data->cmd_flags;
- if (data->flags & BLK_MQ_REQ_PREEMPT)
- rq->rq_flags |= RQF_PREEMPT;
+ if (data->flags & BLK_MQ_REQ_PM)
+ rq->rq_flags |= RQF_PM;
if (blk_queue_io_stat(data->q))
rq->rq_flags |= RQF_IO_STAT;
INIT_LIST_HEAD(&rq->queuelist);
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 1a53c7a75224..beb850679fa9 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -522,8 +522,7 @@ blk_status_t ide_issue_rq(ide_drive_t *drive, struct request *rq,
* state machine.
*/
if ((drive->dev_flags & IDE_DFLAG_BLOCKED) &&
- ata_pm_request(rq) == 0 &&
- (rq->rq_flags & RQF_PREEMPT) == 0) {
+ ata_pm_request(rq) == 0) {
/* there should be no pending command at this point */
ide_unlock_port(hwif);
goto plug_device;
diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c
index 192e6c65d34e..82ab308f1aaf 100644
--- a/drivers/ide/ide-pm.c
+++ b/drivers/ide/ide-pm.c
@@ -77,7 +77,7 @@ int generic_ide_resume(struct device *dev)
}
memset(&rqpm, 0, sizeof(rqpm));
- rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, BLK_MQ_REQ_PREEMPT);
+ rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, BLK_MQ_REQ_PM);
ide_req(rq)->type = ATA_PRIV_PM_RESUME;
ide_req(rq)->special = &rqpm;
rqpm.pm_step = IDE_PM_START_RESUME;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0b0cb727e699..d365bcd53670 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -248,7 +248,8 @@ int __scsi_execute(struct request_queue *q, const unsigned char *cmd,
int ret = DRIVER_ERROR << 24;
req = blk_get_request(q, data_direction == DMA_TO_DEVICE ?
- REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
+ REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN,
+ rq_flags & RQF_PM ? BLK_MQ_REQ_PM : 0);
if (IS_ERR(req))
return ret;
rq = scsi_req(req);
@@ -1216,6 +1217,8 @@ static blk_status_t
scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
{
switch (sdev->sdev_state) {
+ case SDEV_CREATED:
+ return BLK_STS_OK;
case SDEV_OFFLINE:
case SDEV_TRANSPORT_OFFLINE:
/*
@@ -1242,18 +1245,18 @@ scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
return BLK_STS_RESOURCE;
case SDEV_QUIESCE:
/*
- * If the devices is blocked we defer normal commands.
+ * If the device is blocked we only accept power management
+ * commands.
*/
- if (req && !(req->rq_flags & RQF_PREEMPT))
+ if (req && WARN_ON_ONCE(!(req->rq_flags & RQF_PM)))
return BLK_STS_RESOURCE;
return BLK_STS_OK;
default:
/*
* For any other not fully online state we only allow
- * special commands. In particular any user initiated
- * command is not allowed.
+ * power management commands.
*/
- if (req && !(req->rq_flags & RQF_PREEMPT))
+ if (req && !(req->rq_flags & RQF_PM))
return BLK_STS_IOERR;
return BLK_STS_OK;
}
@@ -2487,15 +2490,13 @@ void sdev_evt_send_simple(struct scsi_device *sdev,
EXPORT_SYMBOL_GPL(sdev_evt_send_simple);
/**
- * scsi_device_quiesce - Block user issued commands.
+ * scsi_device_quiesce - Block all commands except power management.
* @sdev: scsi device to quiesce.
*
* This works by trying to transition to the SDEV_QUIESCE state
* (which must be a legal transition). When the device is in this
- * state, only special requests will be accepted, all others will
- * be deferred. Since special requests may also be requeued requests,
- * a successful return doesn't guarantee the device will be
- * totally quiescent.
+ * state, only power management requests will be accepted, all others will
+ * be deferred.
*
* Must be called with user context, may sleep.
*
@@ -2556,12 +2557,12 @@ void scsi_device_resume(struct scsi_device *sdev)
* device deleted during suspend)
*/
mutex_lock(&sdev->state_mutex);
+ if (sdev->sdev_state == SDEV_QUIESCE)
+ scsi_device_set_state(sdev, SDEV_RUNNING);
if (sdev->quiesced_by) {
sdev->quiesced_by = NULL;
blk_clear_pm_only(sdev->request_queue);
}
- if (sdev->sdev_state == SDEV_QUIESCE)
- scsi_device_set_state(sdev, SDEV_RUNNING);
mutex_unlock(&sdev->state_mutex);
}
EXPORT_SYMBOL(scsi_device_resume);
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index b23eeca4d677..1fa350592830 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -444,8 +444,8 @@ enum {
BLK_MQ_REQ_NOWAIT = (__force blk_mq_req_flags_t)(1 << 0),
/* allocate from reserved pool */
BLK_MQ_REQ_RESERVED = (__force blk_mq_req_flags_t)(1 << 1),
- /* set RQF_PREEMPT */
- BLK_MQ_REQ_PREEMPT = (__force blk_mq_req_flags_t)(1 << 3),
+ /* set RQF_PM */
+ BLK_MQ_REQ_PM = (__force blk_mq_req_flags_t)(1 << 3),
};
struct request *blk_mq_alloc_request(struct request_queue *q, unsigned int op,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 7d82959e7b86..13e57464216c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -78,9 +78,6 @@ typedef __u32 __bitwise req_flags_t;
#define RQF_MQ_INFLIGHT ((__force req_flags_t)(1 << 6))
/* don't call prep for this one */
#define RQF_DONTPREP ((__force req_flags_t)(1 << 7))
-/* set for "ide_preempt" requests and also for requests for which the SCSI
- "quiesce" state must be ignored. */
-#define RQF_PREEMPT ((__force req_flags_t)(1 << 8))
/* vaguely specified driver internal error. Ignored by the block layer */
#define RQF_FAILED ((__force req_flags_t)(1 << 10))
/* don't warn about errors */
@@ -425,8 +422,7 @@ struct request_queue {
unsigned long queue_flags;
/*
* Number of contexts that have called blk_set_pm_only(). If this
- * counter is above zero then only RQF_PM and RQF_PREEMPT requests are
- * processed.
+ * counter is above zero then only RQF_PM requests are processed.
*/
atomic_t pm_only;
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 9/9] block: Do not accept any requests while suspended
2020-09-06 1:22 [PATCH 0/9] Rework runtime suspend and SCSI domain validation Bart Van Assche
` (7 preceding siblings ...)
2020-09-06 1:22 ` [PATCH 8/9] block, scsi, ide: Only process PM requests if rpm_status != RPM_ACTIVE Bart Van Assche
@ 2020-09-06 1:22 ` Bart Van Assche
2020-09-06 16:06 ` [PATCH 0/9] Rework runtime suspend and SCSI domain validation Alan Stern
2020-09-19 3:45 ` Bart Van Assche
10 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2020-09-06 1:22 UTC (permalink / raw)
To: Jens Axboe, Martin K . Petersen, James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern,
Bart Van Assche, Can Guo, Stanley Chu, Ming Lei,
Rafael J . Wysocki, Martin Kepplinger
From: Alan Stern <stern@rowland.harvard.edu>
blk_queue_enter() accepts BLK_MQ_REQ_PREEMPT independent of the runtime
power management state. Since SCSI domain validation no longer depends on
this behavior, modify the behavior of blk_queue_enter() as follows:
- Do not accept any requests while suspended.
- Only process power management requests while suspending or resuming.
Submitting BLK_MQ_REQ_PREEMPT requests to a device that is runtime-
suspended causes runtime-suspended block devices not to resume as they
should. The request which should cause a runtime resume instead gets
issued directly, without resuming the device first. Of course the device
can't handle it properly, the I/O fails, and the device remains suspended.
The problem is fixed by checking that the queue's runtime-PM status
isn't RPM_SUSPENDED before allowing a request to be issued, and
queuing a runtime-resume request if it is. In particular, the inline
blk_pm_request_resume() routine is renamed blk_pm_resume_queue() and
the code is unified by merging the surrounding checks into the
routine. If the queue isn't set up for runtime PM, or there currently
is no restriction on allowed requests, the request is allowed.
Likewise if the BLK_MQ_REQ_PREEMPT flag is set and the status isn't
RPM_SUSPENDED. Otherwise a runtime resume is queued and the request
is blocked until conditions are more suitable.
Cc: Can Guo <cang@codeaurora.org>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reported-and-tested-by: Martin Kepplinger <martin.kepplinger@puri.sm>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
[ bvanassche: modified commit message and removed Cc: stable because without
the previous patches from this series this patch would break parallel SCSI
domain validation ]
---
block/blk-core.c | 6 +++---
block/blk-pm.h | 14 +++++++++-----
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index c5a2f8173b1f..22c1c6e6c0e1 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -436,7 +436,8 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
* responsible for ensuring that that counter is
* globally visible before the queue is unfrozen.
*/
- if (pm || !blk_queue_pm_only(q)) {
+ if ((pm && q->rpm_status != RPM_SUSPENDED) ||
+ !blk_queue_pm_only(q)) {
success = true;
} else {
percpu_ref_put(&q->q_usage_counter);
@@ -461,8 +462,7 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
wait_event(q->mq_freeze_wq,
(!q->mq_freeze_depth &&
- (pm || (blk_pm_request_resume(q),
- !blk_queue_pm_only(q)))) ||
+ blk_pm_resume_queue(pm, q)) ||
blk_queue_dying(q));
if (blk_queue_dying(q))
return -ENODEV;
diff --git a/block/blk-pm.h b/block/blk-pm.h
index ea5507d23e75..a2283cc9f716 100644
--- a/block/blk-pm.h
+++ b/block/blk-pm.h
@@ -6,11 +6,14 @@
#include <linux/pm_runtime.h>
#ifdef CONFIG_PM
-static inline void blk_pm_request_resume(struct request_queue *q)
+static inline int blk_pm_resume_queue(const bool pm, struct request_queue *q)
{
- if (q->dev && (q->rpm_status == RPM_SUSPENDED ||
- q->rpm_status == RPM_SUSPENDING))
- pm_request_resume(q->dev);
+ if (!q->dev || !blk_queue_pm_only(q))
+ return 1; /* Nothing to do */
+ if (pm && q->rpm_status != RPM_SUSPENDED)
+ return 1; /* Request allowed */
+ pm_request_resume(q->dev);
+ return 0;
}
static inline void blk_pm_mark_last_busy(struct request *rq)
@@ -44,8 +47,9 @@ static inline void blk_pm_put_request(struct request *rq)
--rq->q->nr_pending;
}
#else
-static inline void blk_pm_request_resume(struct request_queue *q)
+static inline int blk_pm_resume_queue(const bool pm, struct request_queue *q)
{
+ return 1;
}
static inline void blk_pm_mark_last_busy(struct request *rq)
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 0/9] Rework runtime suspend and SCSI domain validation
2020-09-06 1:22 [PATCH 0/9] Rework runtime suspend and SCSI domain validation Bart Van Assche
` (8 preceding siblings ...)
2020-09-06 1:22 ` [PATCH 9/9] block: Do not accept any requests while suspended Bart Van Assche
@ 2020-09-06 16:06 ` Alan Stern
2020-09-19 3:45 ` Bart Van Assche
10 siblings, 0 replies; 17+ messages in thread
From: Alan Stern @ 2020-09-06 16:06 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, Martin K . Petersen, James E . J . Bottomley,
linux-block, Christoph Hellwig, linux-scsi
On Sat, Sep 05, 2020 at 06:22:10PM -0700, Bart Van Assche wrote:
> Hi Jens,
>
> The SCSI runtime suspend and domain validation mechanisms both use
> scsi_device_quiesce(). scsi_device_quiesce() restricts blk_queue_enter() to
> BLK_MQ_REQ_PREEMPT requests. There is a conflict between the requirements
> of runtime suspend and SCSI domain validation: no requests must be sent to
> runtime suspended devices that are in the state RPM_SUSPENDED while
> BLK_MQ_REQ_PREEMPT requests must be processed during SCSI domain
> validation. This conflict is resolved by reworking the SCSI domain
> validation implementation.
>
> Hybernation and runtime suspend have been retested but SCSI domain
> validation not yet.
>
> Please consider this patch series for kernel v5.10.
>
> Thanks,
>
> Bart.
>
> Alan Stern (1):
> block: Do not accept any requests while suspended
>
> Bart Van Assche (8):
> block: Fix a race in the runtime power management code
> ide: Do not set the RQF_PREEMPT flag for sense requests
> scsi: Pass a request queue pointer to __scsi_execute()
> scsi: Rework scsi_mq_alloc_queue()
> scsi: Do not wait for a request in scsi_eh_lock_door()
> scsi_transport_spi: Make spi_execute() accept a request queue pointer
> scsi_transport_spi: Freeze request queues instead of quiescing
> block, scsi, ide: Only process PM requests if rpm_status != RPM_ACTIVE
I don't know enough about the IDE subsystem to judge patches 2 or 8.
For patches 3 - 7:
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
Alan Stern
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 0/9] Rework runtime suspend and SCSI domain validation
2020-09-06 1:22 [PATCH 0/9] Rework runtime suspend and SCSI domain validation Bart Van Assche
` (9 preceding siblings ...)
2020-09-06 16:06 ` [PATCH 0/9] Rework runtime suspend and SCSI domain validation Alan Stern
@ 2020-09-19 3:45 ` Bart Van Assche
2020-09-21 6:00 ` Hannes Reinecke
2020-09-29 12:33 ` Martin Kepplinger
10 siblings, 2 replies; 17+ messages in thread
From: Bart Van Assche @ 2020-09-19 3:45 UTC (permalink / raw)
To: Jens Axboe, Martin K . Petersen, James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern
On 2020-09-05 18:22, Bart Van Assche wrote:
> The SCSI runtime suspend and domain validation mechanisms both use
> scsi_device_quiesce(). scsi_device_quiesce() restricts blk_queue_enter() to
> BLK_MQ_REQ_PREEMPT requests. There is a conflict between the requirements
> of runtime suspend and SCSI domain validation: no requests must be sent to
> runtime suspended devices that are in the state RPM_SUSPENDED while
> BLK_MQ_REQ_PREEMPT requests must be processed during SCSI domain
> validation. This conflict is resolved by reworking the SCSI domain
> validation implementation.
>
> Hybernation and runtime suspend have been retested but SCSI domain
> validation not yet.
Hi Martin and James,
Please advise how to proceed with this patch series. This patch series
includes an important fix for runtime power management. Unfortunately
the only way to fix runtime powermanagement is by reworking SPI DV and
I don't have access to a setup on which I can test the SPI DV changes.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 0/9] Rework runtime suspend and SCSI domain validation
2020-09-19 3:45 ` Bart Van Assche
@ 2020-09-21 6:00 ` Hannes Reinecke
2020-09-30 2:47 ` Martin K. Petersen
2020-09-29 12:33 ` Martin Kepplinger
1 sibling, 1 reply; 17+ messages in thread
From: Hannes Reinecke @ 2020-09-21 6:00 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe, Martin K . Petersen,
James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern
On 9/19/20 5:45 AM, Bart Van Assche wrote:
> On 2020-09-05 18:22, Bart Van Assche wrote:
>> The SCSI runtime suspend and domain validation mechanisms both use
>> scsi_device_quiesce(). scsi_device_quiesce() restricts blk_queue_enter() to
>> BLK_MQ_REQ_PREEMPT requests. There is a conflict between the requirements
>> of runtime suspend and SCSI domain validation: no requests must be sent to
>> runtime suspended devices that are in the state RPM_SUSPENDED while
>> BLK_MQ_REQ_PREEMPT requests must be processed during SCSI domain
>> validation. This conflict is resolved by reworking the SCSI domain
>> validation implementation.
>>
>> Hybernation and runtime suspend have been retested but SCSI domain
>> validation not yet.
>
> Hi Martin and James,
>
> Please advise how to proceed with this patch series. This patch series
> includes an important fix for runtime power management. Unfortunately
> the only way to fix runtime powermanagement is by reworking SPI DV and
> I don't have access to a setup on which I can test the SPI DV changes.
>
I'll check if I can resurrect my setup.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/9] Rework runtime suspend and SCSI domain validation
2020-09-21 6:00 ` Hannes Reinecke
@ 2020-09-30 2:47 ` Martin K. Petersen
0 siblings, 0 replies; 17+ messages in thread
From: Martin K. Petersen @ 2020-09-30 2:47 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Bart Van Assche, Jens Axboe, Martin K . Petersen,
James E . J . Bottomley, linux-block, Christoph Hellwig,
linux-scsi, Alan Stern
Hi Hannes,
> I'll check if I can resurrect my setup.
I'm afraid I don't have any SPI stuff to test with and it would be good
to get at least a basic smoke test performed on this series. So if you
have a setup to try it on that would be much appreciated.
Thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/9] Rework runtime suspend and SCSI domain validation
2020-09-19 3:45 ` Bart Van Assche
2020-09-21 6:00 ` Hannes Reinecke
@ 2020-09-29 12:33 ` Martin Kepplinger
1 sibling, 0 replies; 17+ messages in thread
From: Martin Kepplinger @ 2020-09-29 12:33 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe, Martin K . Petersen,
James E . J . Bottomley
Cc: linux-block, Christoph Hellwig, linux-scsi, Alan Stern
On 19.09.20 05:45, Bart Van Assche wrote:
> On 2020-09-05 18:22, Bart Van Assche wrote:
>> The SCSI runtime suspend and domain validation mechanisms both use
>> scsi_device_quiesce(). scsi_device_quiesce() restricts blk_queue_enter() to
>> BLK_MQ_REQ_PREEMPT requests. There is a conflict between the requirements
>> of runtime suspend and SCSI domain validation: no requests must be sent to
>> runtime suspended devices that are in the state RPM_SUSPENDED while
>> BLK_MQ_REQ_PREEMPT requests must be processed during SCSI domain
>> validation. This conflict is resolved by reworking the SCSI domain
>> validation implementation.
>>
>> Hybernation and runtime suspend have been retested but SCSI domain
>> validation not yet.
>
> Hi Martin and James,
>
> Please advise how to proceed with this patch series. This patch series
> includes an important fix for runtime power management. Unfortunately
> the only way to fix runtime powermanagement is by reworking SPI DV and
> I don't have access to a setup on which I can test the SPI DV changes.
>
> Thanks,
>
> Bart.
>
I only have my setup: SD cardreader connected via USB. I run this series
which fixes runtime PM in the block layer for me.
Sidenote: I need the following hack for my particular device too:
https://lore.kernel.org/linux-scsi/20200824190400.12339-1-martin.kepplinger@puri.sm/
I hope someone else can test the rest for us!
thanks for working on this,
martin
^ permalink raw reply [flat|nested] 17+ messages in thread