[PATCH v4 0/9] Rework runtime suspend and SPI domain validation

[PATCH v4 0/9] Rework runtime suspend and SPI domain validation

[Bart Van Assche]

Hi Martin,

The SCSI runtime suspend and SPI 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, runtime suspend and SCSI domain validation have been retested.

Please consider this patch series for kernel v5.11.

Thanks,

Bart.

Changes between v3 and v4:
- Instead of creating a second request queue for SPI DV, set RQF_PM.

Changes between v2 and v3:
- Inlined scsi_mq_alloc_queue() into scsi_alloc_sdev() as requested by
  Christoph.

Changes between v1 and v2:
- Rebased this patch series on top of kernel v5.10-rc1.

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
  block: Introduce BLK_MQ_REQ_PM
  ide: Do not set the RQF_PREEMPT flag for sense requests
  ide: Mark power management requests with RQF_PM instead of RQF_PREEMPT
  scsi: Do not wait for a request in scsi_eh_lock_door()
  scsi_transport_spi: Set RQF_PM for domain validation commands
  scsi: Only process PM requests if rpm_status != RPM_ACTIVE
  block: Remove RQF_PREEMPT and BLK_MQ_REQ_PREEMPT

 block/blk-core.c                  | 12 ++++++------
 block/blk-mq-debugfs.c            |  1 -
 block/blk-mq.c                    |  4 ++--
 block/blk-pm.c                    | 15 +++++++++------
 block/blk-pm.h                    | 14 +++++++++-----
 drivers/ide/ide-atapi.c           |  1 -
 drivers/ide/ide-io.c              |  7 +------
 drivers/ide/ide-pm.c              |  2 +-
 drivers/scsi/scsi_error.c         |  7 ++++++-
 drivers/scsi/scsi_lib.c           | 27 ++++++++++++++-------------
 drivers/scsi/scsi_transport_spi.c | 27 +++++++++++++++++++--------
 include/linux/blk-mq.h            |  4 ++--
 include/linux/blkdev.h            |  6 +-----
 13 files changed, 70 insertions(+), 57 deletions(-)

[PATCH v4 1/9] block: Fix a race in the runtime power management code

[Bart Van Assche]

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Hannes Reinecke <hare@suse.de>
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;
 }

[PATCH v4 2/9] block: Introduce BLK_MQ_REQ_PM

[Bart Van Assche]

Introduce the BLK_MQ_REQ_PM flag. This flag makes the request allocation
functions set RQF_PM. This is the first step towards removing
BLK_MQ_REQ_PREEMPT.

Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Can Guo <cang@codeaurora.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-core.c       | 7 ++++---
 block/blk-mq.c         | 2 ++
 include/linux/blk-mq.h | 2 ++
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2db8bda43b6e..10696f9fb6ac 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -424,11 +424,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, BLK_MQ_REQ_PM and/or BLK_MQ_REQ_PREEMPT
  */
 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 | BLK_MQ_REQ_PREEMPT);
 
 	while (true) {
 		bool success = false;
@@ -630,7 +630,8 @@ 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 |
+			       BLK_MQ_REQ_PREEMPT));
 
 	req = blk_mq_alloc_request(q, op, flags);
 	if (!IS_ERR(req) && q->mq_ops->initialize_rq_fn)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 1b25ec2fe9be..b5880a1fb38d 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -292,6 +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_PM)
+		rq->rq_flags |= RQF_PM;
 	if (data->flags & BLK_MQ_REQ_PREEMPT)
 		rq->rq_flags |= RQF_PREEMPT;
 	if (blk_queue_io_stat(data->q))
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index b23eeca4d677..c00e856c6fb1 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -444,6 +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_PM */
+	BLK_MQ_REQ_PM		= (__force blk_mq_req_flags_t)(1 << 2),
 	/* set RQF_PREEMPT */
 	BLK_MQ_REQ_PREEMPT	= (__force blk_mq_req_flags_t)(1 << 3),
 };

[PATCH v4 3/9] ide: Do not set the RQF_PREEMPT flag for sense requests

