* [PATCH v3 0/1] scsi: ufs: fix LRB pointer incorrect initialization issue @ 2020-03-09 16:10 huobean 2020-03-09 16:10 ` [PATCH v3 1/1] " huobean 0 siblings, 1 reply; 5+ messages in thread From: huobean @ 2020-03-09 16:10 UTC (permalink / raw) To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel From: Bean Huo <beanhuo@micron.com> Hi, Martin and Bart Based on the Bart's suggestion, delete ufshcd_init_lrb(), and update the patch to v3. This version patch passed stress test. Thanks, //Bean Bean Huo (1): scsi: ufs: fix LRB pointer incorrect initialization issue drivers/scsi/ufs/ufshcd.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 1/1] scsi: ufs: fix LRB pointer incorrect initialization issue 2020-03-09 16:10 [PATCH v3 0/1] scsi: ufs: fix LRB pointer incorrect initialization issue huobean @ 2020-03-09 16:10 ` huobean 2020-03-10 1:58 ` Bart Van Assche 0 siblings, 1 reply; 5+ messages in thread From: huobean @ 2020-03-09 16:10 UTC (permalink / raw) To: alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, bvanassche, tomas.winkler, cang Cc: linux-scsi, linux-kernel From: Bean Huo <beanhuo@micron.com> There are two issues with the ufshcd_init_lrb() call in ufshcd_init_cmd_priv(): - cmd->tag is set from inside scsi_mq_prep_fn() and hence is not yet set when ufshcd_init_cmd_priv() is set. - Inside ufshcd_init_cmd_priv() the tag can only be derived from the SCSI command pointer if no scheduler has been associated with the UFS block device. If no scheduler is associated with a block device, the relationship between hctx->tags->static_rqs[] and rq->tag is static. If a scheduler has been configured, a single tag can be associated with a different struct request if a request is reallocated. Fixes: 34656dda81ac ("ufs: Let the SCSI core allocate per-command UFS data") v2-v3: - delete ufshcd_init_cmd_priv() - change commit message Signed-off-by: Bean Huo <beanhuo@micron.com> --- drivers/scsi/ufs/ufshcd.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index e987fa3a77c7..dfb44e8cc9dd 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -2471,6 +2471,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) BUG(); } + ufshcd_init_lrb(hba, lrbp, tag); + WARN_ON_ONCE(!ufshcd_is_scsi(cmd->request)); if (!down_read_trylock(&hba->clk_scaling_lock)) @@ -2707,6 +2709,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba, init_completion(&wait); lrbp = ufshcd_req_to_lrb(req); + ufshcd_init_lrb(hba, lrbp, tag); err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag); if (unlikely(err)) goto out_put_tag; @@ -3504,14 +3507,6 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba) } } -static int ufshcd_init_cmd_priv(struct Scsi_Host *shost, struct scsi_cmnd *cmd) -{ - struct ufs_hba *hba = shost_priv(shost); - - ufshcd_init_lrb(hba, scsi_cmd_priv(cmd), cmd->tag); - return 0; -} - /** * ufshcd_dme_link_startup - Notify Unipro to perform link startup * @hba: per adapter instance @@ -4834,6 +4829,7 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba, continue; cmd = blk_mq_rq_to_pdu(req); lrbp = scsi_cmd_priv(cmd); + ufshcd_init_lrb(hba, lrbp, index); if (ufshcd_is_scsi(req)) { ufshcd_add_command_trace(hba, req, "complete"); result = ufshcd_transfer_rsp_status(hba, lrbp); @@ -5900,6 +5896,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba, init_completion(&wait); lrbp = ufshcd_req_to_lrb(req); + ufshcd_init_lrb(hba, lrbp, tag); lrbp->sense_bufflen = 0; lrbp->sense_buffer = NULL; lrbp->task_tag = tag; @@ -7180,7 +7177,6 @@ static struct scsi_host_template ufshcd_driver_template = { .module = THIS_MODULE, .name = UFSHCD, .proc_name = UFSHCD, - .init_cmd_priv = ufshcd_init_cmd_priv, .queuecommand = ufshcd_queuecommand, .slave_alloc = ufshcd_slave_alloc, .slave_configure = ufshcd_slave_configure, -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/1] scsi: ufs: fix LRB pointer incorrect initialization issue 2020-03-09 16:10 ` [PATCH v3 1/1] " huobean @ 2020-03-10 1:58 ` Bart Van Assche 2020-03-10 7:53 ` [EXT] " Bean Huo (beanhuo) 0 siblings, 1 reply; 5+ messages in thread From: Bart Van Assche @ 2020-03-10 1:58 UTC (permalink / raw) To: huobean, alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, beanhuo, tomas.winkler, cang Cc: linux-scsi, linux-kernel On 2020-03-09 09:10, huobean@gmail.com wrote: > @@ -4834,6 +4829,7 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba, > continue; > cmd = blk_mq_rq_to_pdu(req); > lrbp = scsi_cmd_priv(cmd); > + ufshcd_init_lrb(hba, lrbp, index); > if (ufshcd_is_scsi(req)) { > ufshcd_add_command_trace(hba, req, "complete"); > result = ufshcd_transfer_rsp_status(hba, lrbp); This ufshcd_init_lrb() call looks incorrect to me. I think that ufshcd_init_lrb() should only be called before a request is submitted to the UFS controller and also that ufshcd_init_lrb() should not be called from the completion path. Thanks, Bart. ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [EXT] Re: [PATCH v3 1/1] scsi: ufs: fix LRB pointer incorrect initialization issue 2020-03-10 1:58 ` Bart Van Assche @ 2020-03-10 7:53 ` Bean Huo (beanhuo) 2020-03-11 3:49 ` Bart Van Assche 0 siblings, 1 reply; 5+ messages in thread From: Bean Huo (beanhuo) @ 2020-03-10 7:53 UTC (permalink / raw) To: Bart Van Assche, huobean, alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, tomas.winkler, cang Cc: linux-scsi, linux-kernel Hi, Bart > Subject: [EXT] Re: [PATCH v3 1/1] scsi: ufs: fix LRB pointer incorrect initialization > issue > > On 2020-03-09 09:10, huobean@gmail.com wrote: > > @@ -4834,6 +4829,7 @@ static void __ufshcd_transfer_req_compl(struct > ufs_hba *hba, > > continue; > > cmd = blk_mq_rq_to_pdu(req); > > lrbp = scsi_cmd_priv(cmd); > > + ufshcd_init_lrb(hba, lrbp, index); > > if (ufshcd_is_scsi(req)) { > > ufshcd_add_command_trace(hba, req, "complete"); > > result = ufshcd_transfer_rsp_status(hba, lrbp); > > This ufshcd_init_lrb() call looks incorrect to me. I think that > ufshcd_init_lrb() should only be called before a request is submitted to the UFS > controller and also that ufshcd_init_lrb() should not be called from the > completion path. > __ufshcd_transfer_req_compl() ufshcd_transfer_rsp_status() will access lrbp->ucd_rsp_ptr. Without calling ufshcd_init_lrb() here, there will be an error. //Bean > Thanks, > > Bart. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [EXT] Re: [PATCH v3 1/1] scsi: ufs: fix LRB pointer incorrect initialization issue 2020-03-10 7:53 ` [EXT] " Bean Huo (beanhuo) @ 2020-03-11 3:49 ` Bart Van Assche 0 siblings, 0 replies; 5+ messages in thread From: Bart Van Assche @ 2020-03-11 3:49 UTC (permalink / raw) To: Bean Huo (beanhuo), huobean, alim.akhtar, avri.altman, asutoshd, jejb, martin.petersen, stanley.chu, tomas.winkler, cang Cc: linux-scsi, linux-kernel On 2020-03-10 00:53, Bean Huo (beanhuo) wrote: > Hi, Bart > >> Subject: [EXT] Re: [PATCH v3 1/1] scsi: ufs: fix LRB pointer incorrect initialization >> issue >> >> On 2020-03-09 09:10, huobean@gmail.com wrote: >>> @@ -4834,6 +4829,7 @@ static void __ufshcd_transfer_req_compl(struct >> ufs_hba *hba, >>> continue; >>> cmd = blk_mq_rq_to_pdu(req); >>> lrbp = scsi_cmd_priv(cmd); >>> + ufshcd_init_lrb(hba, lrbp, index); >>> if (ufshcd_is_scsi(req)) { >>> ufshcd_add_command_trace(hba, req, "complete"); >>> result = ufshcd_transfer_rsp_status(hba, lrbp); >> >> This ufshcd_init_lrb() call looks incorrect to me. I think that >> ufshcd_init_lrb() should only be called before a request is submitted to the UFS >> controller and also that ufshcd_init_lrb() should not be called from the >> completion path. > > __ufshcd_transfer_req_compl() > ufshcd_transfer_rsp_status() will access lrbp->ucd_rsp_ptr. > Without calling ufshcd_init_lrb() here, there will be an error. Hi Bean, I think that ufshcd_init_lrb() should only be called from the code that prepares a command before it is submitted and not from the command completion path. Because v5.6-rc5 has already been released, there is not that much time left until the merge window opens. I think it is less risky to revert commit 34656dda81ac ("scsi: ufs: Let the SCSI core allocate per-command UFS data") than to proceed with the above patch. Do you want to submit a revert or do you perhaps want me to do that? Thanks, Bart. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-11 3:52 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-09 16:10 [PATCH v3 0/1] scsi: ufs: fix LRB pointer incorrect initialization issue huobean 2020-03-09 16:10 ` [PATCH v3 1/1] " huobean 2020-03-10 1:58 ` Bart Van Assche 2020-03-10 7:53 ` [EXT] " Bean Huo (beanhuo) 2020-03-11 3:49 ` Bart Van Assche
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.