mmc: core: not to --qty when calculate timeout for SECURE_ERASE

PATCH In review Chuanxiao Dong 2 participants 4 messages Apr 13, 2012 – May 21, 2012 MMC, SD and SDIO

Share & tools

Chuanxiao Dong

author

2012-04-13 04:25 UTC #0 lore raw

PATCH 1/1 mmc: core: not to --qty when calculate timeout for SECURE_ERASE

--qty when calculating erase timeout for trim/erase & secure trim/erase can prevent the erase range crossing qty+1 erase groups, which made the final timeout value is too large for the host.

When operate SECURE_ERASE, driver needs the erase range is aligned with erase size, otherwise do nothing and return an error. That is to say it is not necessary for SECURE_ERASE to --qty since it will never cross an erase group.

Signed-off-by Chuanxiao Dong <[email protected]>

---
 drivers/mmc/core/core.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

drivers/mmc/core/core.c +8 -1

index e541efb..b5a393a 100644
@@ -1761,7 +1761,7 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
 	if (!qty)
 		return 0;
 
-	if (qty == 1)
+	if (qty == 1 && arg != MMC_SECURE_ERASE_ARG)
 		return 1;
 
 	/* Convert qty to sectors */

@@ -1772,6 +1772,13 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
 	else
 		max_discard = --qty * card->erase_size;
 
+	/*
+	 * since SECURE_ERASE is erase group aligned, otherwise
+	 * it cannot be erased in secure purpose, needn't --qty
+	 */
+	if (arg == MMC_SECURE_ERASE_ARG)
+		max_discard += card->erase_size;
+
 	return max_discard;
 }

Signature · 1 line

1.7.1

Adrian Hunter

2012-05-21 11:05 UTC #1 lore raw

Quoted 19 lines · Chuanxiao Dong

--qty when calculating erase timeout for trim/erase & secure trim/erase can prevent the erase range crossing qty+1 erase groups, which made the final timeout value is too large for the host.
When operate SECURE_ERASE, driver needs the erase range is aligned with erase size, otherwise do nothing and return an error. That is to say it is not necessary for SECURE_ERASE to --qty since it will never cross an erase group.
Signed-off-by Chuanxiao Dong <[email protected]>
---
 drivers/mmc/core/core.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
drivers/mmc/core/core.c +0 -0
index e541efb..b5a393a 100644