[Bart Van Assche]

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.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
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 -
 drivers/ide/ide-io.c    | 5 -----
 2 files changed, 6 deletions(-)

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;
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 1a53c7a75224..c210ea3bd02f 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -515,11 +515,6 @@ blk_status_t ide_issue_rq(ide_drive_t *drive, struct request *rq,
 		 * above to return us whatever is in the queue. Since we call
 		 * ide_do_request() ourselves, we end up taking requests while
 		 * the queue is blocked...
-		 * 
-		 * We let requests forced at head of queue with ide-preempt
-		 * though. I hope that doesn't happen too much, hopefully not
-		 * unless the subdriver triggers such a thing in its own PM
-		 * state machine.
 		 */
 		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) &&
 		    ata_pm_request(rq) == 0 &&

[PATCH v4 4/9] ide: Mark power management requests with RQF_PM instead of RQF_PREEMPT

[Bart Van Assche]

This is another step that prepares for the removal of RQF_PREEMPT.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
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-io.c | 2 +-
 drivers/ide/ide-pm.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index c210ea3bd02f..4867b67b60d6 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -518,7 +518,7 @@ blk_status_t ide_issue_rq(ide_drive_t *drive, struct request *rq,
 		 */
 		if ((drive->dev_flags & IDE_DFLAG_BLOCKED) &&
 		    ata_pm_request(rq) == 0 &&
-		    (rq->rq_flags & RQF_PREEMPT) == 0) {
+		    (rq->rq_flags & RQF_PM) == 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;

[PATCH v4 5/9] scsi: Do not wait for a request in scsi_eh_lock_door()

[Bart Van Assche]

scsi_eh_lock_door() is the only function in the SCSI error handler that
calls blk_get_request(). 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().

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index d94449188270..6de6e1bf3dcb 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1993,7 +1993,12 @@ 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);
+	/*
+	 * It is not guaranteed that a request is available nor that
+	 * sdev->request_queue is unfrozen. Hence the BLK_MQ_REQ_NOWAIT below.
+	 */
+	req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN,
+			      BLK_MQ_REQ_NOWAIT);
 	if (IS_ERR(req))
 		return;
 	rq = scsi_req(req);

[PATCH v4 6/9] scsi_transport_spi: Set RQF_PM for domain validation commands

[Bart Van Assche]

