diff mbox series

[v2,4/9] scsi: Rework scsi_mq_alloc_queue()

Message ID 20201116030459.13963-5-bvanassche@acm.org (mailing list archive)
State Superseded
Headers show
Series Rework runtime suspend and SCSI domain validation | expand

Commit Message

Bart Van Assche Nov. 16, 2020, 3:04 a.m. UTC
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>
Reviewed-by: 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(-)

Comments

Christoph Hellwig Nov. 16, 2020, 5:17 p.m. UTC | #1
On Sun, Nov 15, 2020 at 07:04:54PM -0800, Bart Van Assche wrote:
> 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);

This looks ok to me.  But is there any good to keep scsi_mq_alloc_queue
around at all?  It is so trivial that it can be open coded in the
currently only caller, as well as a new one if added.
Bart Van Assche Nov. 16, 2020, 6:01 p.m. UTC | #2
On 11/16/20 9:17 AM, Christoph Hellwig wrote:
> On Sun, Nov 15, 2020 at 07:04:54PM -0800, Bart Van Assche wrote:
>> 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);
> 
> This looks ok to me.  But is there any good to keep scsi_mq_alloc_queue
> around at all?  It is so trivial that it can be open coded in the
> currently only caller, as well as a new one if added.

Hi Christoph,

A later patch in this series introduces a second call to 
scsi_mq_alloc_queue(). Do we really want to have multiple functions that 
set QUEUE_FLAG_SCSI_PASSTHROUGH? I'm concerned that if the logic for 
creating a SCSI queue would ever be changed that only the copy in the 
SCSI core would be updated but not the copy in the SPI code.

Thanks,

Bart.
Can Guo Nov. 18, 2020, 1:15 a.m. UTC | #3
On 2020-11-16 11:04, Bart Van Assche wrote:
> 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>
> Reviewed-by: 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>

Reviewed-by: Can Guo <cang@codeaurora.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 e4f9ed355be6..ff480fa6261e 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1883,14 +1883,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)
Bart Van Assche Nov. 20, 2020, 1:36 a.m. UTC | #4
On 11/16/20 10:01 AM, Bart Van Assche wrote:
> On 11/16/20 9:17 AM, Christoph Hellwig wrote:
>> On Sun, Nov 15, 2020 at 07:04:54PM -0800, Bart Van Assche wrote:
>>> 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);
>>
>> This looks ok to me.  But is there any good to keep scsi_mq_alloc_queue
>> around at all?  It is so trivial that it can be open coded in the
>> currently only caller, as well as a new one if added.
> 
> Hi Christoph,
> 
> A later patch in this series introduces a second call to
> scsi_mq_alloc_queue(). Do we really want to have multiple functions that
> set QUEUE_FLAG_SCSI_PASSTHROUGH? I'm concerned that if the logic for
> creating a SCSI queue would ever be changed that only the copy in the
> SCSI core would be updated but not the copy in the SPI code.

(replying to my own email)

Hi Christoph,

Is this something that you feel strongly about? I can make this change
but that would require reaching out again to someone who owns an SPI
setup for testing this patch series since I do not own an SPI setup
myself ...

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e4f9ed355be6..ff480fa6261e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1883,14 +1883,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)