Re: [PATCH v3 1/5] scsi: ufs: atomic update for clkgating_enable

[Jaegeuk Kim]

On 10/26, Can Guo wrote:
> On 2020-10-26 14:13, Jaegeuk Kim wrote:
> > On 10/26, Can Guo wrote:
> > > On 2020-10-24 23:06, Jaegeuk Kim wrote:
> > > > From: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> > > >
> > > > When giving a stress test which enables/disables clkgating, we hit
> > > > device
> > > > timeout sometimes. This patch avoids subtle racy condition to address
> > > > it.
> > > >
> > > > If we use __ufshcd_release(), I've seen that gate_work can be called in
> > > > parallel
> > > > with ungate_work, which results in UFS timeout when doing hibern8.
> > > > Should avoid it.
> > > >
> > > 
> > > I don't understand this comment. gate_work and ungate_work are
> > > queued on
> > > an ordered workqueue and an ordered workqueue executes at most one
> > > work item
> > > at any given time in the queued order. How can the two run in
> > > parallel?
> > 
> > When I hit UFS stuck, I saw this by clkgating tracepoint.
> > 
> > - REQ_CLK_OFF
> > - CLKS_OFF
> > - REQ_CLK_OFF
> > - REQ_CLKS_ON
> > ..
> > 
> 
> I don't see how can you tell that the two works are running in parallel
> just from above trace. May I know what is the exact error by "UFS timeout
> when doing hibern8"?
> 
> By using __ufshcd_release() here, I do see one potential issue if your test
> quickly toggles on/off of clk_gating - disable it, enable it, disable it and
> enable it, which will cause that __ufshcd_release() being called twice,
> meaning
> we queue two gate_works back to back. So can you try below code and let me
> know
> if it helps or not? I am OK with your current change, but I would like to
> understand the problem. Thanks.
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 1791bce..3eee438 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2271,6 +2271,8 @@ static void ufshcd_gate_work(struct work_struct *work)
>         unsigned long flags;
> 
>         spin_lock_irqsave(hba->host->host_lock, flags);
> +       if (hba->clk_gating.state == CLKS_OFF)
> +               goto rel_lock;
>         /*
>          * In case you are here to cancel this work the gating state
>          * would be marked as REQ_CLKS_ON. In this case save time by

This doesn't help. So, I checked this back again, and, like what you said, now
suspect __ufshcd_release() which changed state to REQ_CLKS_OFF on CLKS_OFF.

With the below change, I can see the issue anymore. Let me send v4.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b8f573a02713..cc8d5f0c3fdc 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1745,7 +1745,8 @@ 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->active_uic_cmd || hba->uic_async_done)
+	    hba->active_uic_cmd || hba->uic_async_done ||
+	    hba->clk_gating.state == CLKS_OFF)
 		return;
 
 	hba->clk_gating.state = REQ_CLKS_OFF;
-- 
2.29.0.rc1.297.gfa9743e501-goog


> 
> Regards,
> 
> Can Guo.
> 
> > By using active_req, I don't see any problem.
> > 
> > > 
> > > Thanks,
> > > 
> > > Can Guo.
> > > 
> > > > Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> > > > ---
> > > >  drivers/scsi/ufs/ufshcd.c | 12 ++++++------
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > > index b8f573a02713..e0b479f9eb8a 100644
> > > > --- a/drivers/scsi/ufs/ufshcd.c
> > > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > > @@ -1807,19 +1807,19 @@ static ssize_t
> > > > ufshcd_clkgate_enable_store(struct device *dev,
> > > >  		return -EINVAL;
> > > >
> > > >  	value = !!value;
> > > > +
> > > > +	spin_lock_irqsave(hba->host->host_lock, flags);
> > > >  	if (value == hba->clk_gating.is_enabled)
> > > >  		goto out;
> > > >
> > > > -	if (value) {
> > > > -		ufshcd_release(hba);
> > > > -	} else {
> > > > -		spin_lock_irqsave(hba->host->host_lock, flags);
> > > > +	if (value)
> > > > +		hba->clk_gating.active_reqs--;
> > > > +	else
> > > >  		hba->clk_gating.active_reqs++;
> > > > -		spin_unlock_irqrestore(hba->host->host_lock, flags);
> > > > -	}
> > > >
> > > >  	hba->clk_gating.is_enabled = value;
> > > >  out:
> > > > +	spin_unlock_irqrestore(hba->host->host_lock, flags);
> > > >  	return count;
> > > >  }