Disable runtime power management during domain validation. Since a later
patch removes RQF_PREEMPT, set RQF_PM for domain validation commands such
that these are executed in the quiesced SCSI device state.

Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Christoph Hellwig <hch@lst.de>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Woody Suwalski <terraluna977@gmail.com>
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>
Cc: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_transport_spi.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
index f3d5b1bbd5aa..c37dd15d16d2 100644
--- a/drivers/scsi/scsi_transport_spi.c
+++ b/drivers/scsi/scsi_transport_spi.c
@@ -117,12 +117,16 @@ static int spi_execute(struct scsi_device *sdev, const void *cmd,
 		sshdr = &sshdr_tmp;
 
 	for(i = 0; i < DV_RETRIES; i++) {
+		/*
+		 * The purpose of the RQF_PM flag below is to bypass the
+		 * SDEV_QUIESCE state.
+		 */
 		result = scsi_execute(sdev, cmd, dir, buffer, bufflen, sense,
 				      sshdr, DV_TIMEOUT, /* retries */ 1,
 				      REQ_FAILFAST_DEV |
 				      REQ_FAILFAST_TRANSPORT |
 				      REQ_FAILFAST_DRIVER,
-				      0, NULL);
+				      RQF_PM, NULL);
 		if (driver_byte(result) != DRIVER_SENSE ||
 		    sshdr->sense_key != UNIT_ATTENTION)
 			break;
@@ -1005,23 +1009,26 @@ spi_dv_device(struct scsi_device *sdev)
 	 */
 	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;
+		goto put_autopm;
 
 	spi_dv_in_progress(starget) = 1;
 
 	buffer = kzalloc(len, GFP_KERNEL);
 
 	if (unlikely(!buffer))
-		goto out_put;
+		goto put_sdev;
 
 	/* 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 free_buffer;
 
 	scsi_target_quiesce(starget);
 
@@ -1041,12 +1048,16 @@ spi_dv_device(struct scsi_device *sdev)
 
 	spi_initial_dv(starget) = 1;
 
- out_free:
+free_buffer:
 	kfree(buffer);
- out_put:
+
+put_sdev:
 	spi_dv_in_progress(starget) = 0;
 	scsi_device_put(sdev);
-unlock:
+put_autopm:
+	scsi_autopm_put_device(sdev);
+
+unlock_system_sleep:
 	unlock_system_sleep();
 }
 EXPORT_SYMBOL(spi_dv_device);

[PATCH v4 7/9] scsi: Only process PM requests if rpm_status != RPM_ACTIVE

[Bart Van Assche]

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. This patch makes the
SCSI core handle the runtime power management status (rpm_status) as it
should be handled.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Can Guo <cang@codeaurora.org>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Martin Kepplinger <martin.kepplinger@puri.sm>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_lib.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b7ac14571415..91bc39a4c3c3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -249,7 +249,8 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 
 	req = blk_get_request(sdev->request_queue,
 			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);
@@ -1206,6 +1207,8 @@ static blk_status_t
 scsi_device_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:
 		/*
@@ -1232,18 +1235,18 @@ scsi_device_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;
 	}
@@ -2517,15 +2520,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.
  *
@@ -2587,12 +2588,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);

[PATCH v4 8/9] block: Remove RQF_PREEMPT and BLK_MQ_REQ_PREEMPT

[Bart Van Assche]

Remove flag RQF_PREEMPT and BLK_MQ_REQ_PREEMPT since these are no longer
used by any kernel code.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Can Guo <cang@codeaurora.org>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Martin Kepplinger <martin.kepplinger@puri.sm>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-core.c       | 7 +++----
 block/blk-mq-debugfs.c | 1 -
 block/blk-mq.c         | 2 --
 include/linux/blk-mq.h | 2 --
 include/linux/blkdev.h | 6 +-----
 5 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 10696f9fb6ac..a00bce9f46d8 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -424,11 +424,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, BLK_MQ_REQ_PM 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_PM | BLK_MQ_REQ_PREEMPT);
+	const bool pm = flags & BLK_MQ_REQ_PM;
 
 	while (true) {
 		bool success = false;
@@ -630,8 +630,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_PM |
-			       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 3094542e12ae..9336a6f8d6ef 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -297,7 +297,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 b5880a1fb38d..d50504888b68 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -294,8 +294,6 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	rq->cmd_flags = data->cmd_flags;
 	if (data->flags & BLK_MQ_REQ_PM)
 		rq->rq_flags |= RQF_PM;
-	if (data->flags & BLK_MQ_REQ_PREEMPT)
-		rq->rq_flags |= RQF_PREEMPT;
 	if (blk_queue_io_stat(data->q))
 		rq->rq_flags |= RQF_IO_STAT;
 	INIT_LIST_HEAD(&rq->queuelist);
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index c00e856c6fb1..88af1df94308 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -446,8 +446,6 @@ enum {
 	BLK_MQ_REQ_RESERVED	= (__force blk_mq_req_flags_t)(1 << 1),
 	/* set RQF_PM */
 	BLK_MQ_REQ_PM		= (__force blk_mq_req_flags_t)(1 << 2),
-	/* set RQF_PREEMPT */
-	BLK_MQ_REQ_PREEMPT	= (__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 639cae2c158b..7d4b746f7e6a 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -79,9 +79,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 */
@@ -430,8 +427,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;
 

[PATCH v4 9/9] block: Do not accept any requests while suspended

[Bart Van Assche]

From: Alan Stern <stern@rowland.harvard.edu>

blk_queue_enter() accepts BLK_MQ_REQ_PM requests independent of the runtime
power management state. Now that 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_PM requests to a device that is runtime suspended
causes runtime-suspended 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_PM 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.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Can Guo <cang@codeaurora.org>
Reviewed-by: 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 a00bce9f46d8..230880cbf8c8 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -440,7 +440,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);
@@ -465,8 +466,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)

