[<PATCH v1> 0/4] SD card bug fixes

[<PATCH v1> 0/4] SD card bug fixes

[Bao D. Nguyen]

I tried to make some changes earlier in the patch
[<PATCH v1> 9/9] mmc: sd: Fix trivial SD card issues].
There were a lot of valid comments asking to clarify
and separate the fixes into logical patches. With this
patch series, I am trying to address those comments.

Bao D. Nguyen (1):
  mmc: core: Add check for NULL pointer access

Ritesh Harjani (1):
  mmc: core: Make host->card as NULL when card is removed

Sahitya Tummala (1):
  mmc: core: update host->card after getting RCA for SD card

Subhash Jadavani (1):
  mmc: core: Attribute the IO wait time properly in
    mmc_wait_for_req_done()

 drivers/mmc/core/bus.c  | 8 ++++++++
 drivers/mmc/core/core.c | 5 ++++-
 drivers/mmc/core/sd.c   | 6 ++++--
 3 files changed, 16 insertions(+), 3 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[<PATCH v1> 1/4] mmc: core: Add check for NULL pointer access

[Bao D. Nguyen]

If the SD card is removed, the mmc_card pointer can be set to NULL
by the mmc_sd_remove() function. Check mmc_card pointer to avoid NULL
pointer access.

Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/mmc/core/bus.c  | 5 +++++
 drivers/mmc/core/core.c | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 74de3f2..4558f51 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -131,6 +131,11 @@ static void mmc_bus_shutdown(struct device *dev)
 	struct mmc_host *host = card->host;
 	int ret;
 
