2022-04-25 18:47:22

by Shaik Sajida Bhanu

Subject: [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver

Add debug fs entry error state to query eMMC and SD card errors statistics.
If any errors occurred in eMMC and SD card driver level then
err_state value will be set to 1.

Signed-off-by: Liangliang Lu <[email protected]>
Signed-off-by: Sayali Lokhande <[email protected]>
Signed-off-by: Bao D. Nguyen <[email protected]>
Signed-off-by: Shaik Sajida Bhanu <[email protected]>
---
drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)

diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 6aa5a60..2f5b63f 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)

DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
"%llu\n");
+static int mmc_err_state_get(void *data, u64 *val)
+{
+ struct mmc_host *host = data;
+
+ if (!host)
+ return -EINVAL;
+
+ *val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
+ host->err_stats[MMC_ERR_ADMA] ||
+ host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
+ host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
+ host->err_stats[MMC_ERR_CMDQ_RED] ||
+ host->err_stats[MMC_ERR_CMDQ_GCE] ||
+ host->err_stats[MMC_ERR_CMDQ_ICCE] ||
+ host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
+ host->err_stats[MMC_ERR_DAT_CRC] ||
+ host->err_stats[MMC_ERR_CMD_CRC] ||
+ host->err_stats[MMC_ERR_CMD_TIMEOUT];
+
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");

static int mmc_err_stats_show(struct seq_file *file, void *data)
{
@@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
&mmc_clock_fops);

+ debugfs_create_file("err_state", 0600, root, host,
+ &mmc_err_state);
debugfs_create_file("err_stats", 0600, root, host,
&mmc_err_stats_fops);

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

2022-04-26 16:46:23

by Adrian Hunter

Subject: Re: [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver

On 26/04/22 10:54, Adrian Hunter wrote:
> On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
>> Add debug fs entry error state to query eMMC and SD card errors statistics.
>> If any errors occurred in eMMC and SD card driver level then
>> err_state value will be set to 1.
>>
>> Signed-off-by: Liangliang Lu <[email protected]>
>> Signed-off-by: Sayali Lokhande <[email protected]>
>> Signed-off-by: Bao D. Nguyen <[email protected]>
>> Signed-off-by: Shaik Sajida Bhanu <[email protected]>
>> ---
>> drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>> index 6aa5a60..2f5b63f 100644
>> --- a/drivers/mmc/core/debugfs.c
>> +++ b/drivers/mmc/core/debugfs.c
>> @@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)
>>
>> DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
>> "%llu\n");
>
> A blank line would be nice here
>
>> +static int mmc_err_state_get(void *data, u64 *val)
>> +{
>> + struct mmc_host *host = data;
>> +
>> + if (!host)
>> + return -EINVAL;
>> +
>
> I am not sure why you have left out some err_stats[].
> Why not all of them? At least, it needs a comment to explain.
>
>> + *val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
>> + host->err_stats[MMC_ERR_ADMA] ||
>> + host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
>> + host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
>> + host->err_stats[MMC_ERR_CMDQ_RED] ||
>> + host->err_stats[MMC_ERR_CMDQ_GCE] ||
>> + host->err_stats[MMC_ERR_CMDQ_ICCE] ||
>> + host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
>> + host->err_stats[MMC_ERR_DAT_CRC] ||
>> + host->err_stats[MMC_ERR_CMD_CRC] ||
>> + host->err_stats[MMC_ERR_CMD_TIMEOUT];
>> +
>> + return 0;
>> +}
>> +
>> +DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");

Also, if possible, please use DEFINE_DEBUGFS_ATTRIBUTE / debugfs_create_file_unsafe
in this case

>>
>> static int mmc_err_stats_show(struct seq_file *file, void *data)
>> {
>> @@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
>> debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
>> &mmc_clock_fops);
>>
>> + debugfs_create_file("err_state", 0600, root, host,
>> + &mmc_err_state);
>> debugfs_create_file("err_stats", 0600, root, host,
>> &mmc_err_stats_fops);
>>
>