Re: [PATCH v4 3/9] ide: Do not set the RQF_PREEMPT flag for sense requests

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 2/9] block: Introduce BLK_MQ_REQ_PM

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 4/9] ide: Mark power management requests with RQF_PM instead of RQF_PREEMPT

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 6/9] scsi_transport_spi: Set RQF_PM for domain validation commands

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 7/9] scsi: Only process PM requests if rpm_status != RPM_ACTIVE

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 8/9] block: Remove RQF_PREEMPT and BLK_MQ_REQ_PREEMPT

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 0/9] Rework runtime suspend and SPI domain validation

[Martin K. Petersen]

Jens,

Any objections to me picking this up?

> The SCSI runtime suspend and SPI 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, runtime suspend and SCSI domain validation have been
> retested.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v4 2/9] block: Introduce BLK_MQ_REQ_PM

[Hannes Reinecke]

On 11/30/20 3:46 AM, Bart Van Assche wrote:
> Introduce the BLK_MQ_REQ_PM flag. This flag makes the request allocation
> functions set RQF_PM. This is the first step towards removing
> BLK_MQ_REQ_PREEMPT.
> 
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Can Guo <cang@codeaurora.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   block/blk-core.c       | 7 ++++---
>   block/blk-mq.c         | 2 ++
>   include/linux/blk-mq.h | 2 ++
>   3 files changed, 8 insertions(+), 3 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

Re: [PATCH v4 3/9] ide: Do not set the RQF_PREEMPT flag for sense requests

[Hannes Reinecke]

On 11/30/20 3:46 AM, Bart Van Assche wrote:
> 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.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> 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 -
>   drivers/ide/ide-io.c    | 5 -----
>   2 files changed, 6 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

Re: [PATCH v4 4/9] ide: Mark power management requests with RQF_PM instead of RQF_PREEMPT

[Hannes Reinecke]

On 11/30/20 3:46 AM, Bart Van Assche wrote:
> This is another step that prepares for the removal of RQF_PREEMPT.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> 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-io.c | 2 +-
>   drivers/ide/ide-pm.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

Re: [PATCH v4 5/9] scsi: Do not wait for a request in scsi_eh_lock_door()

[Hannes Reinecke]

On 11/30/20 3:46 AM, Bart Van Assche wrote:
> scsi_eh_lock_door() is the only function in the SCSI error handler that
> calls blk_get_request(). 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().
> 
> Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 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 | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index d94449188270..6de6e1bf3dcb 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1993,7 +1993,12 @@ 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);
> +	/*
> +	 * It is not guaranteed that a request is available nor that
> +	 * sdev->request_queue is unfrozen. Hence the BLK_MQ_REQ_NOWAIT below.
> +	 */
> +	req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN,
> +			      BLK_MQ_REQ_NOWAIT);
>   	if (IS_ERR(req))
>   		return;
>   	rq = scsi_req(req);
> 
Well ... had been thinking about that one, too.
The idea of this function is that prior to SCSI EH the device was locked
via scsi_set_medium_removal(). And during SCSI EH the device might have 
become unlocked, so we need to lock it again.
However, scsi_set_medium_removal() not only issues the 
PREVENT_ALLOW_MEDIUM_REMOVAL command, but also sets the 'locked' flag 
based on the result.
So if we fail to get a request here, shouldn't we unset the 'locked' 
flag, too?
And what does happen if we fail here? There is no return value, hence 
SCSI EH might run to completion, and the system will continue
with an unlocked door ...
Not sure if that's a good idea.

But anyway, at the very least unset the 'locked' flag upon failure such 
that the internal state is correctly updated.

_Actually_, the flag should be unset after each successful SCSI EH step, 
to mirror the actual state. But this is probably out of scope for this 
patch.

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

Re: [PATCH v4 7/9] scsi: Only process PM requests if rpm_status != RPM_ACTIVE

[Hannes Reinecke]

On 11/30/20 3:46 AM, Bart Van Assche wrote:
> 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. This patch makes the
> SCSI core handle the runtime power management status (rpm_status) as it
> should be handled.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Martin Kepplinger <martin.kepplinger@puri.sm>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/scsi_lib.c | 27 ++++++++++++++-------------
>   1 file changed, 14 insertions(+), 13 deletions(-)
> Reviewed-by: Hannes Reinecke <hare@suse.de>

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

Re: [PATCH v4 8/9] block: Remove RQF_PREEMPT and BLK_MQ_REQ_PREEMPT

