| Message ID | 1595226956-7779-4-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix up and simplify error recovery mechanism | expand |
On Mon, Jul 20, 2020 at 12:39 AM Can Guo <cang@codeaurora.org> 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. > > Signed-off-by: Can Guo <cang@codeaurora.org> > --- > 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.
On 2020-07-22 22:37, Jeffrey Hugo wrote: > On Mon, Jul 20, 2020 at 12:39 AM Can Guo <cang@codeaurora.org> 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@codeaurora.org> >> --- >> 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.
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); + } } /**
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(). Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/ufs/ufs-qcom.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)