[PATCH 0/4] mmc: sdio: Fix various issues in mmc_sdio_init_card()

[PATCH 0/4] mmc: sdio: Fix various issues in mmc_sdio_init_card()

[Ulf Hansson]

Over the years, the code in mmc_sdio_init_card() has grown to become quite
messy. Unfortunate also leading to several various errors.

This series addresses some of them, but it's likely that there are even more
things that needs to be fixed on top.

I have tested this on Hikey board, with an SDIO card corresponding to a WiFi
chip from TI (wl1835). Additional tests and review comments, are usual much
appreciated.

Kind regards
Uffe

Ulf Hansson (4):
  mmc: sdio: Fix potential NULL pointer error in mmc_sdio_init_card()
  mmc: sdio: Fix several potential memory leaks in mmc_sdio_init_card()
  mmc: sdio: Re-use negotiated OCR mask when re-sending CMD8
  mmc: sdio: Align the initialization commands in retry path for UHS-I

 drivers/mmc/core/sdio.c | 116 +++++++++++++++++++---------------------
 1 file changed, 55 insertions(+), 61 deletions(-)

-- 
2.20.1

[PATCH 1/4] mmc: sdio: Fix potential NULL pointer error in mmc_sdio_init_card()

[Ulf Hansson]

During some scenarios mmc_sdio_init_card() runs a retry path for the UHS-I
specific initialization, which leads to removal of the previously allocated
card. A new card is then re-allocated while retrying.

However, in one of the corresponding error paths we may end up to remove an
already removed card, which likely leads to a NULL pointer exception. So,
let's fix this.

Fixes: 5fc3d80ef496 ("mmc: sdio: don't use rocr to check if the card could support UHS mode")
Cc: <stable@vger.kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index ebb387aa5158..d35679e6e6aa 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -718,9 +718,8 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 			/* Retry init sequence, but without R4_18V_PRESENT. */
 			retries = 0;
 			goto try_again;
-		} else {
-			goto remove;
 		}
+		return err;
 	}
 
 	/*
-- 
2.20.1

[PATCH 2/4] mmc: sdio: Fix several potential memory leaks in mmc_sdio_init_card()

[Ulf Hansson]

Over the years, the code in mmc_sdio_init_card() has grown to become quite
messy. Unfortunate this has also lead to that several paths are leaking
memory in form of an allocated struct mmc_card, which includes additional
data, such as initialized struct device for example.

Unfortunate, it's a too complex task find each offending commit. Therefore,
this change fixes all memory leaks at once.

Cc: <stable@vger.kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 58 +++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index d35679e6e6aa..20eed28ea60d 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -584,7 +584,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	 */
 	err = mmc_send_io_op_cond(host, ocr, &rocr);
 	if (err)
-		goto err;
+		return err;
 
 	/*
 	 * For SPI, enable CRC as appropriate.
@@ -592,17 +592,15 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	if (mmc_host_is_spi(host)) {
 		err = mmc_spi_set_crc(host, use_spi_crc);
 		if (err)
-			goto err;
+			return err;
 	}
 
 	/*
 	 * Allocate card structure.
 	 */
 	card = mmc_alloc_card(host, NULL);
-	if (IS_ERR(card)) {
-		err = PTR_ERR(card);
-		goto err;
-	}
+	if (IS_ERR(card))
+		return PTR_ERR(card);
 
 	if ((rocr & R4_MEMORY_PRESENT) &&
 	    mmc_sd_get_cid(host, ocr & rocr, card->raw_cid, NULL) == 0) {
@@ -610,19 +608,15 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 
 		if (oldcard && (oldcard->type != MMC_TYPE_SD_COMBO ||
 		    memcmp(card->raw_cid, oldcard->raw_cid, sizeof(card->raw_cid)) != 0)) {
-			mmc_remove_card(card);
-			pr_debug("%s: Perhaps the card was replaced\n",
-				mmc_hostname(host));
-			return -ENOENT;
+			err = -ENOENT;
+			goto mismatch;
 		}
 	} else {
 		card->type = MMC_TYPE_SDIO;
 
 		if (oldcard && oldcard->type != MMC_TYPE_SDIO) {
-			mmc_remove_card(card);
-			pr_debug("%s: Perhaps the card was replaced\n",
-				mmc_hostname(host));
-			return -ENOENT;
+			err = -ENOENT;
+			goto mismatch;
 		}
 	}
 
@@ -677,7 +671,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	if (!oldcard && card->type == MMC_TYPE_SD_COMBO) {
 		err = mmc_sd_get_csd(host, card);
 		if (err)
-			return err;
+			goto remove;
 
 		mmc_decode_cid(card);
 	}
