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