Re: [PATCH v4 3/8] ufs: ufs-qcom: Fix a few BUGs in func ufs_qcom_dump_dbg_regs()

[Can Guo]

On 2020-07-22 22:37, Jeffrey Hugo wrote:
> On Mon, Jul 20, 2020 at 12:39 AM Can Guo <cang@xxxxxxxxxxxxxx> wrote:
> > Dumping testbus registers needs to sleep a bit intermittently as there 
> > are
> > too many of them. Skip them for those contexts where sleep is not 
> > allowed.
> >
> > Meanwhile, if ufs_qcom_dump_dbg_regs() calls ufs_qcom_testbus_config() 
> > from
> > ufshcd_suspend/resume and/or clk gate/ungate context, 
> > pm_runtime_get_sync()
> > and ufshcd_hold() will cause racing problems. Fix it by removing the
> > unnecessary calls of pm_runtime_get_sync() and ufshcd_hold().
>
> It sounds like this is two different changes which are clubbed
> together into the same patch and really should be two different
> patches.

Will split them and give commit msgs accordingly in next version.

> > Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
> > ---
> >  drivers/scsi/ufs/ufs-qcom.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> > index 2e6ddb5..3743c17 100644
> > --- a/drivers/scsi/ufs/ufs-qcom.c
> > +++ b/drivers/scsi/ufs/ufs-qcom.c
> > @@ -1604,9 +1604,6 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host 
> > *host)
> >          */
> >         }
> >         mask <<= offset;
> > -
> > -       pm_runtime_get_sync(host->hba->dev);
> > -       ufshcd_hold(host->hba, false);
> >         ufshcd_rmwl(host->hba, TEST_BUS_SEL,
> >                     (u32)host->testbus.select_major << 19,
> >                     REG_UFS_CFG1);
> > @@ -1619,8 +1616,6 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host 
> > *host)
> >          * committed before returning.
> >          */
> >         mb();
> > -       ufshcd_release(host->hba);
> > -       pm_runtime_put_sync(host->hba->dev);
> >
> >         return 0;
> >  }
> > @@ -1658,11 +1653,13 @@ static void ufs_qcom_dump_dbg_regs(struct 
> > ufs_hba *hba)
> >
> >         /* sleep a bit intermittently as we are dumping too much data 
> > */
> >         ufs_qcom_print_hw_debug_reg_all(hba, NULL, 
> > ufs_qcom_dump_regs_wrapper);
> > -       udelay(1000);
> > -       ufs_qcom_testbus_read(hba);
> > -       udelay(1000);
> > -       ufs_qcom_print_unipro_testbus(hba);
> > -       udelay(1000);
> > +       if (in_task()) {
> > +               udelay(1000);
> > +               ufs_qcom_testbus_read(hba);
> > +               udelay(1000);
> > +               ufs_qcom_print_unipro_testbus(hba);
> > +               udelay(1000);
> > +       }
>
> Did you run into a specific issue with this?  udelay is not a "sleep"
> in the sense that it causes scheduling to occur, which is the problem
> with atomic contexts.

Here, ufs_qcom_print_unipro_testbus is actually causing the problem as 
it has
kmalloc with flag GFP_KERNEL. Even we change the kmalloc flag to ATOMIC, 
the
prints are still too heavy for atomic contexts. So we want to mute all 
test bus
prints in atomic contexts. Hence the in_task() check. But apprently I 
should move
the check up to have all the testbus prints inside the check. I will 
modify the
change and  the commit msg to tell the true story.

Thanks,

Can Guo.