| Message ID | 20191105004226.232635-3-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Simplify and optimize the UFS driver | expand |
On Mon, Nov 04, 2019 at 04:42:23PM -0800, Bart Van Assche wrote: > Reserved tags are numerically lower than non-reserved tags. Compensate the > change caused by reserving tags by subtracting the number of reserved tags > from the tag number assigned by the block layer. Why would you do that? Do we really care about the exact tag number? If so would it make sense to reverse in the block layer how we allocate private vs normal tags? Also this change should probably merged into the patch that actually starts using the private tags by actually allocating requests using them.
On 11/4/19 4:57 PM, Christoph Hellwig wrote: > On Mon, Nov 04, 2019 at 04:42:23PM -0800, Bart Van Assche wrote: >> Reserved tags are numerically lower than non-reserved tags. Compensate the >> change caused by reserving tags by subtracting the number of reserved tags >> from the tag number assigned by the block layer. > > Why would you do that? Do we really care about the exact tag number? > If so would it make sense to reverse in the block layer how we allocate > private vs normal tags? > > Also this change should probably merged into the patch that actually > starts using the private tags by actually allocating requests using > them. Hi Christoph, The UFS driver writes the actual tags into doorbell registers. There are two such doorbell registers: one for regular commands and one for task management functions. Both doorbell registers are bitmasks that start from bit zero. So I don't see how to avoid this kind of tag conversions? From the UFS driver, for regular commands: ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL); And for TMFs: ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL); Thanks, Bart.
Hi, Bart > > Reserved tags are numerically lower than non-reserved tags. Compensate the > change caused by reserving tags by subtracting the number of reserved tags > from the tag number assigned by the block layer. > > Cc: Yaniv Gardi <ygardi@codeaurora.org> > Cc: Subhash Jadavani <subhashj@codeaurora.org> > Cc: Stanley Chu <stanley.chu@mediatek.com> > Cc: Avri Altman <avri.altman@wdc.com> > Cc: Tomas Winkler <tomas.winkler@intel.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/ufs/ufshcd.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index > 9fc05a535624..3e3c6257a343 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -2402,7 +2402,7 @@ static int ufshcd_queuecommand(struct Scsi_Host > *host, struct scsi_cmnd *cmd) > > hba = shost_priv(host); > > - tag = cmd->request->tag; > + tag = cmd->request->tag - hba->nutmrs; > if (!ufshcd_valid_tag(hba, tag)) { > dev_err(hba->dev, > "%s: invalid command tag %d: cmd=0x%p, cmd- > >request=0x%p", @@ -5965,7 +5965,8 @@ static int > ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd) > > host = cmd->device->host; > hba = shost_priv(host); > - tag = cmd->request->tag; > + tag = cmd->request->tag - hba->nutmrs; > + WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag)); Changing request tag number here is not proper way, we have trace tool using this tag to track request from block, SCSI to UFS layer. If tags being changed in UFS driver, there will be a confusion. > > lrbp = &hba->lrb[tag]; > err = ufshcd_issue_tm_cmd(hba, lrbp->lun, 0, UFS_LOGICAL_RESET, > &resp); @@ -6036,7 +6037,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) > > host = cmd->device->host; > hba = shost_priv(host); > - tag = cmd->request->tag; > + tag = cmd->request->tag - hba->nutmrs; > lrbp = &hba->lrb[tag]; > if (!ufshcd_valid_tag(hba, tag)) { > dev_err(hba->dev, > @@ -8320,7 +8321,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem > *mmio_base, unsigned int irq) > /* Configure LRB */ > ufshcd_host_memory_configure(hba); > > - host->can_queue = hba->nutrs; > + host->can_queue = hba->nutrs + hba->nutmrs; > + host->reserved_tags = hba->nutmrs; hba->nutmrs is for task management, relevant door-bell register is bit0-bit7 validate. If my understanding is correct, UFS task management requests normally issued by UFS layer, rather than upper layers. Before issuing task management request, UFS driver layer will apply a free slot, see ufshcd_get_tm_free_slot(). I don't think it is necessary to change host->can_queue size by adding task management depth hba->nutmrs. > host->cmd_per_lun = hba->nutrs; > host->max_id = UFSHCD_MAX_ID; > host->max_lun = UFS_MAX_LUNS; > -- > 2.24.0.rc1.363.gb1bccd3e3d-goog //Bean
On 11/5/19 3:58 AM, Bean Huo (beanhuo) wrote: >> host = cmd->device->host; >> hba = shost_priv(host); >> - tag = cmd->request->tag; >> + tag = cmd->request->tag - hba->nutmrs; >> + WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag)); > > Changing request tag number here is not proper way, we have trace tool using this tag to track request from block, > SCSI to UFS layer. If tags being changed in UFS driver, there will be a confusion. Hi Bean, Thanks for having taken a look. Which information is used by the tracing tool? cmd->request->tag or the variable called 'tag' above? The latter should not be modified by this patch. cmd->request->tag is modified however for every command that is not a TMF. Preserving the block layer tag value is possible but would require to introduce a new tag set for TMFs. Thanks, Bart.
> > On 11/5/19 3:58 AM, Bean Huo (beanhuo) wrote: > >> host = cmd->device->host; > >> hba = shost_priv(host); > >> - tag = cmd->request->tag; > >> + tag = cmd->request->tag - hba->nutmrs; > >> + WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag)); > > > > Changing request tag number here is not proper way, we have trace tool > > using this tag to track request from block, SCSI to UFS layer. If tags being > changed in UFS driver, there will be a confusion. > > Hi Bean, > > Thanks for having taken a look. Which information is used by the tracing tool? > cmd->request->tag or the variable called 'tag' above? The latter should not be > modified by this patch. cmd->request->tag is modified however for every > command that is not a TMF. Preserving the block layer tag value is possible but > would require to introduce a new tag set for TMFs. > > Thanks, > > Bart. Hi, Bart We are using latter variable 'tag'. What I concern is that cmd->request->tag and variable tag(UFS driver uses this variable tag) are consistent without this patch, and now we should change our tag tracking handling mechanism. But it is not big deal, we can plus hba->nutmrs subtracted before in order to in line with block/scsi tag. Thanks, //Bean
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 9fc05a535624..3e3c6257a343 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -2402,7 +2402,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) hba = shost_priv(host); - tag = cmd->request->tag; + tag = cmd->request->tag - hba->nutmrs; if (!ufshcd_valid_tag(hba, tag)) { dev_err(hba->dev, "%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p", @@ -5965,7 +5965,8 @@ static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd) host = cmd->device->host; hba = shost_priv(host); - tag = cmd->request->tag; + tag = cmd->request->tag - hba->nutmrs; + WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag)); lrbp = &hba->lrb[tag]; err = ufshcd_issue_tm_cmd(hba, lrbp->lun, 0, UFS_LOGICAL_RESET, &resp); @@ -6036,7 +6037,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) host = cmd->device->host; hba = shost_priv(host); - tag = cmd->request->tag; + tag = cmd->request->tag - hba->nutmrs; lrbp = &hba->lrb[tag]; if (!ufshcd_valid_tag(hba, tag)) { dev_err(hba->dev, @@ -8320,7 +8321,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) /* Configure LRB */ ufshcd_host_memory_configure(hba); - host->can_queue = hba->nutrs; + host->can_queue = hba->nutrs + hba->nutmrs; + host->reserved_tags = hba->nutmrs; host->cmd_per_lun = hba->nutrs; host->max_id = UFSHCD_MAX_ID; host->max_lun = UFS_MAX_LUNS;
Reserved tags are numerically lower than non-reserved tags. Compensate the change caused by reserving tags by subtracting the number of reserved tags from the tag number assigned by the block layer. Cc: Yaniv Gardi <ygardi@codeaurora.org> Cc: Subhash Jadavani <subhashj@codeaurora.org> Cc: Stanley Chu <stanley.chu@mediatek.com> Cc: Avri Altman <avri.altman@wdc.com> Cc: Tomas Winkler <tomas.winkler@intel.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/ufs/ufshcd.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)