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 <[email protected]>
>> Cc: Rabin Vincent <[email protected]>
>> Signed-off-by: Linus Walleij <[email protected]>
>> ---
>> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
