On 2020-10-29 03:43, Jaegeuk Kim wrote:
On 10/27, Can Guo wrote:
On 2020-10-27 11:33, Jaegeuk Kim wrote:
> On 10/27, Can Guo wrote:
> > On 2020-10-27 03:51, 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.
> > >
> > > Note that, this requires a patch to address the device stuck by
> > > REQ_CLKS_OFF in
> > > __ufshcd_release().
> > >
> > > The fix is "scsi: ufs: avoid to call REQ_CLKS_OFF to CLKS_OFF".
> >
> > Why don't you just squash the fix into this one?
>
> I'm seeing this patch just revealed that problem.
That scenario (back to back calling of ufshcd_release()) only happens
when you stress the clkgate_enable sysfs node, so let's keep the fix
as one to make things simple. What do you think?
If we don't have this patch, do you think this issue won't happen at
all?
At least I've never seen this scenario these years, otherwise I would
have
put up a fix.
Thanks,
Can Guo.
Thanks,
Can Guo.
>
> >
> > 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 cc8d5f0c3fdc..6c9269bffcbd 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -1808,19 +1808,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)
> > > + __ufshcd_release(hba);
> > > + 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;
> > > }