[Hannes Reinecke]

On 11/30/20 3:46 AM, Bart Van Assche wrote:
> Remove flag RQF_PREEMPT and BLK_MQ_REQ_PREEMPT since these are no longer
> used by any kernel code.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Martin Kepplinger <martin.kepplinger@puri.sm>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   block/blk-core.c       | 7 +++----
>   block/blk-mq-debugfs.c | 1 -
>   block/blk-mq.c         | 2 --
>   include/linux/blk-mq.h | 2 --
>   include/linux/blkdev.h | 6 +-----
>   5 files changed, 4 insertions(+), 14 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

Re: [PATCH v4 9/9] block: Do not accept any requests while suspended

[Hannes Reinecke]

On 11/30/20 3:46 AM, Bart Van Assche wrote:
> From: Alan Stern <stern@rowland.harvard.edu>
> 
> blk_queue_enter() accepts BLK_MQ_REQ_PM requests independent of the runtime
> power management state. Now that 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_PM requests to a device that is runtime suspended
> causes runtime-suspended 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_PM 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.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Can Guo <cang@codeaurora.org>
> Reviewed-by: 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(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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

Re: [PATCH v4 5/9] scsi: Do not wait for a request in scsi_eh_lock_door()

[Bart Van Assche]

On 12/1/20 11:06 PM, Hannes Reinecke wrote:
> On 11/30/20 3:46 AM, Bart Van Assche wrote:
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index d94449188270..6de6e1bf3dcb 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -1993,7 +1993,12 @@ 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);
>> +    /*
>> +     * It is not guaranteed that a request is available nor that
>> +     * sdev->request_queue is unfrozen. Hence the BLK_MQ_REQ_NOWAIT
>> below.
>> +     */
>> +    req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN,
>> +                  BLK_MQ_REQ_NOWAIT);
>>       if (IS_ERR(req))
>>           return;
>>       rq = scsi_req(req);
>>
>
> Well ... had been thinking about that one, too.
> The idea of this function is that prior to SCSI EH the device was locked
> via scsi_set_medium_removal(). And during SCSI EH the device might have
> become unlocked, so we need to lock it again.
> However, scsi_set_medium_removal() not only issues the
> PREVENT_ALLOW_MEDIUM_REMOVAL command, but also sets the 'locked' flag
> based on the result.
> So if we fail to get a request here, shouldn't we unset the 'locked'
> flag, too?

Probably not. My interpretation of the 'locked' flag is that it
represents the door state before error handling began. The following
code in the SCSI error handler restores the door state after a bus reset:

	if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) {
		scsi_eh_lock_door(sdev);
		sdev->was_reset = 0;
	}

> And what does happen if we fail here? There is no return value, hence
> SCSI EH might run to completion, and the system will continue
> with an unlocked door ...
> Not sure if that's a good idea.

How about applying the following patch on top of patch 5/9?

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 6de6e1bf3dcb..feac7262e40e 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1988,7 +1988,7 @@ static void eh_lock_door_done(struct request *req, blk_status_t status)
  * 	We queue up an asynchronous "ALLOW MEDIUM REMOVAL" request on the
  * 	head of the devices request queue, and continue.
  */
-static void scsi_eh_lock_door(struct scsi_device *sdev)
+static int scsi_eh_lock_door(struct scsi_device *sdev)
 {
 	struct request *req;
 	struct scsi_request *rq;
@@ -2000,7 +2000,7 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
 	req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN,
 			      BLK_MQ_REQ_NOWAIT);
 	if (IS_ERR(req))
-		return;
+		return PTR_ERR(req);
 	rq = scsi_req(req);

 	rq->cmd[0] = ALLOW_MEDIUM_REMOVAL;
@@ -2016,6 +2016,7 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
 	rq->retries = 5;

 	blk_execute_rq_nowait(req->q, NULL, req, 1, eh_lock_door_done);
+	return 0;
 }

 /**
@@ -2037,8 +2038,8 @@ static void scsi_restart_operations(struct Scsi_Host *shost)
 	 * is no point trying to lock the door of an off-line device.
 	 */
 	shost_for_each_device(sdev, shost) {
-		if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) {
-			scsi_eh_lock_door(sdev);
+		if (scsi_device_online(sdev) && sdev->was_reset &&
+		    sdev->locked && scsi_eh_lock_door(sdev) == 0) {
 			sdev->was_reset = 0;
 		}
 	}

