Reigsters dump intiated from atomic context should not sleep. To fix it,
add one boolean parameter to register dump vops to inform vendor driver if
sleep is allowed or not.
Signed-off-by: Can Guo <[email protected]>
diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 3b5b2d9..c30139c 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1619,13 +1619,17 @@ static void ufs_qcom_print_unipro_testbus(struct ufs_hba *hba)
kfree(testbus);
}
-static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba)
+static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba, bool no_sleep)
{
ufshcd_dump_regs(hba, REG_UFS_SYS1CLK_1US, 16 * 4,
"HCI Vendor Specific Registers ");
/* 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);
+
+ if (no_sleep)
+ return;
+
usleep_range(1000, 1100);
ufs_qcom_testbus_read(hba);
usleep_range(1000, 1100);
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 0ac5d47..37f1539 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -398,7 +398,7 @@ static void ufshcd_print_err_hist(struct ufs_hba *hba,
dev_err(hba->dev, "No record of %s\n", err_name);
}
-static void ufshcd_print_host_regs(struct ufs_hba *hba)
+static inline void __ufshcd_print_host_regs(struct ufs_hba *hba, bool no_sleep)
{
ufshcd_dump_regs(hba, 0, UFSHCI_REG_SPACE_SIZE, "host_regs: ");
dev_err(hba->dev, "hba->ufs_version = 0x%x, hba->capabilities = 0x%x\n",
@@ -430,7 +430,12 @@ static void ufshcd_print_host_regs(struct ufs_hba *hba)
ufshcd_print_clk_freqs(hba);
- ufshcd_vops_dbg_register_dump(hba);
+ ufshcd_vops_dbg_register_dump(hba, no_sleep);
+}
+
+static void ufshcd_print_host_regs(struct ufs_hba *hba)
+{
+ __ufshcd_print_host_regs(hba, false);
}
static
@@ -4821,7 +4826,7 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
dev_err(hba->dev,
"OCS error from controller = %x for tag %d\n",
ocs, lrbp->task_tag);
- ufshcd_print_host_regs(hba);
+ __ufshcd_print_host_regs(hba, true);
ufshcd_print_host_state(hba);
break;
} /* end of switch */
@@ -5617,7 +5622,7 @@ static irqreturn_t ufshcd_check_errors(struct ufs_hba *hba)
__func__, hba->saved_err,
hba->saved_uic_err);
- ufshcd_print_host_regs(hba);
+ __ufshcd_print_host_regs(hba, true);
ufshcd_print_pwr_info(hba);
ufshcd_print_tmrs(hba, hba->outstanding_tasks);
ufshcd_print_trs(hba, hba->outstanding_reqs,
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 2ae6c7c..3de7cbb 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -323,7 +323,7 @@ struct ufs_hba_variant_ops {
int (*apply_dev_quirks)(struct ufs_hba *hba);
int (*suspend)(struct ufs_hba *, enum ufs_pm_op);
int (*resume)(struct ufs_hba *, enum ufs_pm_op);
- void (*dbg_register_dump)(struct ufs_hba *hba);
+ void (*dbg_register_dump)(struct ufs_hba *hba, bool no_sleep);
int (*phy_initialization)(struct ufs_hba *);
void (*device_reset)(struct ufs_hba *hba);
};
@@ -1078,10 +1078,11 @@ static inline int ufshcd_vops_resume(struct ufs_hba *hba, enum ufs_pm_op op)
return 0;
}
-static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba)
+static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba,
+ bool no_sleep)
{
if (hba->vops && hba->vops->dbg_register_dump)
- hba->vops->dbg_register_dump(hba);
+ hba->vops->dbg_register_dump(hba, no_sleep);
}
static inline void ufshcd_vops_device_reset(struct ufs_hba *hba)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 2020-02-04 22:06, Can Guo wrote:
> @@ -5617,7 +5622,7 @@ static irqreturn_t ufshcd_check_errors(struct
>
> __func__, hba->saved_err,
> hba->saved_uic_err);
>
> - ufshcd_print_host_regs(hba);
> + __ufshcd_print_host_regs(hba, true);
> ufshcd_print_pwr_info(hba);
> ufshcd_print_tmrs(hba,
> hba->outstanding_tasks);
> ufshcd_print_trs(hba,
> hba->outstanding_reqs,
Hi Can,
Please fix this by splitting ufs_qcom_dump_dbg_regs() into two
functions: one function that doesn't sleep and a second function that
behaves identically to the current function. If the function names will
make it clear which function sleeps and which function doesn't that will
result in code that is much easier to read than the above code. For the
above code it is namely impossible to figure out what will happen
without looking up the caller.
Thanks,
Bart.
On 2020-02-05 14:21, Bart Van Assche wrote:
> On 2020-02-04 22:06, Can Guo wrote:
>> @@ -5617,7 +5622,7 @@ static irqreturn_t ufshcd_check_errors(struct
>>
>> __func__, hba->saved_err,
>> hba->saved_uic_err);
>>
>> - ufshcd_print_host_regs(hba);
>> + __ufshcd_print_host_regs(hba, true);
>> ufshcd_print_pwr_info(hba);
>> ufshcd_print_tmrs(hba,
>> hba->outstanding_tasks);
>> ufshcd_print_trs(hba,
>> hba->outstanding_reqs,
>
> Hi Can,
>
> Please fix this by splitting ufs_qcom_dump_dbg_regs() into two
> functions: one function that doesn't sleep and a second function that
> behaves identically to the current function. If the function names will
> make it clear which function sleeps and which function doesn't that
> will
> result in code that is much easier to read than the above code. For the
> above code it is namely impossible to figure out what will happen
> without looking up the caller.
>
> Thanks,
>
> Bart.
Hi Bart,
Do you mean by splitting ufshcd_print_host_regs() into two functions?
One behaves identically same to the current function, another one called
ufshcd_print_host_regs_nosleep(). No?
Thanks,
Can Guo.
On 2/4/20 10:31 PM, Can Guo wrote:
> Do you mean by splitting ufshcd_print_host_regs() into two functions?
> One behaves identically same to the current function, another one called
> ufshcd_print_host_regs_nosleep(). No?
Hi Can,
Not really. I had something else in mind.
Having taken a closer look at ufs_qcom_dump_dbg_regs() I started
wondering why there are sleep statements in that function. Is the goal
of these sleep statements perhaps to reduce how often printk() is
called? Has it been considered to remove all sleep calls from
ufs_qcom_dump_dbg_regs() and instead add something like the following at
the start of that function:
static DEFINE_RATELIMIT_STATE(_rs,
DEFAULT_RATELIMIT_INTERVAL,
DEFAULT_RATELIMIT_BURST);
if (!__ratelimit(&_rs))
return;
Thanks,
Bart.