2022-04-27 10:00:03

by Adrian Hunter

Subject: Re: [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver

On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
> Add debug fs entry error state to query eMMC and SD card errors statistics.
> If any errors occurred in eMMC and SD card driver level then
> err_state value will be set to 1.
>
> Signed-off-by: Liangliang Lu <[email protected]>
> Signed-off-by: Sayali Lokhande <[email protected]>
> Signed-off-by: Bao D. Nguyen <[email protected]>
> Signed-off-by: Shaik Sajida Bhanu <[email protected]>
> ---
> drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
> index 6aa5a60..2f5b63f 100644
> --- a/drivers/mmc/core/debugfs.c
> +++ b/drivers/mmc/core/debugfs.c
> @@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)
>
> DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
> "%llu\n");

A blank line would be nice here

> +static int mmc_err_state_get(void *data, u64 *val)
> +{
> + struct mmc_host *host = data;
> +
> + if (!host)
> + return -EINVAL;
> +

I am not sure why you have left out some err_stats[].
Why not all of them? At least, it needs a comment to explain.

> + *val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
> + host->err_stats[MMC_ERR_ADMA] ||
> + host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
> + host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
> + host->err_stats[MMC_ERR_CMDQ_RED] ||
> + host->err_stats[MMC_ERR_CMDQ_GCE] ||
> + host->err_stats[MMC_ERR_CMDQ_ICCE] ||
> + host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
> + host->err_stats[MMC_ERR_DAT_CRC] ||
> + host->err_stats[MMC_ERR_CMD_CRC] ||
> + host->err_stats[MMC_ERR_CMD_TIMEOUT];
> +
> + return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");
>
> static int mmc_err_stats_show(struct seq_file *file, void *data)
> {
> @@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
> debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
> &mmc_clock_fops);
>
> + debugfs_create_file("err_state", 0600, root, host,
> + &mmc_err_state);
> debugfs_create_file("err_stats", 0600, root, host,
> &mmc_err_stats_fops);
>

2022-05-09 10:54:12

by Shaik Sajida Bhanu

Subject: Re: [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver

Hi,

Thank you for review.

Please find the inline comments.

Thanks,

Sajida

On 4/26/2022 1:28 PM, Adrian Hunter wrote:
> On 26/04/22 10:54, Adrian Hunter wrote:
>> On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
>>> Add debug fs entry error state to query eMMC and SD card errors statistics.
>>> If any errors occurred in eMMC and SD card driver level then
>>> err_state value will be set to 1.
>>>
>>> Signed-off-by: Liangliang Lu <[email protected]>
>>> Signed-off-by: Sayali Lokhande <[email protected]>
>>> Signed-off-by: Bao D. Nguyen <[email protected]>
>>> Signed-off-by: Shaik Sajida Bhanu <[email protected]>
>>> ---
>>> drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
>>> 1 file changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>>> index 6aa5a60..2f5b63f 100644
>>> --- a/drivers/mmc/core/debugfs.c
>>> +++ b/drivers/mmc/core/debugfs.c
>>> @@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)
>>>
>>> DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
>>> "%llu\n");
>> A blank line would be nice here
>>
>>> +static int mmc_err_state_get(void *data, u64 *val)
>>> +{
>>> + struct mmc_host *host = data;
>>> +
>>> + if (!host)
>>> + return -EINVAL;
>>> +
>> I am not sure why you have left out some err_stats[].
>> Why not all of them? At least, it needs a comment to explain.
>>
>>> + *val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
>>> + host->err_stats[MMC_ERR_ADMA] ||
>>> + host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
>>> + host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
>>> + host->err_stats[MMC_ERR_CMDQ_RED] ||
>>> + host->err_stats[MMC_ERR_CMDQ_GCE] ||
>>> + host->err_stats[MMC_ERR_CMDQ_ICCE] ||
>>> + host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
>>> + host->err_stats[MMC_ERR_DAT_CRC] ||
>>> + host->err_stats[MMC_ERR_CMD_CRC] ||
>>> + host->err_stats[MMC_ERR_CMD_TIMEOUT];
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");
> Also, if possible, please use DEFINE_DEBUGFS_ATTRIBUTE / debugfs_create_file_unsafe
> in this case
Sure.. will use  DEFINE_DEBUGFS_ATTRIBUTE
>
>>>
>>> static int mmc_err_stats_show(struct seq_file *file, void *data)
>>> {
>>> @@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
>>> debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
>>> &mmc_clock_fops);
>>>
>>> + debugfs_create_file("err_state", 0600, root, host,
>>> + &mmc_err_state);
>>> debugfs_create_file("err_stats", 0600, root, host,
>>> &mmc_err_stats_fops);
>>>

2022-05-09 10:55:33

by Adrian Hunter

Subject: Re: [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver

On 9/05/22 12:42, Sajida Bhanu (Temp) wrote:
> Hi,
>
> Thank you for the review.
>
> Please find the inline comments.
>
> Thanks,
>
> Sajida
>
> On 4/26/2022 1:24 PM, Adrian Hunter wrote:
>
>> On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
>>> Add debug fs entry error state to query eMMC and SD card errors statistics.
>>> If any errors occurred in eMMC and SD card driver level then
>>> err_state value will be set to 1.
>>>
>>> Signed-off-by: Liangliang Lu <[email protected]>
>>> Signed-off-by: Sayali Lokhande <[email protected]>
>>> Signed-off-by: Bao D. Nguyen <[email protected]>
>>> Signed-off-by: Shaik Sajida Bhanu <[email protected]>
>>> ---
>>> drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
>>> 1 file changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>>> index 6aa5a60..2f5b63f 100644
>>> --- a/drivers/mmc/core/debugfs.c
>>> +++ b/drivers/mmc/core/debugfs.c
>>> @@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)
>>>
>>> DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
>>> "%llu\n");
>> A blank line would be nice here
> Sure
>>> +static int mmc_err_state_get(void *data, u64 *val)
>>> +{
>>> + struct mmc_host *host = data;
>>> +
>>> + if (!host)
>>> + return -EINVAL;
>>> +
>> I am not sure why you have left out some err_stats[].
>> Why not all of them? At least, it needs a comment to explain.
>
> MMC_ERR_ICE_CFG --> we don't have ICE config.

So err_stats[MMC_ERR_ICE_CFG] would be zero and make
no difference.

If you are going to check all then you could loop
through them

*val = 0;
for (i = 0; i < MMC_ERR_MAX; i++) {
if (host->err_stats[i]) {
*val = 1;
break;
}
}

>
> Remaining we need to update, Thank you for pointing.
>
>>> + *val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
>>> + host->err_stats[MMC_ERR_ADMA] ||
>>> + host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
>>> + host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
>>> + host->err_stats[MMC_ERR_CMDQ_RED] ||
>>> + host->err_stats[MMC_ERR_CMDQ_GCE] ||
>>> + host->err_stats[MMC_ERR_CMDQ_ICCE] ||
>>> + host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
>>> + host->err_stats[MMC_ERR_DAT_CRC] ||
>>> + host->err_stats[MMC_ERR_CMD_CRC] ||
>>> + host->err_stats[MMC_ERR_CMD_TIMEOUT];
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");
>>>
>>> static int mmc_err_stats_show(struct seq_file *file, void *data)
>>> {
>>> @@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
>>> debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
>>> &mmc_clock_fops);
>>>
>>> + debugfs_create_file("err_state", 0600, root, host,
>>> + &mmc_err_state);
>>> debugfs_create_file("err_stats", 0600, root, host,
>>> &mmc_err_stats_fops);
>>>

2022-05-09 11:39:14

by Shaik Sajida Bhanu

Subject: Re: [PATCH V5 4/5] mmc: debugfs: Add debug fs error state entry for mmc driver

On 5/9/2022 3:33 PM, Adrian Hunter wrote:
> On 9/05/22 12:42, Sajida Bhanu (Temp) wrote:
>> Hi,
>>
>> Thank you for the review.
>>
>> Please find the inline comments.
>>
>> Thanks,
>>
>> Sajida
>>
>> On 4/26/2022 1:24 PM, Adrian Hunter wrote:
>>
>>> On 25/04/22 19:00, Shaik Sajida Bhanu wrote:
>>>> Add debug fs entry error state to query eMMC and SD card errors statistics.
>>>> If any errors occurred in eMMC and SD card driver level then
>>>> err_state value will be set to 1.
>>>>
>>>> Signed-off-by: Liangliang Lu <[email protected]>
>>>> Signed-off-by: Sayali Lokhande <[email protected]>
>>>> Signed-off-by: Bao D. Nguyen <[email protected]>
>>>> Signed-off-by: Shaik Sajida Bhanu <[email protected]>
>>>> ---
>>>> drivers/mmc/core/debugfs.c | 25 +++++++++++++++++++++++++
>>>> 1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>>>> index 6aa5a60..2f5b63f 100644
>>>> --- a/drivers/mmc/core/debugfs.c
>>>> +++ b/drivers/mmc/core/debugfs.c
>>>> @@ -222,6 +222,29 @@ static int mmc_clock_opt_set(void *data, u64 val)
>>>>
>>>> DEFINE_DEBUGFS_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
>>>> "%llu\n");
>>> A blank line would be nice here
>> Sure
>>>> +static int mmc_err_state_get(void *data, u64 *val)
>>>> +{
>>>> + struct mmc_host *host = data;
>>>> +
>>>> + if (!host)
>>>> + return -EINVAL;
>>>> +
>>> I am not sure why you have left out some err_stats[].
>>> Why not all of them? At least, it needs a comment to explain.
>> MMC_ERR_ICE_CFG --> we don't have ICE config.
> So err_stats[MMC_ERR_ICE_CFG] would be zero and make
> no difference.
>
> If you are going to check all then you could loop
> through them
>
> *val = 0;
> for (i = 0; i < MMC_ERR_MAX; i++) {
> if (host->err_stats[i]) {
> *val = 1;
> break;
> }
> }
Sure Thanks
>> Remaining we need to update, Thank you for pointing.
>>
>>>> + *val = host->err_stats[MMC_ERR_REQ_TIMEOUT] ||
>>>> + host->err_stats[MMC_ERR_ADMA] ||
>>>> + host->err_stats[MMC_ERR_CTRL_TIMEOUT] ||
>>>> + host->err_stats[MMC_ERR_UNEXPECTED_IRQ] ||
>>>> + host->err_stats[MMC_ERR_CMDQ_RED] ||
>>>> + host->err_stats[MMC_ERR_CMDQ_GCE] ||
>>>> + host->err_stats[MMC_ERR_CMDQ_ICCE] ||
>>>> + host->err_stats[MMC_ERR_DAT_TIMEOUT] ||
>>>> + host->err_stats[MMC_ERR_DAT_CRC] ||
>>>> + host->err_stats[MMC_ERR_CMD_CRC] ||
>>>> + host->err_stats[MMC_ERR_CMD_TIMEOUT];
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +DEFINE_SIMPLE_ATTRIBUTE(mmc_err_state, mmc_err_state_get, NULL, "%llu\n");
>>>>
>>>> static int mmc_err_stats_show(struct seq_file *file, void *data)
>>>> {
>>>> @@ -289,6 +312,8 @@ void mmc_add_host_debugfs(struct mmc_host *host)
>>>> debugfs_create_file_unsafe("clock", S_IRUSR | S_IWUSR, root, host,
>>>> &mmc_clock_fops);
>>>>
>>>> + debugfs_create_file("err_state", 0600, root, host,
>>>> + &mmc_err_state);
>>>> debugfs_create_file("err_stats", 0600, root, host,
>>>> &mmc_err_stats_fops);
>>>>