[PATCH 0/1] mmc: sdhci: check host->cmd is null or not in sdhci_cqe_irq()

[PATCH 0/1] mmc: sdhci: check host->cmd is null or not in sdhci_cqe_irq()

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

If we don't check host->cmd. We will get the following oops
when booting from eMMC on RockPi4.

 Unable to handle kernel read from unreadable memory at
   virtual address 0000000000000000
 Call trace:
   sdhci_cqe_irq+0x40/0x254

Ying-Chun Liu (PaulLiu) (1):
  mmc: sdhci: check host->cmd is null or not in sdhci_cqe_irq()

 drivers/mmc/host/sdhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH 1/1] mmc: sdhci: check host->cmd is null or not in sdhci_cqe_irq()

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

If we don't check host->cmd. We will get the following oops
when booting from eMMC on RockPi4.

 Unable to handle kernel read from unreadable memory at
   virtual address 0000000000000000
 Call trace:
   sdhci_cqe_irq+0x40/0x254

Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Signed-off-by: Ram Muthiah <rammuthiah@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ritesh Harjani <riteshh@codeaurora.org>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: linux-mmc@vger.kernel.org
---
 drivers/mmc/host/sdhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 7689ffec5ad1..e0e7768bd6c0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3928,7 +3928,7 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
 
 	if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC)) {
 		*cmd_error = -EILSEQ;
-		if (!mmc_op_tuning(host->cmd->opcode))
+		if (host->cmd && !mmc_op_tuning(host->cmd->opcode))
 			sdhci_err_stats_inc(host, CMD_CRC);
 	} else if (intmask & SDHCI_INT_TIMEOUT) {
 		*cmd_error = -ETIMEDOUT;
@@ -3938,7 +3938,7 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
 
 	if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC)) {
 		*data_error = -EILSEQ;
-		if (!mmc_op_tuning(host->cmd->opcode))
+		if (host->cmd && !mmc_op_tuning(host->cmd->opcode))
 			sdhci_err_stats_inc(host, DAT_CRC);
 	} else if (intmask & SDHCI_INT_DATA_TIMEOUT) {
 		*data_error = -ETIMEDOUT;
-- 
2.35.1

Re: [PATCH 1/1] mmc: sdhci: check host->cmd is null or not in sdhci_cqe_irq()

[Adrian Hunter]

On 31/08/22 15:43, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> If we don't check host->cmd. We will get the following oops
> when booting from eMMC on RockPi4.
> 
>  Unable to handle kernel read from unreadable memory at
>    virtual address 0000000000000000
>  Call trace:
>    sdhci_cqe_irq+0x40/0x254

Thank you!  However, I prefer this patch:

https://lore.kernel.org/linux-mmc/20220907035847.13783-1-wenchao.chen666@gmail.com/

> 
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Signed-off-by: Ram Muthiah <rammuthiah@google.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Ritesh Harjani <riteshh@codeaurora.org>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-mmc@vger.kernel.org
> ---
>  drivers/mmc/host/sdhci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 7689ffec5ad1..e0e7768bd6c0 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3928,7 +3928,7 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
>  
>  	if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC)) {
>  		*cmd_error = -EILSEQ;
> -		if (!mmc_op_tuning(host->cmd->opcode))
> +		if (host->cmd && !mmc_op_tuning(host->cmd->opcode))
>  			sdhci_err_stats_inc(host, CMD_CRC);
>  	} else if (intmask & SDHCI_INT_TIMEOUT) {
>  		*cmd_error = -ETIMEDOUT;
> @@ -3938,7 +3938,7 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
>  
>  	if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC)) {
>  		*data_error = -EILSEQ;
> -		if (!mmc_op_tuning(host->cmd->opcode))
> +		if (host->cmd && !mmc_op_tuning(host->cmd->opcode))
>  			sdhci_err_stats_inc(host, DAT_CRC);
>  	} else if (intmask & SDHCI_INT_DATA_TIMEOUT) {
>  		*data_error = -ETIMEDOUT;

Re: [PATCH 1/1] mmc: sdhci: check host->cmd is null or not in sdhci_cqe_irq()

[Ulf Hansson]

Hi Adrian,

On Wed, 7 Sept 2022 at 09:06, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 31/08/22 15:43, Ying-Chun Liu (PaulLiu) wrote:
> > From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> >
> > If we don't check host->cmd. We will get the following oops
> > when booting from eMMC on RockPi4.
> >
> >  Unable to handle kernel read from unreadable memory at
> >    virtual address 0000000000000000
> >  Call trace:
> >    sdhci_cqe_irq+0x40/0x254
>
> Thank you!  However, I prefer this patch:
>
> https://lore.kernel.org/linux-mmc/20220907035847.13783-1-wenchao.chen666@gmail.com/

That patch means that we need to do a sdhci_readw() to figure out what
command we are running, rather than just checking for a valid pointer.
Maybe it's not a big deal as this code isn't executed that often?

Anyway, it's your call to make, I pick the patch that you prefer.

Kind regards
Uffe

>
> >
> > Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> > Signed-off-by: Ram Muthiah <rammuthiah@google.com>
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Ritesh Harjani <riteshh@codeaurora.org>
> > Cc: Asutosh Das <asutoshd@codeaurora.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: linux-mmc@vger.kernel.org
> > ---
> >  drivers/mmc/host/sdhci.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 7689ffec5ad1..e0e7768bd6c0 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -3928,7 +3928,7 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
> >
> >       if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC)) {
> >               *cmd_error = -EILSEQ;
> > -             if (!mmc_op_tuning(host->cmd->opcode))
> > +             if (host->cmd && !mmc_op_tuning(host->cmd->opcode))
> >                       sdhci_err_stats_inc(host, CMD_CRC);
> >       } else if (intmask & SDHCI_INT_TIMEOUT) {
> >               *cmd_error = -ETIMEDOUT;
> > @@ -3938,7 +3938,7 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
> >
> >       if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC)) {
> >               *data_error = -EILSEQ;
> > -             if (!mmc_op_tuning(host->cmd->opcode))
> > +             if (host->cmd && !mmc_op_tuning(host->cmd->opcode))
> >                       sdhci_err_stats_inc(host, DAT_CRC);
> >       } else if (intmask & SDHCI_INT_DATA_TIMEOUT) {
> >               *data_error = -ETIMEDOUT;
>

Re: [PATCH 1/1] mmc: sdhci: check host->cmd is null or not in sdhci_cqe_irq()

[Adrian Hunter]

On 7/09/22 13:03, Ulf Hansson wrote:
> Hi Adrian,
> 
> On Wed, 7 Sept 2022 at 09:06, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 31/08/22 15:43, Ying-Chun Liu (PaulLiu) wrote:
>>> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
>>>
>>> If we don't check host->cmd. We will get the following oops
>>> when booting from eMMC on RockPi4.
>>>
>>>  Unable to handle kernel read from unreadable memory at
>>>    virtual address 0000000000000000
>>>  Call trace:
>>>    sdhci_cqe_irq+0x40/0x254
>>
>> Thank you!  However, I prefer this patch:
>>
>> https://lore.kernel.org/linux-mmc/20220907035847.13783-1-wenchao.chen666@gmail.com/
> 
> That patch means that we need to do a sdhci_readw() to figure out what
> command we are running, rather than just checking for a valid pointer.
> Maybe it's not a big deal as this code isn't executed that often?

That was my thinking, but CQE/CQHCI does not set host->cmd either.
It is the same approach taken in sdhci_data_irq() so consistent at
least.

> 
> Anyway, it's your call to make, I pick the patch that you prefer.
> 
> Kind regards
> Uffe
> 
>>
>>>
>>> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
>>> Signed-off-by: Ram Muthiah <rammuthiah@google.com>
>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>> Cc: Ritesh Harjani <riteshh@codeaurora.org>
>>> Cc: Asutosh Das <asutoshd@codeaurora.org>
>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>> Cc: linux-mmc@vger.kernel.org
>>> ---
>>>  drivers/mmc/host/sdhci.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 7689ffec5ad1..e0e7768bd6c0 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -3928,7 +3928,7 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
>>>
>>>       if (intmask & (SDHCI_INT_INDEX | SDHCI_INT_END_BIT | SDHCI_INT_CRC)) {
>>>               *cmd_error = -EILSEQ;
>>> -             if (!mmc_op_tuning(host->cmd->opcode))
>>> +             if (host->cmd && !mmc_op_tuning(host->cmd->opcode))
>>>                       sdhci_err_stats_inc(host, CMD_CRC);
>>>       } else if (intmask & SDHCI_INT_TIMEOUT) {
>>>               *cmd_error = -ETIMEDOUT;
>>> @@ -3938,7 +3938,7 @@ bool sdhci_cqe_irq(struct sdhci_host *host, u32 intmask, int *cmd_error,
>>>
>>>       if (intmask & (SDHCI_INT_DATA_END_BIT | SDHCI_INT_DATA_CRC)) {
>>>               *data_error = -EILSEQ;
>>> -             if (!mmc_op_tuning(host->cmd->opcode))
>>> +             if (host->cmd && !mmc_op_tuning(host->cmd->opcode))
>>>                       sdhci_err_stats_inc(host, DAT_CRC);
>>>       } else if (intmask & SDHCI_INT_DATA_TIMEOUT) {
>>>               *data_error = -ETIMEDOUT;
>>