+	if (!card) {
+		dev_dbg(dev, "%s: %s: card is NULL\n", dev_name(dev), __func__);
+		return;
+	}
+
 	if (dev->driver && drv->shutdown)
 		drv->shutdown(card);
 
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6b38c19..94441a0 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -666,6 +666,9 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
 {
 	unsigned int mult;
 
+	if (!card)
+		return;
+
 	/*
 	 * SDIO cards only define an upper 1 s limit on access.
 	 */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [<PATCH v1> 1/4] mmc: core: Add check for NULL pointer access

[Ulf Hansson]

On Thu, 27 Feb 2020 at 23:06, Bao D. Nguyen <nguyenb@codeaurora.org> wrote:
>
> If the SD card is removed, the mmc_card pointer can be set to NULL
> by the mmc_sd_remove() function. Check mmc_card pointer to avoid NULL
> pointer access.
>
> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/mmc/core/bus.c  | 5 +++++
>  drivers/mmc/core/core.c | 3 +++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 74de3f2..4558f51 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -131,6 +131,11 @@ static void mmc_bus_shutdown(struct device *dev)
>         struct mmc_host *host = card->host;
>         int ret;

This obviously doesn't solve anything as we have already dereferenced
the card->host above. In other words we should hit a NULL pointer
dereference bug then.

More exactly, how do you trigger this problem?

>
> +       if (!card) {
> +               dev_dbg(dev, "%s: %s: card is NULL\n", dev_name(dev), __func__);
> +               return;
> +       }
> +
>         if (dev->driver && drv->shutdown)
>                 drv->shutdown(card);
>

[...]

Kind regards
Uffe

Re: [<PATCH v1> 1/4] mmc: core: Add check for NULL pointer access

[nguyenb]

On 2020-02-27 22:46, Ulf Hansson wrote:
> On Thu, 27 Feb 2020 at 23:06, Bao D. Nguyen <nguyenb@codeaurora.org> 
> wrote:
>> 
>> If the SD card is removed, the mmc_card pointer can be set to NULL
>> by the mmc_sd_remove() function. Check mmc_card pointer to avoid NULL
>> pointer access.
>> 
>> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> ---
>>  drivers/mmc/core/bus.c  | 5 +++++
>>  drivers/mmc/core/core.c | 3 +++
>>  2 files changed, 8 insertions(+)
>> 
>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index 74de3f2..4558f51 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -131,6 +131,11 @@ static void mmc_bus_shutdown(struct device *dev)
>>         struct mmc_host *host = card->host;
>>         int ret;
> 
> This obviously doesn't solve anything as we have already dereferenced
> the card->host above. In other words we should hit a NULL pointer
> dereference bug then.
> 
> More exactly, how do you trigger this problem?
I am porting this fix in the older kernel version 3.4. In that version 
3.4, the pointer check was needed.
Obviously, this NULL pointer check is not helping anything here as you 
pointed out. I will remove this check and resubmit.

> 
>> 
>> +       if (!card) {
>> +               dev_dbg(dev, "%s: %s: card is NULL\n", dev_name(dev), 
>> __func__);
>> +               return;
>> +       }
>> +
>>         if (dev->driver && drv->shutdown)
>>                 drv->shutdown(card);
>> 
> 
> [...]
> 
> Kind regards
> Uffe

Re: [<PATCH v1> 1/4] mmc: core: Add check for NULL pointer access

[Ulf Hansson]

On Fri, 6 Mar 2020 at 04:38, <nguyenb@codeaurora.org> wrote:
>
> On 2020-02-27 22:46, Ulf Hansson wrote:
> > On Thu, 27 Feb 2020 at 23:06, Bao D. Nguyen <nguyenb@codeaurora.org>
> > wrote:
> >>
> >> If the SD card is removed, the mmc_card pointer can be set to NULL
> >> by the mmc_sd_remove() function. Check mmc_card pointer to avoid NULL
> >> pointer access.
> >>
> >> Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
> >> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> >> ---
> >>  drivers/mmc/core/bus.c  | 5 +++++
> >>  drivers/mmc/core/core.c | 3 +++
> >>  2 files changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> >> index 74de3f2..4558f51 100644
> >> --- a/drivers/mmc/core/bus.c
> >> +++ b/drivers/mmc/core/bus.c
> >> @@ -131,6 +131,11 @@ static void mmc_bus_shutdown(struct device *dev)
> >>         struct mmc_host *host = card->host;
> >>         int ret;
> >
> > This obviously doesn't solve anything as we have already dereferenced
> > the card->host above. In other words we should hit a NULL pointer
> > dereference bug then.
> >
> > More exactly, how do you trigger this problem?
> I am porting this fix in the older kernel version 3.4. In that version
> 3.4, the pointer check was needed.
> Obviously, this NULL pointer check is not helping anything here as you
> pointed out. I will remove this check and resubmit.

Don't get me wrong, we have had serious errors in the mmc core before,
that we needed to fix. Perhaps, the series are addressing some issues
like this, I don't know.

The point is, either you need to provide a detailed theoretical proof,
or a description of how to reproduce the problem. If not, I don't see
how I can pick any of your patches, sorry.

[...]

Kind regards
Uffe

[<PATCH v1> 2/4] mmc: core: Attribute the IO wait time properly in mmc_wait_for_req_done()

[Bao D. Nguyen]

From: Subhash Jadavani <subhashj@codeaurora.org>

In mmc_wait_for_req_done() function, change the call wait_for_completion()
to wait_for_compltion_io(). This change makes the kernel account for
wait time as I/O wait and through another configuration, this io wait
is treated as busy which makes the acpu clock to scale up.

Signed-off-by: Murali Palnati <palnatim@codeaurora.org>
Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/mmc/core/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 94441a0..97a384a 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -399,7 +399,7 @@ void mmc_wait_for_req_done(struct mmc_host *host, struct mmc_request *mrq)
 	struct mmc_command *cmd;
 
 	while (1) {
-		wait_for_completion(&mrq->completion);
+		wait_for_completion_io(&mrq->completion);
 
 		cmd = mrq->cmd;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[<PATCH v1> 3/4] mmc: core: Make host->card as NULL when card is removed

[Bao D. Nguyen]

From: Ritesh Harjani <riteshh@codeaurora.org>

Make host->card as NULL when card is removed otherwise
we do see some kernel crashes in async contexts,
where host->card is referred (as dangling pointer).

Signed-off-by: Ritesh Harjani <riteshh@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/mmc/core/bus.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 4558f51..4677603 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -252,12 +252,15 @@ void mmc_unregister_driver(struct mmc_driver *drv)
 static void mmc_release_card(struct device *dev)
 {
 	struct mmc_card *card = mmc_dev_to_card(dev);
+	struct mmc_host *host = card->host;
 
 	sdio_free_common_cis(card);
 
 	kfree(card->info);
 
 	kfree(card);
+	if (host)
+		host->card = NULL;
 }
 
 /*
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[<PATCH v1> 4/4] mmc: core: update host->card after getting RCA for SD card

[Bao D. Nguyen]

From: Sahitya Tummala <stummala@codeaurora.org>

Make the host->card available before tuning is invoked for SD card.
In the sdhci_msm_execute_tuning(), we will send CMD13 only if
host->card is present because it needs the card->rca as its
argument to be sent. For emmc, host->card is already updated
immediately after the mmc_alloc_card(). In the similar way,
this change is for SD card. Without this change, tuning functionality
will not be able to send CMD13 to make sure the card is ready
for next data command. If the last tuning command failed
and we did not send CMD13 to ensure card is in transfer state,
the next read/write command will fail.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Bao D. Nguyen <nguyenb@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/mmc/core/sd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 76c7add..f0872e3 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -989,6 +989,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 		err = mmc_send_relative_addr(host, &card->rca);
 		if (err)
 			goto free_card;
+		host->card = card;
 	}
 
 	if (!oldcard) {
@@ -1100,12 +1101,13 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 		goto free_card;
 	}
 done:
-	host->card = card;
 	return 0;
 
 free_card:
-	if (!oldcard)
+	if (!oldcard) {
+		host->card = NULL;
 		mmc_remove_card(card);
+	}
 
 	return err;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project