@@ -704,7 +698,12 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 			mmc_set_timing(card->host, MMC_TIMING_SD_HS);
 		}
 
-		goto finish;
+		if (oldcard)
+			mmc_remove_card(card);
+		else
+			host->card = card;
+
+		return 0;
 	}
 
 	/*
@@ -730,16 +729,14 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 		goto remove;
 
 	if (oldcard) {
-		int same = (card->cis.vendor == oldcard->cis.vendor &&
-			    card->cis.device == oldcard->cis.device);
-		mmc_remove_card(card);
-		if (!same) {
-			pr_debug("%s: Perhaps the card was replaced\n",
-				mmc_hostname(host));
-			return -ENOENT;
+		if (card->cis.vendor == oldcard->cis.vendor &&
+		    card->cis.device == oldcard->cis.device) {
+			mmc_remove_card(card);
+			card = oldcard;
+		} else {
+			err = -ENOENT;
+			goto mismatch;
 		}
-
-		card = oldcard;
 	}
 	card->ocr = ocr_card;
 	mmc_fixup_device(card, sdio_fixup_methods);
@@ -800,16 +797,15 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 		err = -EINVAL;
 		goto remove;
 	}
-finish:
-	if (!oldcard)
-		host->card = card;
+
+	host->card = card;
 	return 0;
 
+mismatch:
+	pr_debug("%s: Perhaps the card was replaced\n", mmc_hostname(host));
 remove:
-	if (!oldcard)
+	if (oldcard != card)
 		mmc_remove_card(card);
-
-err:
 	return err;
 }
 
-- 
2.20.1

[PATCH 3/4] mmc: sdio: Re-use negotiated OCR mask when re-sending CMD8

[Ulf Hansson]

While initializing an SDIO card in mmc_sdio_init_card(), we may need to
retry the UHS-I specific initialization, in case the first attempt fails.

This leads to resending a CMD8, but also to restart from scratch with the
so called OCR mask negotiations. This is unnecessary as we already have a
negotiated OCR mask, so let's use that instead. In this way, the behaviour
also becomes more consistent with other similar paths.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 20eed28ea60d..853ac65f0485 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -543,12 +543,12 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
 	return err;
 }
 
-static void mmc_sdio_resend_if_cond(struct mmc_host *host,
+static void mmc_sdio_resend_if_cond(struct mmc_host *host, u32 ocr,
 				    struct mmc_card *card)
 {
 	sdio_reset(host);
 	mmc_go_idle(host);
-	mmc_send_if_cond(host, host->ocr_avail);
+	mmc_send_if_cond(host, ocr);
 	mmc_remove_card(card);
 }
 
@@ -640,7 +640,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	if (rocr & ocr & R4_18V_PRESENT) {
 		err = mmc_set_uhs_voltage(host, ocr_card);
 		if (err == -EAGAIN) {
-			mmc_sdio_resend_if_cond(host, card);
+			mmc_sdio_resend_if_cond(host, ocr_card, card);
 			retries--;
 			goto try_again;
 		} else if (err) {
@@ -712,7 +712,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	 */
 	err = sdio_read_cccr(card, ocr);
 	if (err) {
-		mmc_sdio_resend_if_cond(host, card);
+		mmc_sdio_resend_if_cond(host, ocr_card, card);
 		if (ocr & R4_18V_PRESENT) {
 			/* Retry init sequence, but without R4_18V_PRESENT. */
 			retries = 0;
-- 
2.20.1

[PATCH 4/4] mmc: sdio: Align the initialization commands in retry path for UHS-I

[Ulf Hansson]

According to the comment in mmc_sdio_reinit_card(), some SDIO cards may
require a "[CMD5,5,3,7] init sequence", which isn't always obeyed in
mmc_sdio_init_card(). Especially, when we end up retrying the UHS-I
specific initialization, there is a missing CMD5.

Let's update the code to make the behaviour consistent and let's also take
the opportunity to clean up the code a bit, to avoid open coding.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 53 ++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 853ac65f0485..435de47a6ee0 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -543,13 +543,33 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
 	return err;
 }
 
-static void mmc_sdio_resend_if_cond(struct mmc_host *host, u32 ocr,
-				    struct mmc_card *card)
+static int mmc_sdio_pre_init(struct mmc_host *host, u32 ocr,
+			     struct mmc_card *card)
 {
+	if (card)
+		mmc_remove_card(card);
+
+	/*
+	 * Reset the card by performing the same steps that are taken by
+	 * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe.
+	 *
+	 * sdio_reset() is technically not needed. Having just powered up the
+	 * hardware, it should already be in reset state. However, some
+	 * platforms (such as SD8686 on OLPC) do not instantly cut power,
+	 * meaning that a reset is required when restoring power soon after
+	 * powering off. It is harmless in other cases.
+	 *
+	 * The CMD5 reset (mmc_send_io_op_cond()), according to the SDIO spec,
+	 * is not necessary for non-removable cards. However, it is required
+	 * for OLPC SD8686 (which expects a [CMD5,5,3,7] init sequence), and
+	 * harmless in other situations.
+	 *
+	 */
+
 	sdio_reset(host);
 	mmc_go_idle(host);
 	mmc_send_if_cond(host, ocr);
-	mmc_remove_card(card);
+	return mmc_send_io_op_cond(host, 0, NULL);
 }
 
 /*
@@ -640,7 +660,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	if (rocr & ocr & R4_18V_PRESENT) {
 		err = mmc_set_uhs_voltage(host, ocr_card);
 		if (err == -EAGAIN) {
-			mmc_sdio_resend_if_cond(host, ocr_card, card);
+			mmc_sdio_pre_init(host, ocr_card, card);
 			retries--;
 			goto try_again;
 		} else if (err) {
@@ -712,7 +732,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	 */
 	err = sdio_read_cccr(card, ocr);
 	if (err) {
-		mmc_sdio_resend_if_cond(host, ocr_card, card);
+		mmc_sdio_pre_init(host, ocr_card, card);
 		if (ocr & R4_18V_PRESENT) {
 			/* Retry init sequence, but without R4_18V_PRESENT. */
 			retries = 0;
@@ -813,28 +833,7 @@ static int mmc_sdio_reinit_card(struct mmc_host *host)
 {
 	int ret;
 
-	/*
-	 * Reset the card by performing the same steps that are taken by
-	 * mmc_rescan_try_freq() and mmc_attach_sdio() during a "normal" probe.
-	 *
-	 * sdio_reset() is technically not needed. Having just powered up the
-	 * hardware, it should already be in reset state. However, some
-	 * platforms (such as SD8686 on OLPC) do not instantly cut power,
-	 * meaning that a reset is required when restoring power soon after
-	 * powering off. It is harmless in other cases.
-	 *
-	 * The CMD5 reset (mmc_send_io_op_cond()), according to the SDIO spec,
-	 * is not necessary for non-removable cards. However, it is required
-	 * for OLPC SD8686 (which expects a [CMD5,5,3,7] init sequence), and
-	 * harmless in other situations.
-	 *
-	 */
-
-	sdio_reset(host);
-	mmc_go_idle(host);
-	mmc_send_if_cond(host, host->card->ocr);
-
-	ret = mmc_send_io_op_cond(host, 0, NULL);
+	ret = mmc_sdio_pre_init(host, host->card->ocr, NULL);
 	if (ret)
 		return ret;
 
-- 
2.20.1

Re: [PATCH 2/4] mmc: sdio: Fix several potential memory leaks in mmc_sdio_init_card()

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.6.7, v5.4.35, v4.19.118, v4.14.177, v4.9.220, v4.4.220.

v5.6.7: Build OK!
v5.4.35: Build OK!
v4.19.118: Failed to apply! Possible dependencies:
    099b64811609 ("mmc: core: Add a debug print when the card may have been replaced")
    3c30e73977e5 ("mmc: sdio: Drop unused in-parameter to mmc_sdio_reinit_card()")
    4aaaf3ab1509 ("mmc: sdio: Drop unused in-parameter from mmc_sdio_init_card()")
    6ebc581c3f9e ("mmc: sdio: Don't re-initialize powered-on removable SDIO cards at resume")
    7fbbe725378d ("mmc: sdio: Drop powered-on re-init at runtime resume and HW reset")

v4.14.177: Failed to apply! Possible dependencies:
    099b64811609 ("mmc: core: Add a debug print when the card may have been replaced")
    247cfe535575 ("mmc: core: Add capability to avoid 3.3V signaling")
    3a3db6030b64 ("mmc: core: Rename ->reset() bus ops to ->hw_reset()")
    3c30e73977e5 ("mmc: sdio: Drop unused in-parameter to mmc_sdio_reinit_card()")
    4aaaf3ab1509 ("mmc: sdio: Drop unused in-parameter from mmc_sdio_init_card()")
    6a11fc47f175 ("mmc: sd: Fix signal voltage when there is no power cycle")
    6ebc581c3f9e ("mmc: sdio: Don't re-initialize powered-on removable SDIO cards at resume")
    7405df4c79cd ("mmc: core: Implement ->sw_reset bus ops for SDIO")
    7fbbe725378d ("mmc: sdio: Drop powered-on re-init at runtime resume and HW reset")
    f690f4409ddd ("mmc: mmc: Enable CQE's")
    fb09f44e290b ("mmc: core: Re-factor some code for SDIO re-initialization")

v4.9.220: Failed to apply! Possible dependencies:
    066185d69063 ("mmc: core: First step in cleaning up private mmc header files")
    099b64811609 ("mmc: core: Add a debug print when the card may have been replaced")
    20348d1981da ("mmc: core: Make mmc_switch_status() available for mmc core")
    247cfe535575 ("mmc: core: Add capability to avoid 3.3V signaling")
    2ed573b603f7 ("mmc: core: Clarify usage of mmc_set_signal_voltage()")
    3a3db6030b64 ("mmc: core: Rename ->reset() bus ops to ->hw_reset()")
    3c30e73977e5 ("mmc: sdio: Drop unused in-parameter to mmc_sdio_reinit_card()")
    437590a123b6 ("mmc: core: Retry instead of ignore at CRC errors when polling for busy")
    4aaaf3ab1509 ("mmc: sdio: Drop unused in-parameter from mmc_sdio_init_card()")
    4facdde11394 ("mmc: core: Move public functions from card.h to private headers")
    55244c5659b5 ("mmc: core: Move public functions from core.h to private headers")
    625228fa3e01 ("mmc: core: Rename ignore_crc to retry_crc_err to reflect its purpose")
    6ebc581c3f9e ("mmc: sdio: Don't re-initialize powered-on removable SDIO cards at resume")
    70562644f4ee ("mmc: core: Don't use ->card_busy() and CMD13 in combination when polling")
    716bdb8953c7 ("mmc: core: Factor out code related to polling in __mmc_switch()")
    7405df4c79cd ("mmc: core: Implement ->sw_reset bus ops for SDIO")
    7fbbe725378d ("mmc: sdio: Drop powered-on re-init at runtime resume and HW reset")
    9d4579a85c84 ("mmc: mmc_test: Disable Command Queue while mmc_test is used")
    aa33ce3c411a ("mmc: core: Enable __mmc_switch() to change bus speed timing for the host")
    cb26ce069ffa ("mmc: core: Clarify code which deals with polling in __mmc_switch()")
    f690f4409ddd ("mmc: mmc: Enable CQE's")
    fb09f44e290b ("mmc: core: Re-factor some code for SDIO re-initialization")

v4.4.220: Failed to apply! Possible dependencies:
    066185d69063 ("mmc: core: First step in cleaning up private mmc header files")
    099b64811609 ("mmc: core: Add a debug print when the card may have been replaced")
    247cfe535575 ("mmc: core: Add capability to avoid 3.3V signaling")
    29eb7bd01e80 ("mmc: card: do away with indirection pointer")
    2ed573b603f7 ("mmc: core: Clarify usage of mmc_set_signal_voltage()")
    3a3db6030b64 ("mmc: core: Rename ->reset() bus ops to ->hw_reset()")
    3c30e73977e5 ("mmc: sdio: Drop unused in-parameter to mmc_sdio_reinit_card()")
    48ab086d262e ("mmc: block: add missing header dependencies")
    4aaaf3ab1509 ("mmc: sdio: Drop unused in-parameter from mmc_sdio_init_card()")
    4e6c71788d6b ("mmc: core: Do regular power cycle when lacking eMMC HW reset support")
    4facdde11394 ("mmc: core: Move public functions from card.h to private headers")
    55244c5659b5 ("mmc: core: Move public functions from core.h to private headers")
    5b96fea730ab ("mmc: pwrseq_simple: add to_pwrseq_simple() macro")
    6ebc581c3f9e ("mmc: sdio: Don't re-initialize powered-on removable SDIO cards at resume")
    7405df4c79cd ("mmc: core: Implement ->sw_reset bus ops for SDIO")
    7fbbe725378d ("mmc: sdio: Drop powered-on re-init at runtime resume and HW reset")
    81f351615772 ("xen/blkfront: separate per ring information out of device info")
    9d4579a85c84 ("mmc: mmc_test: Disable Command Queue while mmc_test is used")
    c2df40dfb8c0 ("drivers: use req op accessor")
    d97a1e5d7cd2 ("mmc: pwrseq: convert to proper platform device")
    f01b72d0fd53 ("mmc: pwrseq_emmc: add to_pwrseq_emmc() macro")
    f690f4409ddd ("mmc: mmc: Enable CQE's")
    fb09f44e290b ("mmc: core: Re-factor some code for SDIO re-initialization")
    ffedbd2210f2 ("mmc: pwrseq: constify mmc_pwrseq_ops structures")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha