Re: [PATCH v2 2/2] scsi: ufs-qcom: enter and exit hibern8 during clock scaling

2019-10-23 Thread asutoshd 

On 2019-10-23 09:39, Asutosh Das wrote:

Qualcomm controller needs to be in hibern8 before scaling clocks.
This change puts the controller in hibern8 state before scaling
and brings it out after scaling of clocks.

Signed-off-by: Asutosh Das 
---
 drivers/scsi/ufs/ufs-qcom.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index a5b7148..d117088 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1305,6 +1305,9 @@ static int ufs_qcom_clk_scale_notify(struct 
ufs_hba *hba,

int err = 0;

if (status == PRE_CHANGE) {
+   err = ufshcd_uic_hibern8_enter(hba);
+   if (err)
+   return err;
if (scale_up)
err = ufs_qcom_clk_scale_up_pre_change(hba);

The error handling is not done here.


else
@@ -1324,6 +1327,7 @@ static int ufs_qcom_clk_scale_notify(struct 
ufs_hba *hba,

dev_req_params->hs_rate,
false);
ufs_qcom_update_bus_bw_vote(host);
+   ufshcd_uic_hibern8_exit(hba);
}

 out:


In the post-change condition as well, the error handling is not done.
On error, it should be brought out of hibernate.
I'll put up another change fixing that.

-asd

Re: [PATCH v3 2/5] scsi: ufs: clear UAC for FFU and RPMB LUNs

2020-10-26 Thread asutoshd 

On 2020-10-24 08:06, Jaegeuk Kim wrote:

From: Jaegeuk Kim 

In order to conduct FFU or RPMB operations, UFS needs to clear UAC. 
This patch
clears it explicitly, so that we could get no failure given early 
execution.




What's the meaning of 'given early execution'?


Signed-off-by: Jaegeuk Kim 
---
 drivers/scsi/ufs/ufshcd.c | 70 +++
 drivers/scsi/ufs/ufshcd.h |  1 +
 2 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e0b479f9eb8a..011e80a21170 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7057,7 +7057,6 @@ static inline void
ufshcd_blk_pm_runtime_init(struct scsi_device *sdev)
 static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
 {
int ret = 0;
-   struct scsi_device *sdev_rpmb;
struct scsi_device *sdev_boot;

hba->sdev_ufs_device = __scsi_add_device(hba->host, 0, 0,
@@ -7070,14 +7069,14 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba 
*hba)

ufshcd_blk_pm_runtime_init(hba->sdev_ufs_device);
scsi_device_put(hba->sdev_ufs_device);

-   sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
+   hba->sdev_rpmb = __scsi_add_device(hba->host, 0, 0,
ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN), NULL);
-   if (IS_ERR(sdev_rpmb)) {
-   ret = PTR_ERR(sdev_rpmb);
+   if (IS_ERR(hba->sdev_rpmb)) {
+   ret = PTR_ERR(hba->sdev_rpmb);
goto remove_sdev_ufs_device;
}
-   ufshcd_blk_pm_runtime_init(sdev_rpmb);
-   scsi_device_put(sdev_rpmb);
+   ufshcd_blk_pm_runtime_init(hba->sdev_rpmb);
+   scsi_device_put(hba->sdev_rpmb);

sdev_boot = __scsi_add_device(hba->host, 0, 0,
ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_BOOT_WLUN), NULL);
@@ -7601,6 +7600,63 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
return ret;
 }

+static int
+ufshcd_send_request_sense(struct ufs_hba *hba, struct scsi_device 
*sdp);

