Re: [PATCH] mmc: card: move variable initialization earlier

[Adrian Hunter <adrian.hunter@intel.com>]

On 01/04/12 07:07, Chris Ball wrote:
> Hi Adrian,
> 
> On Fri, Mar 23 2012, Linus Walleij wrote:
>> I was pretty tired of seeing these in my kernel compiles:
>>
>> drivers/mmc/card/block.c: In function ‘mmc_blk_issue_secdiscard_rq’:
>> drivers/mmc/card/block.c:911:18: warning: ‘arg’ may be used uninitialized in this function [-Wuninitialized]
>> drivers/mmc/card/block.c:910:6: warning: ‘nr’ may be used uninitialized in this function [-Wuninitialized]
>> drivers/mmc/card/block.c:910:6: warning: ‘from’ may be used uninitialized in this function [-Wuninitialized]
>>
>> The problem stems from the code path in
>> mmc_blk_issue_secdiscard_rq() where mmc_switch()
>> with EXT_CSD_SANITIZE_START may return -EIO and fall back
>> to using the default trim operations instead. At this point
>> the variables needed for the fallback will be uninitialized.
>>
>> Cc: Ulf Hansson <ulf.hansson@stericsson.com>
>> Cc: Rabin Vincent <rabin@rab.in>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> I don't know if this is the actual intention - maybe we
>> should just fail the call entirely if the sanitize command
>> fails?
> 
> I think you (Adrian) introduced this "goto out->goto retry" logic in
> upstream commit 67716327eec7e9 -- please could you take a look here?
> 

The sanitize logic looks wrong to me.  I would expect it to look
like this:


diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index b180965..f5e0534 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -881,17 +881,12 @@ static int mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
 		goto out;
 	}

-	/* The sanitize operation is supported at v4.5 only */
-	if (mmc_can_sanitize(card)) {
-		err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-				EXT_CSD_SANITIZE_START, 1, 0);
-		goto out;
-	}
-
 	from = blk_rq_pos(req);
 	nr = blk_rq_sectors(req);

-	if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr))
+	if (mmc_can_sanitize(card))
+		arg = MMC_DISCARD_ARG;
+	else if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr))
 		arg = MMC_SECURE_TRIM1_ARG;
 	else
 		arg = MMC_SECURE_ERASE_ARG;
@@ -918,6 +913,12 @@ retry:
 		}
 		err = mmc_erase(card, from, nr, MMC_SECURE_TRIM2_ARG);
 	}
+
+	/* The sanitize operation is supported at v4.5 only */
+	if (!err && mmc_can_sanitize(card)) {
+		err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+				EXT_CSD_SANITIZE_START, 1, 0);
+	}
 out:
 	if (err == -EIO && !mmc_blk_reset(md, card->host, type))
 		goto retry;



Also the timeout for eMMC v4.5 DISCARD looks wrong.  It should be
the same as TRIM:


diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 14f262e..00fd7db 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1407,7 +1407,7 @@ static unsigned int mmc_mmc_erase_timeout(struct mmc_card *card,

 	if (card->ext_csd.erase_group_def & 1) {
 		/* High Capacity Erase Group Size uses HC timeouts */
-		if (arg == MMC_TRIM_ARG)
+		if (arg == MMC_TRIM_ARG || arg == MMC_DISCARD_ARG)
 			erase_timeout = card->ext_csd.trim_timeout;
 		else
 			erase_timeout = card->ext_csd.hc_erase_timeout;



In addition eMMC v4.5 seems to indicate the use of the trim timeout
irrespective of the setting of erase_group_def, so maybe it should be
like this:



diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 14f262e..4691a23 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1405,7 +1405,10 @@ static unsigned int mmc_mmc_erase_timeout(struct mmc_card *card,
 {
 	unsigned int erase_timeout;

-	if (card->ext_csd.erase_group_def & 1) {
+	if (arg == MMC_DISCARD_ARG ||
+	    (arg == MMC_TRIM_ARG && card->ext_csd.rev >= 6)) {
+		erase_timeout = card->ext_csd.trim_timeout;
+	} else if (card->ext_csd.erase_group_def & 1) {
 		/* High Capacity Erase Group Size uses HC timeouts */
 		if (arg == MMC_TRIM_ARG)
 			erase_timeout = card->ext_csd.trim_timeout;




Alternatively, maybe it would be better to switch to HC erase size for all
eMMC v4.5 cards?