drivers/mmc/core/core.c static unsigned int mmc_do_calc_max_discard(struct mmc_card *card, · line 1765

Quoted 6 lines

drivers/mmc/core/core.c +1 -1

@@ -1761,7 +1761,7 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
 	if (!qty)
 		return 0;
 
-	if (qty == 1)
+	if (qty == 1 && arg != MMC_SECURE_ERASE_ARG)
 		return 1;

arg is never MMC_SECURE_ERASE_ARG

Quoted 6 lines

drivers/mmc/core/core.c +0 -0

 	/* Convert qty to sectors */

@@ -1772,6 +1772,13 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
 	else
 		max_discard = --qty * card->erase_size;

drivers/mmc/core/core.c static unsigned int mmc_do_calc_max_discard(struct mmc_card *card, · line 1784

Quoted 10 lines

drivers/mmc/core/core.c +7 -0

+	/*
+	 * since SECURE_ERASE is erase group aligned, otherwise
+	 * it cannot be erased in secure purpose, needn't --qty
+	 */
+	if (arg == MMC_SECURE_ERASE_ARG)
+		max_discard += card->erase_size;
+
 	return max_discard;
 }

What about:

From: Adrian Hunter <[email protected]>
Date: Mon, 21 May 2012 13:32:42 +0300
Subject: [PATCH] mmc: core: fix max_discard calculation

The maximum discard calculation was unnecessarily pessimistic in the case of erasing entire erase groups. In that case, the quantity does not need to be decreased by 1 to allow for misalignment because the erasure is always aligned to whole erase groups.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/core/core.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

drivers/mmc/core/core.c +17 -5

index 0b6141d..36bfdce 100644
@@ -1742,7 +1742,7 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
 {
 	struct mmc_host *host = card->host;
 	unsigned int max_discard, x, y, qty = 0, max_qty, timeout;
-	unsigned int last_timeout = 0;
+	unsigned int last_timeout = 0, aligned_qty;
 
 	if (card->erase_shift)
 		max_qty = UINT_MAX >> card->erase_shift;

@@ -1769,16 +1769,28 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
 	if (!qty)
 		return 0;
 
-	if (qty == 1)
-		return 1;
+	if (arg & MMC_TRIM_ARGS) {
+		/*
+		 * The requested number of sectors may not be aligned to an
+		 * erase group, so we have to decrease the quantity by 1 (unless
+		 * it is 1) e.g. trimming 2 sectors could cause 2 erase groups
+		 * to be affected even though 2 sectors is less than the size of
+		 * 1 erase group.
+		 */
+		if (qty == 1)
+			return 1;
+		aligned_qty = qty - 1;
+	} else {
+		aligned_qty = qty;
+	}
 
 	/* Convert qty to sectors */
 	if (card->erase_shift)
-		max_discard = --qty << card->erase_shift;
+		max_discard = aligned_qty << card->erase_shift;
 	else if (mmc_card_sd(card))
 		max_discard = qty;
 	else
-		max_discard = --qty * card->erase_size;
+		max_discard = aligned_qty * card->erase_size;
 
 	return max_discard;
 }

Signature · 1 line

1.7.6.5

Dong, Chuanxiao

author

2012-05-21 11:19 UTC #2 lore raw

Quoted 110 lines

-----Original Message-----
From: Hunter, Adrian
Sent: Monday, May 21, 2012 7:06 PM
To: Dong, Chuanxiao
Cc: [email protected]; [email protected]
Subject: Re: [PATCH]mmc: core: not to --qty when calculate timeout for SECURE_ERASE
drivers/mmc/core/core.c static unsigned int mmc_do_calc_max_discard(struct
Quoted 19 lines · Chuanxiao Dong
--qty when calculating erase timeout for trim/erase & secure trim/erase can prevent the erase range crossing qty+1 erase groups, which made the final timeout value is too large for the host.
When operate SECURE_ERASE, driver needs the erase range is aligned with erase size, otherwise do nothing and return an error. That is to say it is not necessary for SECURE_ERASE to --qty since it will never cross an erase group.
Signed-off-by Chuanxiao Dong <[email protected]>
---
 drivers/mmc/core/core.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
drivers/mmc/core/core.c index renamed from drivers/mmc/core/core.c +0 -0
e541efb..b5a393a 100644
drivers/mmc/core/core.c +0 -0
@@ -1761,7 +1761,7 @@ static unsigned int mmc_do_calc_max_discard(struct
mmc_card *card,
drivers/mmc/core/core.c static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
Quoted 6 lines
drivers/mmc/core/core.c +1 -1
 	if (!qty)
 		return 0;
 
-	if (qty == 1)
+	if (qty == 1 && arg != MMC_SECURE_ERASE_ARG)
 		return 1;
arg is never MMC_SECURE_ERASE_ARG
drivers/mmc/core/core.c static unsigned int mmc_do_calc_max_discard(struct
Quoted 3 lines
drivers/mmc/core/core.c +0 -0
 	/* Convert qty to sectors */
@@ -1772,6 +1772,13 @@ static unsigned int mmc_do_calc_max_discard(struct
mmc_card *card,
drivers/mmc/core/core.c static unsigned int mmc_do_calc_max_discard(struct · line 1784
Quoted 13 lines
drivers/mmc/core/core.c +7 -0
 	else
 		max_discard = --qty * card->erase_size;
 
+	/*
+	 * since SECURE_ERASE is erase group aligned, otherwise
+	 * it cannot be erased in secure purpose, needn't --qty
+	 */
+	if (arg == MMC_SECURE_ERASE_ARG)
+		max_discard += card->erase_size;
+
 	return max_discard;
 }
 
What about:
From: Adrian Hunter <[email protected]>
Date: Mon, 21 May 2012 13:32:42 +0300
Subject: [PATCH] mmc: core: fix max_discard calculation
The maximum discard calculation was unnecessarily pessimistic in the case of erasing entire erase groups. In that case, the quantity does not need to be decreased by 1 to allow for misalignment because the erasure is always aligned to whole erase groups.
Signed-off-by Adrian Hunter <[email protected]>
---
 drivers/mmc/core/core.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)
drivers/mmc/core/core.c index renamed from drivers/mmc/core/core.c +0 -0
0b6141d..36bfdce 100644
drivers/mmc/core/core.c +1 -1
@@ -1742,7 +1742,7 @@ static unsigned int mmc_do_calc_max_discard(struct
 mmc_card *card,  {
 	struct mmc_host *host = card->host;
 	unsigned int max_discard, x, y, qty = 0, max_qty, timeout;
-	unsigned int last_timeout = 0;
+	unsigned int last_timeout = 0, aligned_qty;
 
 	if (card->erase_shift)
 		max_qty = UINT_MAX >> card->erase_shift; @@ -1769,16 +1769,28 @@
static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
drivers/mmc/core/core.c +14 -2
 	if (!qty)
 		return 0;
 
-	if (qty == 1)
-		return 1;
+	if (arg & MMC_TRIM_ARGS) {
+		/*
+		 * The requested number of sectors may not be aligned to an
+		 * erase group, so we have to decrease the quantity by 1 (unless
+		 * it is 1) e.g. trimming 2 sectors could cause 2 erase groups
+		 * to be affected even though 2 sectors is less than the size of
+		 * 1 erase group.
+		 */
+		if (qty == 1)
+			return 1;
+		aligned_qty = qty - 1;
+	} else {
+		aligned_qty = qty;
+	}
 
 	/* Convert qty to sectors */
 	if (card->erase_shift)

drivers/mmc/core/core.c static unsigned int mmc_do_calc_max_discard(struct mmc_card *card, · line 1796

Quoted 10 lines

drivers/mmc/core/core.c +2 -2

-		max_discard = --qty << card->erase_shift;
+		max_discard = aligned_qty << card->erase_shift;
 	else if (mmc_card_sd(card))
 		max_discard = qty;
 	else
-		max_discard = --qty * card->erase_size;
+		max_discard = aligned_qty * card->erase_size;
 
 	return max_discard;
 }

Hi Hunter,
Your patch looks good to me.

Since you also mentioned that the arg will never be MMC_SECURE_ERASE_ARG, I want to know why not calculate erase size for secure trim/erase operations? As specification said, secure trim/erase operations has different timeout value with trim/erase.

Thanks
Chuanxiao

Adrian Hunter

2012-05-21 11:27 UTC #3 lore raw

Quoted 118 lines · Chuanxiao

Quoted 118 lines
-----Original Message-----
From: Hunter, Adrian
Sent: Monday, May 21, 2012 7:06 PM
To: Dong, Chuanxiao
Cc: [email protected]; [email protected]
Subject: Re: [PATCH]mmc: core: not to --qty when calculate timeout for SECURE_ERASE
drivers/mmc/core/core.c static unsigned int mmc_do_calc_max_discard(struct
Quoted 19 lines · Chuanxiao Dong
--qty when calculating erase timeout for trim/erase & secure trim/erase can prevent the erase range crossing qty+1 erase groups, which made the final timeout value is too large for the host.
When operate SECURE_ERASE, driver needs the erase range is aligned with erase size, otherwise do nothing and return an error. That is to say it is not necessary for SECURE_ERASE to --qty since it will never cross an erase group.
Signed-off-by Chuanxiao Dong <[email protected]>
---
 drivers/mmc/core/core.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
drivers/mmc/core/core.c index renamed from drivers/mmc/core/core.c +0 -0
e541efb..b5a393a 100644
drivers/mmc/core/core.c +0 -0
@@ -1761,7 +1761,7 @@ static unsigned int mmc_do_calc_max_discard(struct
mmc_card *card,
drivers/mmc/core/core.c static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
Quoted 6 lines
drivers/mmc/core/core.c +1 -1
 	if (!qty)
 		return 0;
 
-	if (qty == 1)
+	if (qty == 1 && arg != MMC_SECURE_ERASE_ARG)
 		return 1;
arg is never MMC_SECURE_ERASE_ARG
drivers/mmc/core/core.c static unsigned int mmc_do_calc_max_discard(struct
Quoted 3 lines
drivers/mmc/core/core.c +0 -0
 	/* Convert qty to sectors */
@@ -1772,6 +1772,13 @@ static unsigned int mmc_do_calc_max_discard(struct
mmc_card *card,
drivers/mmc/core/core.c static unsigned int mmc_do_calc_max_discard(struct · line 1784
Quoted 13 lines
drivers/mmc/core/core.c +7 -0
 	else
 		max_discard = --qty * card->erase_size;
 
+	/*
+	 * since SECURE_ERASE is erase group aligned, otherwise
+	 * it cannot be erased in secure purpose, needn't --qty
+	 */
+	if (arg == MMC_SECURE_ERASE_ARG)
+		max_discard += card->erase_size;
+
 	return max_discard;
 }
 
What about:
From: Adrian Hunter <[email protected]>
Date: Mon, 21 May 2012 13:32:42 +0300
Subject: [PATCH] mmc: core: fix max_discard calculation
The maximum discard calculation was unnecessarily pessimistic in the case of erasing entire erase groups. In that case, the quantity does not need to be decreased by 1 to allow for misalignment because the erasure is always aligned to whole erase groups.
Signed-off-by Adrian Hunter <[email protected]>
---
 drivers/mmc/core/core.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)
drivers/mmc/core/core.c index renamed from drivers/mmc/core/core.c +0 -0
0b6141d..36bfdce 100644
drivers/mmc/core/core.c +1 -1
@@ -1742,7 +1742,7 @@ static unsigned int mmc_do_calc_max_discard(struct
 mmc_card *card,  {
 	struct mmc_host *host = card->host;
 	unsigned int max_discard, x, y, qty = 0, max_qty, timeout;
-	unsigned int last_timeout = 0;
+	unsigned int last_timeout = 0, aligned_qty;
 
 	if (card->erase_shift)
 		max_qty = UINT_MAX >> card->erase_shift; @@ -1769,16 +1769,28 @@
static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
drivers/mmc/core/core.c +16 -4
 	if (!qty)
 		return 0;
 
-	if (qty == 1)
-		return 1;
+	if (arg & MMC_TRIM_ARGS) {
+		/*
+		 * The requested number of sectors may not be aligned to an
+		 * erase group, so we have to decrease the quantity by 1 (unless
+		 * it is 1) e.g. trimming 2 sectors could cause 2 erase groups
+		 * to be affected even though 2 sectors is less than the size of
+		 * 1 erase group.
+		 */
+		if (qty == 1)
+			return 1;
+		aligned_qty = qty - 1;
+	} else {
+		aligned_qty = qty;
+	}
 
 	/* Convert qty to sectors */
 	if (card->erase_shift)
-		max_discard = --qty << card->erase_shift;
+		max_discard = aligned_qty << card->erase_shift;
 	else if (mmc_card_sd(card))
 		max_discard = qty;
 	else
-		max_discard = --qty * card->erase_size;
+		max_discard = aligned_qty * card->erase_size;

Quoted 7 lines

Quoted 2 lines
return max_discard;
}
Hi Hunter,
Your patch looks good to me.
Since you also mentioned that the arg will never be MMC_SECURE_ERASE_ARG, I want to know why not calculate erase size for secure trim/erase operations? As specification said, secure trim/erase operations has different timeout value with trim/erase.

There are 2 problems. First, there is only 1 value for maximum discard whether secure or not. Secondly, the timeout for secure erase can be so great that any quantity exceeds the maximum timeout.

Quoted 5 lines

Thanks
Chuanxiao