+
+static int ufshcd_clear_ua_wlun(struct ufs_hba *hba, u8 wlun)
+{
+   struct scsi_device *sdp;
+   unsigned long flags;
+   int ret = 0;
+
+   spin_lock_irqsave(hba->host->host_lock, flags);
+   if (wlun  == UFS_UPIU_UFS_DEVICE_WLUN)
+   sdp = hba->sdev_ufs_device;
+   else if (wlun  == UFS_UPIU_RPMB_WLUN)
+   sdp = hba->sdev_rpmb;
+   else
+   BUG_ON(1);
+   if (sdp) {
+   ret = scsi_device_get(sdp);
+   if (!ret && !scsi_device_online(sdp)) {
+   ret = -ENODEV;
+   scsi_device_put(sdp);
+   }
+   } else {
+   ret = -ENODEV;
+   }
+   spin_unlock_irqrestore(hba->host->host_lock, flags);
+   if (ret)
+   goto out_err;
+
+   ret = ufshcd_send_request_sense(hba, sdp);
+   scsi_device_put(sdp);
+out_err:
+   if (ret)
+   dev_err(hba->dev, "%s: UAC clear LU=%x ret = %d\n",
+   __func__, wlun, ret);
+   return ret;
+}
+
+static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
+{
+   int ret = 0;
+
+   if (!hba->wlun_dev_clr_ua)
+   goto out;
+
+   ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
+   if (!ret)
+   ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
+   if (!ret)
+   hba->wlun_dev_clr_ua = false;
+out:
+   if (ret)
+   dev_err(hba->dev, "%s: Failed to clear UAC WLUNS ret = %d\n",
+   __func__, ret);
+   return ret;
+}
+
 /**
  * ufshcd_probe_hba - probe hba to detect device and initialize
  * @hba: per-adapter instance
@@ -7720,6 +7776,8 @@ static void ufshcd_async_scan(void *data,
async_cookie_t cookie)
pm_runtime_put_sync(hba->dev);
ufshcd_exit_clk_scaling(hba);
ufshcd_hba_exit(hba);
+   } else {
+   ufshcd_clear_ua_wluns(hba);
}
 }

diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 47eb1430274c..718881d038f5 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -681,6 +681,7 @@ struct ufs_hba {
 * "UFS device" W-LU.
 */
struct scsi_device *sdev_ufs_device;
+   struct scsi_device *sdev_rpmb;

enum ufs_dev_pwr_mode curr_dev_pwr_mode;
enum uic_link_state uic_link_state;

Re: [PATCH v3 3/5] scsi: ufs: use WQ_HIGHPRI for gating work

2020-10-26 Thread asutoshd 

On 2020-10-24 08:06, Jaegeuk Kim wrote:

From: Jaegeuk Kim 

Must have WQ_MEM_RECLAIM
``WQ_MEM_RECLAIM``
  All wq which might be used in the memory reclaim paths **MUST**
  have this flag set.  The wq is guaranteed to have at least one
  execution context regardless of memory pressure.

Signed-off-by: Jaegeuk Kim 
---


Reviewed-by: Asutosh Das 


 drivers/scsi/ufs/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 011e80a21170..bc0d623aed66 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1867,7 +1867,7 @@ static void ufshcd_init_clk_gating(struct ufs_hba 
*hba)

snprintf(wq_name, ARRAY_SIZE(wq_name), "ufs_clk_gating_%d",
 hba->host->host_no);
hba->clk_gating.clk_gating_workq = alloc_ordered_workqueue(wq_name,
-  WQ_MEM_RECLAIM);
+   WQ_MEM_RECLAIM | WQ_HIGHPRI);

hba->clk_gating.is_enabled = true;

Re: [PATCH v3 5/5] scsi: ufs: fix clkgating on/off correctly

2020-10-26 Thread asutoshd 

On 2020-10-24 08:06, Jaegeuk Kim wrote:

The below call stack prevents clk_gating at every IO completion.
We can remove the condition, ufshcd_any_tag_in_use(), since 
clkgating_work

will check it again.

ufshcd_complete_requests(struct ufs_hba *hba)
  ufshcd_transfer_req_compl()
__ufshcd_transfer_req_compl()
  __ufshcd_release(hba)
if (ufshcd_any_tag_in_use() == 1)
   return;
  ufshcd_tmc_handler(hba);
blk_mq_tagset_busy_iter();

Signed-off-by: Jaegeuk Kim 
---


Reviewed-by: Asutosh Das 


 drivers/scsi/ufs/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c94610cbecae..38043c6b8d5f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1746,7 +1746,7 @@ static void __ufshcd_release(struct ufs_hba *hba)

if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
-   ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks ||
+   hba->outstanding_tasks ||
hba->active_uic_cmd || hba->uic_async_done)
return;

Re: [PATCH v2 5/5] scsi: ufs: fix clkgating on/off correctly

2020-10-26 Thread asutoshd 

On 2020-10-22 17:53, Jaegeuk Kim wrote:

On 10/21, Can Guo wrote:

On 2020-10-21 12:52, jaeg...@kernel.org wrote:
> On 10/21, Can Guo wrote:
> > On 2020-10-21 03:52, Jaegeuk Kim wrote:
> > > The below call stack prevents clk_gating at every IO completion.
> > > We can remove the condition, ufshcd_any_tag_in_use(), since
> > > clkgating_work
> > > will check it again.
> > >
> >
> > I think checking ufshcd_any_tag_in_use() in either ufshcd_release() or
> > gate_work() can break UFS clk gating's functionality.
> >
> > ufshcd_any_tag_in_use() was introduced to replace hba->lrb_in_use.
> > However,
> > they are not exactly same - ufshcd_any_tag_in_use() returns true if
> > any tag
> > assigned from block layer is still in use, but tags are released
> > asynchronously
> > (through block softirq), meaning it does not reflect the real
> > occupation of
> > UFS host.
> > That is after UFS host finishes all tasks, ufshcd_any_tag_in_use()
> > can still
> > return true.
> >
> > This change only removes the check of ufshcd_any_tag_in_use() in
> > ufshcd_release(),
> > but having the check of it in gate_work() can still prevent gating
> > from
> > happening.
> > The current change works for you maybe because the tags are release
> > before
> > hba->clk_gating.delay_ms expires, but if hba->clk_gating.delay_ms is
> > shorter
> > or
> > somehow block softirq is retarded, gate_work() may have chance to see
> > ufshcd_any_tag_in_use()
> > returns true. What do you think?
>
> I don't think this breaks clkgating, but fix the wrong condition check
> which
> prevented gate_work at all. As you mentioned, even if this schedules
> gate_work
> by racy conditions, gate_work will handle it as a last resort.
>

If clocks cannot be gated after the last task is cleared from UFS 
host, then

clk gating
is broken, no? Assume UFS has completed the last task in its queue, as 
this

change says,
ufshcd_any_tag_in_use() is preventing ufshcd_release() from invoking
gate_work().
Similarly, ufshcd_any_tag_in_use() can prevent gate_work() from doing 
its

real work -
disabling the clocks. Do you agree?

if (hba->clk_gating.active_reqs
|| hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL
|| ufshcd_any_tag_in_use(hba) || 
hba->outstanding_tasks

|| hba->active_uic_cmd || hba->uic_async_done)
goto rel_lock;


I see the point, but this happens only when clkgate_delay_ms is too 
short
to give enough time for releasing tag. If it's correctly set, I think 
there'd
be no problem, unless softirq was delayed by other RT threads which is 
just

a corner case tho.



Thanks,

Can Guo.

> >
> > Thanks,
> >
> > Can Guo.
> >
> > In __ufshcd_transfer_req_compl
> > Ihba->lrb_in_use is cleared immediately when UFS driver
> > finishes all tasks
> >
> > > ufshcd_complete_requests(struct ufs_hba *hba)
> > >   ufshcd_transfer_req_compl()
> > > __ufshcd_transfer_req_compl()
> > >   __ufshcd_release(hba)
> > > if (ufshcd_any_tag_in_use() == 1)
> > >return;
> > >   ufshcd_tmc_handler(hba);
> > > blk_mq_tagset_busy_iter();
> > >
> > > Cc: Alim Akhtar 
> > > Cc: Avri Altman 
> > > Cc: Can Guo 
> > > Signed-off-by: Jaegeuk Kim 
> > > ---
> > >  drivers/scsi/ufs/ufshcd.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > index b5ca0effe636..cecbd4ace8b4 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -1746,7 +1746,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
> > >
> > >  if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended 
||
> > >  hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
> > > -ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks ||
> > > +hba->outstanding_tasks ||
> > >  hba->active_uic_cmd || hba->uic_async_done)
> > >  return;


How about checking outstanding_reqs as well, say in 
ufshcd_any_tag_in_use() ?


-asd