Thanks,

Bart.

Re: [PATCH v4 5/9] scsi: Do not wait for a request in scsi_eh_lock_door()

[Hannes Reinecke]

On 12/3/20 6:10 AM, Bart Van Assche wrote:
> On 12/1/20 11:06 PM, Hannes Reinecke wrote:
>> On 11/30/20 3:46 AM, Bart Van Assche wrote:
>>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>>> index d94449188270..6de6e1bf3dcb 100644
>>> --- a/drivers/scsi/scsi_error.c
>>> +++ b/drivers/scsi/scsi_error.c
>>> @@ -1993,7 +1993,12 @@ 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);
>>> +    /*
>>> +     * It is not guaranteed that a request is available nor that
>>> +     * sdev->request_queue is unfrozen. Hence the BLK_MQ_REQ_NOWAIT
>>> below.
>>> +     */
>>> +    req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN,
>>> +                  BLK_MQ_REQ_NOWAIT);
>>>        if (IS_ERR(req))
>>>            return;
>>>        rq = scsi_req(req);
>>>
>>
>> Well ... had been thinking about that one, too.
>> The idea of this function is that prior to SCSI EH the device was locked
>> via scsi_set_medium_removal(). And during SCSI EH the device might have
>> become unlocked, so we need to lock it again.
>> However, scsi_set_medium_removal() not only issues the
>> PREVENT_ALLOW_MEDIUM_REMOVAL command, but also sets the 'locked' flag
>> based on the result.
>> So if we fail to get a request here, shouldn't we unset the 'locked'
>> flag, too?
> 
> Probably not. My interpretation of the 'locked' flag is that it
> represents the door state before error handling began. The following
> code in the SCSI error handler restores the door state after a bus reset:
> 
> 	if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) {
> 		scsi_eh_lock_door(sdev);
> 		sdev->was_reset = 0;
> 	}
> 
>> And what does happen if we fail here? There is no return value, hence
>> SCSI EH might run to completion, and the system will continue
>> with an unlocked door ...
>> Not sure if that's a good idea.
> 
> How about applying the following patch on top of patch 5/9?
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 6de6e1bf3dcb..feac7262e40e 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1988,7 +1988,7 @@ static void eh_lock_door_done(struct request *req, blk_status_t status)
>    * 	We queue up an asynchronous "ALLOW MEDIUM REMOVAL" request on the
>    * 	head of the devices request queue, and continue.
>    */
> -static void scsi_eh_lock_door(struct scsi_device *sdev)
> +static int scsi_eh_lock_door(struct scsi_device *sdev)
>   {
>   	struct request *req;
>   	struct scsi_request *rq;
> @@ -2000,7 +2000,7 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
>   	req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN,
>   			      BLK_MQ_REQ_NOWAIT);
>   	if (IS_ERR(req))
> -		return;
> +		return PTR_ERR(req);
>   	rq = scsi_req(req);
> 
>   	rq->cmd[0] = ALLOW_MEDIUM_REMOVAL;
> @@ -2016,6 +2016,7 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
>   	rq->retries = 5;
> 
>   	blk_execute_rq_nowait(req->q, NULL, req, 1, eh_lock_door_done);
> +	return 0;
>   }
> 
>   /**
> @@ -2037,8 +2038,8 @@ static void scsi_restart_operations(struct Scsi_Host *shost)
>   	 * is no point trying to lock the door of an off-line device.
>   	 */
>   	shost_for_each_device(sdev, shost) {
> -		if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) {
> -			scsi_eh_lock_door(sdev);
> +		if (scsi_device_online(sdev) && sdev->was_reset &&
> +		    sdev->locked && scsi_eh_lock_door(sdev) == 0) {
>   			sdev->was_reset = 0;
>   		}
>   	}
> 
I probably didn't make myself clear.
As per SBC (in this case, sbc3r36) the effects of 
PREVENT_ALLOW_MEDIUM_REMOVAL are being reset by a successfull LUN Reset, 
Hard Reset, Power/On Reset, or an I_T Nexus loss. Which incidentally 
maps nicely onto SCSI EH, so after a successful SCSI EH the door will be 
unlocked (which is why we need to call scsi_eh_lock_door()).
In the SCSI midlayer this state is being reflected by the 'locked' flag.
Now, if scsi_eh_lock_door() is _not_ being executed due to a 
blk_get_request() failure, the device remains unlocked, and as such the 
'locked' flag would need to be _unset_.

