RE: [PATCH V4 2/7] mmc: sdhci: Capture eMMC and SD card errors

From: Sajida Bhanu (Temp) (QUIC)
Date: Sat Mar 12 2022 - 12:49:11 EST

Next message: James Jones: "Re: [PATCH] x86: Remove a.out support"
Previous message: Paul E. McKenney: "Re: [PATCH v3 2/3] sched/cpuacct: optimize away RCU read lock"
In reply to: Adrian Hunter: "Re: [PATCH V4 2/7] mmc: sdhci: Capture eMMC and SD card errors"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

Thanks for the review.

Please find the comments inline.

Thanks,
Sajida
> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> Sent: Tuesday, March 8, 2022 3:07 PM
> To: Sajida Bhanu (Temp) (QUIC) <quic_c_sbhanu@xxxxxxxxxxx>;
> quic_riteshh@xxxxxxxxxxx; Asutosh Das (asd) <asutoshd@xxxxxxxxxxx>;
> ulf.hansson@xxxxxxxxxx; agross@xxxxxxxxxx; bjorn.andersson@xxxxxxxxxx;
> linux-mmc@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Cc: Veerabhadrarao Badiganti (QUIC) <quic_vbadigan@xxxxxxxxxxx>; Ram
> Prakash Gupta (QUIC) <quic_rampraka@xxxxxxxxxxx>; Pradeep Pragallapati
> (QUIC) <quic_pragalla@xxxxxxxxxxx>; Sarthak Garg (QUIC)
> <quic_sartgarg@xxxxxxxxxxx>; Nitin Rawat (QUIC)
> <quic_nitirawa@xxxxxxxxxxx>; Sayali Lokhande (QUIC)
> <quic_sayalil@xxxxxxxxxxx>; Liangliang Lu <quic_luliang@xxxxxxxxxxx>;
> quic_nguyenb <quic_nguyenb@xxxxxxxxxxx>
> Subject: Re: [PATCH V4 2/7] mmc: sdhci: Capture eMMC and SD card errors
>
> On 2.3.2022 15.03, Shaik Sajida Bhanu wrote:
> > Add changes to capture eMMC and SD card errors.
> > This is useful for debug and testing.
> >
> > Signed-off-by: Liangliang Lu <quic_luliang@xxxxxxxxxxx>
> > Signed-off-by: Sayali Lokhande <quic_sayalil@xxxxxxxxxxx>
> > Signed-off-by: Bao D. Nguyen <quic_nguyenb@xxxxxxxxxxx>
> > Signed-off-by: Shaik Sajida Bhanu <quic_c_sbhanu@xxxxxxxxxxx>
> > ---
> > drivers/mmc/host/sdhci.c | 52
> > +++++++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 40 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index
> > 07c6da1..741fb06 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -3159,6 +3159,7 @@ static void sdhci_timeout_timer(struct timer_list
> *t)
> > spin_lock_irqsave(&host->lock, flags);
> >
> > if (host->cmd && !sdhci_data_line_cmd(host->cmd)) {
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_REQ_TIMEOUT);
>
> Let's make a macro for this:
>
> #define sdhci_err_stats_inc(host, err_name) \
> mmc_debugfs_err_stats_inc(host->mmc, MMC_ERR_ ## err_name)
>
Sure
> > pr_err("%s: Timeout waiting for hardware cmd interrupt.\n",
> > mmc_hostname(host->mmc));
>
> Please move the err_stats_inc here after the error message i.e.
>
> sdhci_err_stats_inc(host, REQ_TIMEOUT);
>
Ok
> > sdhci_dumpregs(host);
> > @@ -3181,6 +3182,7 @@ static void sdhci_timeout_data_timer(struct
> > timer_list *t)
> >
> > if (host->data || host->data_cmd ||
> > (host->cmd && sdhci_data_line_cmd(host->cmd))) {
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_REQ_TIMEOUT);
> > pr_err("%s: Timeout waiting for hardware interrupt.\n",
> > mmc_hostname(host->mmc));
>
> Please move the err_stats_inc here after the error message i.e.
>
> sdhci_err_stats_inc(host, REQ_TIMEOUT);
>
Ok
> > sdhci_dumpregs(host);
> > @@ -3240,11 +3242,15 @@ static void sdhci_cmd_irq(struct sdhci_host
> > *host, u32 intmask, u32 *intmask_p)
> >
> > if (intmask & (SDHCI_INT_TIMEOUT | SDHCI_INT_CRC |
> > SDHCI_INT_END_BIT | SDHCI_INT_INDEX)) {
> > - if (intmask & SDHCI_INT_TIMEOUT)
> > + if (intmask & SDHCI_INT_TIMEOUT) {
> > host->cmd->error = -ETIMEDOUT;
> > - else
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_CMD_TIMEOUT);
> > + } else {
> > host->cmd->error = -EILSEQ;
> > -
> > + if (host->cmd->opcode !=
> MMC_SEND_TUNING_BLOCK ||
> > + host->cmd->opcode !=
> MMC_SEND_TUNING_BLOCK_HS200)
>
> That should be && not ||
>
> Please add a helper as below, then this can be:
>
> if (!mmc_op_tuning(host->cmd->opcode))
> sdhci_err_stats_inc(host, CMD_CRC);
>
>
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index
> d9a65c6a8816..d809bdfcc59b 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -99,6 +99,12 @@ static inline bool mmc_op_multi(u32 opcode)
> opcode == MMC_READ_MULTIPLE_BLOCK; }
>
> +static inline bool mmc_op_tuning(u32 opcode) {
> + return opcode == MMC_SEND_TUNING_BLOCK ||
> + opcode == MMC_SEND_TUNING_BLOCK_HS200; }
> +
> /*
> * MMC_SWITCH argument format:
> *
>
>
>
Sure will add a function and call
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_CMD_CRC);
> > + }
> > /* Treat data command CRC error the same as data CRC error
> */
> > if (host->cmd->data &&
> > (intmask & (SDHCI_INT_CRC | SDHCI_INT_TIMEOUT)) ==
> @@ -3265,6
> > +3271,7 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32
> intmask, u32 *intmask_p)
> > int err = (auto_cmd_status & SDHCI_AUTO_CMD_TIMEOUT)
> ?
> > -ETIMEDOUT :
> > -EILSEQ;
>
> Should be a blank line here after the local variable declarations.
>
Sure
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_AUTO_CMD);
> >
> > if (sdhci_auto_cmd23(host, mrq)) {
> > mrq->sbc->error = err;
> > @@ -3342,6 +3349,7 @@ static void sdhci_data_irq(struct sdhci_host
> *host, u32 intmask)
> > if (intmask & SDHCI_INT_DATA_TIMEOUT) {
> > host->data_cmd = NULL;
> > data_cmd->error = -ETIMEDOUT;
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_CMD_TIMEOUT);
> > __sdhci_finish_mrq(host, data_cmd->mrq);
> > return;
> > }
> > @@ -3375,18 +3383,25 @@ static void sdhci_data_irq(struct sdhci_host
> *host, u32 intmask)
> > return;
> > }
> >
> > - if (intmask & SDHCI_INT_DATA_TIMEOUT)
> > + if (intmask & SDHCI_INT_DATA_TIMEOUT) {
> > host->data->error = -ETIMEDOUT;
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_DAT_TIMEOUT);
> > + }
>
> The parenthesis should be with the 'else'. Please use checkpatch with the --
> strict option and fix the issues.
>
Sure Thank you
> > else if (intmask & SDHCI_INT_DATA_END_BIT)
> > host->data->error = -EILSEQ;
> > else if ((intmask & SDHCI_INT_DATA_CRC) &&
> > SDHCI_GET_CMD(sdhci_readw(host, SDHCI_COMMAND))
> > - != MMC_BUS_TEST_R)
> > + != MMC_BUS_TEST_R) {
> > host->data->error = -EILSEQ;
> > + if (host->cmd->opcode != MMC_SEND_TUNING_BLOCK ||
> > + host->cmd->opcode !=
> MMC_SEND_TUNING_BLOCK_HS200)
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_DAT_CRC);
>
> As previously:
>
> if (!mmc_op_tuning(host->cmd->opcode))
> sdhci_err_stats_inc(host, DAT_CRC);
>
Sure will update
> > + }
> > else if (intmask & SDHCI_INT_ADMA_ERROR) {
> > pr_err("%s: ADMA error: 0x%08x\n", mmc_hostname(host-
> >mmc),
> > intmask);
> > sdhci_adma_show_error(host);
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_ADMA);
> > host->data->error = -EIO;
> > if (host->ops->adma_workaround)
> > host->ops->adma_workaround(host, intmask); @@ -
> 3905,20 +3920,33 @@
> > bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
> > if (!host->cqe_on)
> > return false;
> >
> > - if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT |
> SDHCI_INT_CRC))
> > + if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT |
> SDHCI_INT_CRC))
> > +{
> > *cmd_error = -EILSEQ;
> > - else if (intmask & SDHCI_INT_TIMEOUT)
> > + if (intmask & SDHCI_INT_CRC) {
> > + if (host->cmd->opcode !=
> MMC_SEND_TUNING_BLOCK ||
> > + host->cmd->opcode !=
> MMC_SEND_TUNING_BLOCK_HS200)
>
> Please count INDEX and END_BIT errors same as CRC errors i.e. just
>
> if (!mmc_op_tuning(host->cmd->opcode))
> sdhci_err_stats_inc(host, CMD_CRC);
>
Sure
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_CMD_CRC);
> > + }
> > + } else if (intmask & SDHCI_INT_TIMEOUT) {
> > *cmd_error = -ETIMEDOUT;
> > - else
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_CMD_TIMEOUT);
> > + } else
> > *cmd_error = 0;
> >
> > - if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC))
> > + if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC))
> {
> > *data_error = -EILSEQ;
> > - else if (intmask & SDHCI_INT_DATA_TIMEOUT)
> > + if (intmask & SDHCI_INT_DATA_CRC) {
> > + if (host->cmd->opcode !=
> MMC_SEND_TUNING_BLOCK ||
> > + host->cmd->opcode !=
> MMC_SEND_TUNING_BLOCK_HS200)
>
> Please count END_BIT error same as CRC error i.e. just
>
> if (!mmc_op_tuning(host->cmd->opcode))
> sdhci_err_stats_inc(host, DAT_CRC);
>
Sure
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_DAT_CRC);
> > + }
> > + } else if (intmask & SDHCI_INT_DATA_TIMEOUT) {
> > *data_error = -ETIMEDOUT;
> > - else if (intmask & SDHCI_INT_ADMA_ERROR)
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_DAT_TIMEOUT);
> > + } else if (intmask & SDHCI_INT_ADMA_ERROR) {
> > *data_error = -EIO;
> > - else
> > + mmc_debugfs_err_stats_inc(host->mmc,
> MMC_ERR_ADMA);
> > + } else
> > *data_error = 0;
> >
> > /* Clear selected interrupts. */

Next message: James Jones: "Re: [PATCH] x86: Remove a.out support"
Previous message: Paul E. McKenney: "Re: [PATCH v3 2/3] sched/cpuacct: optimize away RCU read lock"
In reply to: Adrian Hunter: "Re: [PATCH V4 2/7] mmc: sdhci: Capture eMMC and SD card errors"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]