| Message ID | 1592635992-35619-1-git-send-email-kwmad.kim@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v1,1/2] ufs: introduce callbacks to get command information | expand |
On 2020-06-19 23:53, Kiwoong Kim wrote: > Some SoC specific might need command history for > various reasons, such as stacking command contexts > in system memory to check for debugging in the future > or scaling some DVFS knobs to boost IO throughput. > > What you would do with the information could be > variant per SoC vendor. Isn't this something that should be done in an I/O scheduler instead of in a SCSI LLD? I don't like the idea behind this patch series at all. Bart.
> On 2020-06-19 23:53, Kiwoong Kim wrote: > > Some SoC specific might need command history for various reasons, such > > as stacking command contexts in system memory to check for debugging > > in the future or scaling some DVFS knobs to boost IO throughput. > > > > What you would do with the information could be variant per SoC > > vendor. > > Isn't this something that should be done in an I/O scheduler instead of in > a SCSI LLD? I don't like the idea behind this patch series at all. > > Bart. If you could get the information, Many would exploit it for their respective purposes. But, it's important for the information to contain accurate timestamps when the driver hooks it, if you're trying to figure out something wrong. As for scaling DVFS knobs to boost UFS throughput, locating in the ufs driver is more Beneficial because SoC vendors have their own power domains and thus lead to make different way of what to scale up to boost. If it's populated in block layer, there is no way to introduce boosting per SoC.
On 2020-06-22 19:28, Kiwoong Kim wrote: > If you could get the information, Many would exploit it for their respective purposes. > But, it's important for the information to contain accurate timestamps when the driver > hooks it, if you're trying to figure out something wrong. > > As for scaling DVFS knobs to boost UFS throughput, locating in the ufs driver is more > Beneficial because SoC vendors have their own power domains and thus lead to make > different way of what to scale up to boost. If it's populated in block layer, there is > no way to introduce boosting per SoC. Thanks for the clarification. Unfortunately we do not yet have an API for sharing information between I/O schedulers and block drivers ... Bart.
Hi Kiwoong, Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月20日 週六 下午3:00寫道： > > Some SoC specific might need command history for > various reasons, such as stacking command contexts > in system memory to check for debugging in the future > or scaling some DVFS knobs to boost IO throughput. > > What you would do with the information could be > variant per SoC vendor. > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > --- > drivers/scsi/ufs/ufshcd.c | 4 ++++ > drivers/scsi/ufs/ufshcd.h | 8 ++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 52abe82..0eae3ce 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) > /* issue command to the controller */ > spin_lock_irqsave(hba->host->host_lock, flags); > ufshcd_vops_setup_xfer_req(hba, tag, true); > + if (cmd) > + ufshcd_vops_cmd_log(hba, cmd, 1); > ufshcd_send_command(hba, tag); > out_unlock: > spin_unlock_irqrestore(hba->host->host_lock, flags); > @@ -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba, > /* Mark completed command as NULL in LRB */ > lrbp->cmd = NULL; > lrbp->compl_time_stamp = ktime_get(); > + ufshcd_vops_cmd_log(hba, cmd, 2); > + > /* Do not touch lrbp after scsi done */ > cmd->scsi_done(cmd); > __ufshcd_release(hba); If your cmd_log vop callbacks are only existed in "ufshcd_queuecommand" and "ufshcd_transfer_req_compl", perhaps you could re-use "ufshcd_vops_setup_xfer_req()" and an added "ufshcd_vops_compl_req()" instead of a brand new "ufshcd_vops_cmd_log()" ? Thanks, Stanley Chu
> Hi Kiwoong, > > Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月20日 週六 下午3:00寫道： > > > > Some SoC specific might need command history for various reasons, such > > as stacking command contexts in system memory to check for debugging > > in the future or scaling some DVFS knobs to boost IO throughput. > > > > What you would do with the information could be variant per SoC > > vendor. > > > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > > --- > > drivers/scsi/ufs/ufshcd.c | 4 ++++ > > drivers/scsi/ufs/ufshcd.h | 8 ++++++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > index 52abe82..0eae3ce 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct Scsi_Host > *host, struct scsi_cmnd *cmd) > > /* issue command to the controller */ > > spin_lock_irqsave(hba->host->host_lock, flags); > > ufshcd_vops_setup_xfer_req(hba, tag, true); > > + if (cmd) > > + ufshcd_vops_cmd_log(hba, cmd, 1); > > ufshcd_send_command(hba, tag); > > out_unlock: > > spin_unlock_irqrestore(hba->host->host_lock, flags); @@ > > -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct > ufs_hba *hba, > > /* Mark completed command as NULL in LRB */ > > lrbp->cmd = NULL; > > lrbp->compl_time_stamp = ktime_get(); > > + ufshcd_vops_cmd_log(hba, cmd, 2); > > + > > /* Do not touch lrbp after scsi done */ > > cmd->scsi_done(cmd); > > __ufshcd_release(hba); > > If your cmd_log vop callbacks are only existed in "ufshcd_queuecommand" > and "ufshcd_transfer_req_compl", perhaps you could re-use > "ufshcd_vops_setup_xfer_req()" and an added "ufshcd_vops_compl_req()" > instead of a brand new "ufshcd_vops_cmd_log()" ? > > Thanks, > Stanley Chu Currently, ufshcd_vops_setup_xfer_req doesn't get scsi_cmnd variable. Actually, when introduced this callback first, I was willing to make it do that but someone gave me another idea. Then do you agree to change argument set of the function? And I can't find ufshcd_vops_compl_req in 5.9/scsi-queue. Could you let me know where to find? Thank you for your opinions.
Hi Kiwoong, Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月26日 週五 下午7:42寫道： > > > Hi Kiwoong, > > > > Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月20日 週六 下午3:00寫道： > > > > > > Some SoC specific might need command history for various reasons, such > > > as stacking command contexts in system memory to check for debugging > > > in the future or scaling some DVFS knobs to boost IO throughput. > > > > > > What you would do with the information could be variant per SoC > > > vendor. > > > > > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > > > --- > > > drivers/scsi/ufs/ufshcd.c | 4 ++++ > > > drivers/scsi/ufs/ufshcd.h | 8 ++++++++ > > > 2 files changed, 12 insertions(+) > > > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > > index 52abe82..0eae3ce 100644 > > > --- a/drivers/scsi/ufs/ufshcd.c > > > +++ b/drivers/scsi/ufs/ufshcd.c > > > @@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct Scsi_Host > > *host, struct scsi_cmnd *cmd) > > > /* issue command to the controller */ > > > spin_lock_irqsave(hba->host->host_lock, flags); > > > ufshcd_vops_setup_xfer_req(hba, tag, true); > > > + if (cmd) > > > + ufshcd_vops_cmd_log(hba, cmd, 1); > > > ufshcd_send_command(hba, tag); > > > out_unlock: > > > spin_unlock_irqrestore(hba->host->host_lock, flags); @@ > > > -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct > > ufs_hba *hba, > > > /* Mark completed command as NULL in LRB */ > > > lrbp->cmd = NULL; > > > lrbp->compl_time_stamp = ktime_get(); > > > + ufshcd_vops_cmd_log(hba, cmd, 2); > > > + > > > /* Do not touch lrbp after scsi done */ > > > cmd->scsi_done(cmd); > > > __ufshcd_release(hba); > > > > If your cmd_log vop callbacks are only existed in "ufshcd_queuecommand" > > and "ufshcd_transfer_req_compl", perhaps you could re-use > > "ufshcd_vops_setup_xfer_req()" and an added "ufshcd_vops_compl_req()" > > instead of a brand new "ufshcd_vops_cmd_log()" ? > > > > Thanks, > > Stanley Chu > > Currently, ufshcd_vops_setup_xfer_req doesn't get scsi_cmnd variable. You could get scsi_cmnd by hba->lrb[tag].cmd if is_scsi_cmd is true in your ufshcd_vops_setup_xfer_req vendor implementation. > Actually, when introduced this callback first, I was willing to make it do that > but someone gave me another idea. Then do you agree to change argument set of the function? > > And I can't find ufshcd_vops_compl_req in 5.9/scsi-queue. Could you let me know where to find? > Sorry to not describing clearly. What I mean is that you could "add" ufshcd_vops_compl_xfer_req vop callback in your patch. Thanks, Stanley Chu
> Hi Kiwoong, > > Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月26日 週五 下午7:42寫道： > > > > > Hi Kiwoong, > > > > > > Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月20日 週六 下午3:00寫道： > > > > > > > > Some SoC specific might need command history for various reasons, > such > > > > as stacking command contexts in system memory to check for debugging > > > > in the future or scaling some DVFS knobs to boost IO throughput. > > > > > > > > What you would do with the information could be variant per SoC > > > > vendor. > > > > > > > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > > > > --- > > > > drivers/scsi/ufs/ufshcd.c | 4 ++++ > > > > drivers/scsi/ufs/ufshcd.h | 8 ++++++++ > > > > 2 files changed, 12 insertions(+) > > > > > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > > > index 52abe82..0eae3ce 100644 > > > > --- a/drivers/scsi/ufs/ufshcd.c > > > > +++ b/drivers/scsi/ufs/ufshcd.c > > > > @@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct > Scsi_Host > > > *host, struct scsi_cmnd *cmd) > > > > /* issue command to the controller */ > > > > spin_lock_irqsave(hba->host->host_lock, flags); > > > > ufshcd_vops_setup_xfer_req(hba, tag, true); > > > > + if (cmd) > > > > + ufshcd_vops_cmd_log(hba, cmd, 1); > > > > ufshcd_send_command(hba, tag); > > > > out_unlock: > > > > spin_unlock_irqrestore(hba->host->host_lock, flags); @@ > > > > -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct > > > ufs_hba *hba, > > > > /* Mark completed command as NULL in LRB */ > > > > lrbp->cmd = NULL; > > > > lrbp->compl_time_stamp = ktime_get(); > > > > + ufshcd_vops_cmd_log(hba, cmd, 2); > > > > + > > > > /* Do not touch lrbp after scsi done */ > > > > cmd->scsi_done(cmd); > > > > __ufshcd_release(hba); > > > > > > If your cmd_log vop callbacks are only existed in > "ufshcd_queuecommand" > > > and "ufshcd_transfer_req_compl", perhaps you could re-use > > > "ufshcd_vops_setup_xfer_req()" and an added "ufshcd_vops_compl_req()" > > > instead of a brand new "ufshcd_vops_cmd_log()" ? > > > > > > Thanks, > > > Stanley Chu > > > > Currently, ufshcd_vops_setup_xfer_req doesn't get scsi_cmnd variable. > > You could get scsi_cmnd by hba->lrb[tag].cmd if is_scsi_cmd is true in > your ufshcd_vops_setup_xfer_req vendor implementation. > > > Actually, when introduced this callback first, I was willing to make it > do that > > but someone gave me another idea. Then do you agree to change argument > set of the function? > > > > And I can't find ufshcd_vops_compl_req in 5.9/scsi-queue. Could you let > me know where to find? > > > > Sorry to not describing clearly. What I mean is that you could "add" > ufshcd_vops_compl_xfer_req vop callback in your patch. > > Thanks, > Stanley Chu Got it and I'll refer to what you said. Thanks. Kiwoong Kim
> Hi Kiwoong, > > Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月26日 週五 下午7:42寫道： > > > > > Hi Kiwoong, > > > > > > Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月20日 週六 下午3:00寫道： > > > > > > > > Some SoC specific might need command history for various reasons, > such > > > > as stacking command contexts in system memory to check for debugging > > > > in the future or scaling some DVFS knobs to boost IO throughput. > > > > > > > > What you would do with the information could be variant per SoC > > > > vendor. > > > > > > > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > > > > --- > > > > drivers/scsi/ufs/ufshcd.c | 4 ++++ > > > > drivers/scsi/ufs/ufshcd.h | 8 ++++++++ > > > > 2 files changed, 12 insertions(+) > > > > > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > > > index 52abe82..0eae3ce 100644 > > > > --- a/drivers/scsi/ufs/ufshcd.c > > > > +++ b/drivers/scsi/ufs/ufshcd.c > > > > @@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct > Scsi_Host > > > *host, struct scsi_cmnd *cmd) > > > > /* issue command to the controller */ > > > > spin_lock_irqsave(hba->host->host_lock, flags); > > > > ufshcd_vops_setup_xfer_req(hba, tag, true); > > > > + if (cmd) > > > > + ufshcd_vops_cmd_log(hba, cmd, 1); > > > > ufshcd_send_command(hba, tag); > > > > out_unlock: > > > > spin_unlock_irqrestore(hba->host->host_lock, flags); @@ > > > > -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct > > > ufs_hba *hba, > > > > /* Mark completed command as NULL in LRB */ > > > > lrbp->cmd = NULL; > > > > lrbp->compl_time_stamp = ktime_get(); > > > > + ufshcd_vops_cmd_log(hba, cmd, 2); > > > > + > > > > /* Do not touch lrbp after scsi done */ > > > > cmd->scsi_done(cmd); > > > > __ufshcd_release(hba); > > > > > > If your cmd_log vop callbacks are only existed in > "ufshcd_queuecommand" > > > and "ufshcd_transfer_req_compl", perhaps you could re-use > > > "ufshcd_vops_setup_xfer_req()" and an added "ufshcd_vops_compl_req()" > > > instead of a brand new "ufshcd_vops_cmd_log()" ? > > > > > > Thanks, > > > Stanley Chu > > > > Currently, ufshcd_vops_setup_xfer_req doesn't get scsi_cmnd variable. > > You could get scsi_cmnd by hba->lrb[tag].cmd if is_scsi_cmd is true in > your ufshcd_vops_setup_xfer_req vendor implementation. > > > Actually, when introduced this callback first, I was willing to make it > do that > > but someone gave me another idea. Then do you agree to change argument > set of the function? > > > > And I can't find ufshcd_vops_compl_req in 5.9/scsi-queue. Could you let > me know where to find? > > > > Sorry to not describing clearly. What I mean is that you could "add" > ufshcd_vops_compl_xfer_req vop callback in your patch. > > Thanks, > Stanley Chu Got it and I'll refer to what you said. Thanks. Kiwoong Kim
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 52abe82..0eae3ce 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) /* issue command to the controller */ spin_lock_irqsave(hba->host->host_lock, flags); ufshcd_vops_setup_xfer_req(hba, tag, true); + if (cmd) + ufshcd_vops_cmd_log(hba, cmd, 1); ufshcd_send_command(hba, tag); out_unlock: spin_unlock_irqrestore(hba->host->host_lock, flags); @@ -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba, /* Mark completed command as NULL in LRB */ lrbp->cmd = NULL; lrbp->compl_time_stamp = ktime_get(); + ufshcd_vops_cmd_log(hba, cmd, 2); + /* Do not touch lrbp after scsi done */ cmd->scsi_done(cmd); __ufshcd_release(hba); diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index c774012..80c4f0d 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -307,6 +307,7 @@ struct ufs_hba_variant_ops { void (*config_scaling_param)(struct ufs_hba *hba, struct devfreq_dev_profile *profile, void *data); + void (*cmd_log)(struct ufs_hba *hba, struct scsi_cmnd *cmd, int enter); }; /* clock gating state */ @@ -1137,6 +1138,13 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba, hba->vops->config_scaling_param(hba, profile, data); } +static inline void ufshcd_vops_cmd_log(struct ufs_hba *hba, + struct scsi_cmnd *cmd, int enter) +{ + if (hba->vops && hba->vops->cmd_log) + hba->vops->cmd_log(hba, cmd, enter); +} + extern struct ufs_pm_lvl_states ufs_pm_lvl_states[]; /*
Some SoC specific might need command history for various reasons, such as stacking command contexts in system memory to check for debugging in the future or scaling some DVFS knobs to boost IO throughput. What you would do with the information could be variant per SoC vendor. Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> --- drivers/scsi/ufs/ufshcd.c | 4 ++++ drivers/scsi/ufs/ufshcd.h | 8 ++++++++ 2 files changed, 12 insertions(+)