So I was thinking more along these lines:

@@ -2030,7 +2037,8 @@ static void scsi_restart_operations(struct 
Scsi_Host *shost)
          */
         shost_for_each_device(sdev, shost) {
                 if (scsi_device_online(sdev) && sdev->was_reset && 
sdev->locked) {
-                       scsi_eh_lock_door(sdev);
+                       if (scsi_eh_lock_door(sdev) < 0)
+                               sdev->locked = 0;
                         sdev->was_reset = 0;
                 }
         }


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

Re: [PATCH v4 5/9] scsi: Do not wait for a request in scsi_eh_lock_door()

[Ming Lei]

On Thu, Dec 03, 2020 at 08:18:57AM +0100, Hannes Reinecke wrote:
> On 12/3/20 6:10 AM, Bart Van Assche wrote:
> > On 12/1/20 11:06 PM, Hannes Reinecke wrote:
> > > On 11/30/20 3:46 AM, Bart Van Assche wrote:
> > > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > > > index d94449188270..6de6e1bf3dcb 100644
> > > > --- a/drivers/scsi/scsi_error.c
> > > > +++ b/drivers/scsi/scsi_error.c
> > > > @@ -1993,7 +1993,12 @@ 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);
> > > > +    /*
> > > > +     * It is not guaranteed that a request is available nor that
> > > > +     * sdev->request_queue is unfrozen. Hence the BLK_MQ_REQ_NOWAIT
> > > > below.
> > > > +     */
> > > > +    req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN,
> > > > +                  BLK_MQ_REQ_NOWAIT);
> > > >        if (IS_ERR(req))
> > > >            return;
> > > >        rq = scsi_req(req);
> > > > 
> > > 
> > > Well ... had been thinking about that one, too.
> > > The idea of this function is that prior to SCSI EH the device was locked
> > > via scsi_set_medium_removal(). And during SCSI EH the device might have
> > > become unlocked, so we need to lock it again.
> > > However, scsi_set_medium_removal() not only issues the
> > > PREVENT_ALLOW_MEDIUM_REMOVAL command, but also sets the 'locked' flag
> > > based on the result.
> > > So if we fail to get a request here, shouldn't we unset the 'locked'
> > > flag, too?
> > 
> > Probably not. My interpretation of the 'locked' flag is that it
> > represents the door state before error handling began. The following
> > code in the SCSI error handler restores the door state after a bus reset:
> > 
> > 	if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) {
> > 		scsi_eh_lock_door(sdev);
> > 		sdev->was_reset = 0;
> > 	}
> > 
> > > And what does happen if we fail here? There is no return value, hence
> > > SCSI EH might run to completion, and the system will continue
> > > with an unlocked door ...
> > > Not sure if that's a good idea.
> > 
> > How about applying the following patch on top of patch 5/9?
> > 
> > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > index 6de6e1bf3dcb..feac7262e40e 100644
> > --- a/drivers/scsi/scsi_error.c
> > +++ b/drivers/scsi/scsi_error.c
> > @@ -1988,7 +1988,7 @@ static void eh_lock_door_done(struct request *req, blk_status_t status)
> >    * 	We queue up an asynchronous "ALLOW MEDIUM REMOVAL" request on the
> >    * 	head of the devices request queue, and continue.
> >    */
> > -static void scsi_eh_lock_door(struct scsi_device *sdev)
> > +static int scsi_eh_lock_door(struct scsi_device *sdev)
> >   {
> >   	struct request *req;
> >   	struct scsi_request *rq;
> > @@ -2000,7 +2000,7 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
> >   	req = blk_get_request(sdev->request_queue, REQ_OP_SCSI_IN,
> >   			      BLK_MQ_REQ_NOWAIT);
> >   	if (IS_ERR(req))
> > -		return;
> > +		return PTR_ERR(req);
> >   	rq = scsi_req(req);
> > 
> >   	rq->cmd[0] = ALLOW_MEDIUM_REMOVAL;
> > @@ -2016,6 +2016,7 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
> >   	rq->retries = 5;
> > 
> >   	blk_execute_rq_nowait(req->q, NULL, req, 1, eh_lock_door_done);
> > +	return 0;
> >   }
> > 
> >   /**
> > @@ -2037,8 +2038,8 @@ static void scsi_restart_operations(struct Scsi_Host *shost)
> >   	 * is no point trying to lock the door of an off-line device.
> >   	 */
> >   	shost_for_each_device(sdev, shost) {
> > -		if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) {
> > -			scsi_eh_lock_door(sdev);
> > +		if (scsi_device_online(sdev) && sdev->was_reset &&
> > +		    sdev->locked && scsi_eh_lock_door(sdev) == 0) {
> >   			sdev->was_reset = 0;
> >   		}
> >   	}
> > 
> I probably didn't make myself clear.
> As per SBC (in this case, sbc3r36) the effects of
> PREVENT_ALLOW_MEDIUM_REMOVAL are being reset by a successfull LUN Reset,
> Hard Reset, Power/On Reset, or an I_T Nexus loss. Which incidentally maps
> nicely onto SCSI EH, so after a successful SCSI EH the door will be unlocked
> (which is why we need to call scsi_eh_lock_door()).
> In the SCSI midlayer this state is being reflected by the 'locked' flag.
> Now, if scsi_eh_lock_door() is _not_ being executed due to a
> blk_get_request() failure, the device remains unlocked, and as such the
> 'locked' flag would need to be _unset_.
> 
> So I was thinking more along these lines:
> 
> @@ -2030,7 +2037,8 @@ static void scsi_restart_operations(struct Scsi_Host
> *shost)
>          */
>         shost_for_each_device(sdev, shost) {
>                 if (scsi_device_online(sdev) && sdev->was_reset &&
> sdev->locked) {
> -                       scsi_eh_lock_door(sdev);
> +                       if (scsi_eh_lock_door(sdev) < 0)
> +                               sdev->locked = 0;

BTW, scsi_eh_lock_door() returns void, and it can't be sync because
there may not be any driver tag available. Even though it is available,
the host state isn't running yet, so the command can't be queued to LLD
yet.

Maybe the above lines should be put after host state is updated to
RUNNING.

Also changing to NOWAIT can't avoid the issue completely, what if 'none'
is used?


Thanks,
Ming

Re: [PATCH v4 5/9] scsi: Do not wait for a request in scsi_eh_lock_door()

[Bart Van Assche]

On 12/2/20 11:27 PM, Ming Lei wrote:
> BTW, scsi_eh_lock_door() returns void, and it can't be sync because
> there may not be any driver tag available. Even though it is available,
> the host state isn't running yet, so the command can't be queued to LLD
> yet.
> 
> Maybe the above lines should be put after host state is updated to
> RUNNING.
> 
> Also changing to NOWAIT can't avoid the issue completely, what if 'none'
> is used?

Hi Ming,

I am considering to drop this patch since the latest version of the SPI
DV patch no longer introduces a new blk_mq_freeze_queue() call in the
SPI DV code. In other words, any potential issues with
scsi_eh_lock_door() are existing issues and are not made worse by my
patch series.

Thanks,

Bart.

Re: [PATCH v4 0/9] Rework runtime suspend and SPI domain validation

[Jens Axboe]

On 12/1/20 6:51 PM, Martin K. Petersen wrote:
> 
> Jens,
> 
> Any objections to me picking this up?

No, go ahead. Looks good to me, you can add:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

to the series.

-- 
Jens Axboe

Re: [PATCH v4 0/9] Rework runtime suspend and SPI domain validation

[Martin K. Petersen]

Bart,

> The SCSI runtime suspend and SPI 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.

Applied to 5.11/scsi-staging except for patch #5. Appreciate all that
work that went into this series!

-- 
Martin K. Petersen	Oracle Linux Engineering