Discussion:
[PATCH] mmc: core: don't return 1 for max_discard
Stephen Warren
2013-12-18 22:27:43 UTC
Permalink
From: Stephen Warren <***@nvidia.com>

In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.

Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.

Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.

Cc: Adrian Hunter <***@intel.com>
Cc: Dong Aisheng <***@gmail.com>
Cc: Ulf Hansson <***@linaro.org>
Cc: Vladimir Zapolskiy <***@mleia.com>
Fixes: e056a1b5b67b "(mmc: queue: let host controllers specify maximum discard timeout")
Signed-off-by: Stephen Warren <***@nvidia.com>
---
drivers/mmc/core/core.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 57a2b403bf8e..eb952ca634ea 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2150,8 +2150,25 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
if (!qty)
return 0;

- if (qty == 1)
- return 1;
+ /*
+ * Discard operations may not be aligned to an erase block. In an
+ * unaligned case, we need to issue 1 more discard operation to HW
+ * than strictly calculated by:
+ * sectors_to_erase / sectors_per_discard_operation
+ *
+ * To account for this in the timeout calculations, we assume we can
+ * actually discard one less erase block than fits into the HW
+ * timeout. This explains the --qty below.
+ *
+ * When only a single discard block operation fits into the timeout,
+ * disallow any discard operations at all. For example, discarding one
+ * sector at a time is so chronically slow as to be useless. However,
+ * make an exception for SD cards without an erase shift, since qty
+ * isn't multiplied up by an erase block size in the code below for
+ * that case.
+ */
+ if (qty == 1 && !(!card->erase_shift && mmc_card_sd(card)))
+ return 0;

/* Convert qty to sectors */
if (card->erase_shift)
--
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephen Warren
2013-12-18 23:00:22 UTC
Permalink
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host, &cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
Vladimir Zapolskiy
2013-12-19 08:22:02 UTC
Permalink
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host,&cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
Personally I'd prefer this change.

Out of curiosity have you tried the approach I proposed as RFC with set
mmc_core.limit_erase_groups=0?

With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Adrian Hunter
2013-12-19 09:01:19 UTC
Permalink
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host, &cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
But large erases will timeout when they should have been split into smaller
chunks.

A generic solution needs to be able to explain what happens when the host
controller *does* timeout.
Vladimir Zapolskiy
2013-12-19 09:14:31 UTC
Permalink
Post by Adrian Hunter
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host,&cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
But large erases will timeout when they should have been split into smaller
chunks.
A generic solution needs to be able to explain what happens when the host
controller *does* timeout.
Please correct me, but if Data Timeout Error is disabled, then this is not
an issue for most of the host controllers.

With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Adrian Hunter
2013-12-19 09:42:45 UTC
Permalink
Post by Vladimir Zapolskiy
Post by Adrian Hunter
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host,&cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
But large erases will timeout when they should have been split into smaller
chunks.
A generic solution needs to be able to explain what happens when the host
controller *does* timeout.
Please correct me, but if Data Timeout Error is disabled, then this is not
an issue for most of the host controllers.
That is a very good point. My experience with SDHCI was that masking the
"Data Timeout Error Status Enable" and "Data Timeout Error Signal Enable
" bits did not disable the timeout i.e. the host controller would not
deliver a TC interrupt if the erase exceeded the timeout.

What happens on your board?
Ulf Hansson
2013-12-19 10:26:49 UTC
Permalink
Post by Adrian Hunter
Post by Vladimir Zapolskiy
Post by Adrian Hunter
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host,&cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
But large erases will timeout when they should have been split into smaller
chunks.
A generic solution needs to be able to explain what happens when the host
controller *does* timeout.
Please correct me, but if Data Timeout Error is disabled, then this is not
an issue for most of the host controllers.
That is a very good point. My experience with SDHCI was that masking the
"Data Timeout Error Status Enable" and "Data Timeout Error Signal Enable
" bits did not disable the timeout i.e. the host controller would not
deliver a TC interrupt if the erase exceeded the timeout.
What happens on your board?
I posted a response yesterday for "[PATCH] mmc: core: don't decrement
qty when calculating max_discard", related to this. Please have a
look.

I think the interesting case to consider here is how we can handle
busy detection timeouts that is bigger than what the host hw can
support.

Option 1)
Should we tell the host to disable the timeout in this case? That
potentially means hanging forever - if the card misbehaves. Like
omap_hsmmc does for erase commands. Maybe that is an okay limitation?

Option 2)
Use a R1 response instead if R1B to prevent the host from doing busy
detection. Then rely on the CMD13 to poll for completion instead.
Obviously we can then stop polling after some selected timeout is the
card don't complete it's operations.

Would be very interesting to know what option you prefer!?

Kind regards
Uffe
Dong Aisheng
2013-12-19 11:18:55 UTC
Permalink
Post by Ulf Hansson
Post by Adrian Hunter
Post by Vladimir Zapolskiy
Post by Adrian Hunter
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host,&cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
But large erases will timeout when they should have been split into smaller
chunks.
A generic solution needs to be able to explain what happens when the host
controller *does* timeout.
Please correct me, but if Data Timeout Error is disabled, then this is not
an issue for most of the host controllers.
That is a very good point. My experience with SDHCI was that masking the
"Data Timeout Error Status Enable" and "Data Timeout Error Signal Enable
" bits did not disable the timeout i.e. the host controller would not
deliver a TC interrupt if the erase exceeded the timeout.
What happens on your board?
I posted a response yesterday for "[PATCH] mmc: core: don't decrement
qty when calculating max_discard", related to this. Please have a
look.
I think the interesting case to consider here is how we can handle
busy detection timeouts that is bigger than what the host hw can
support.
Option 1)
Should we tell the host to disable the timeout in this case? That
potentially means hanging forever - if the card misbehaves. Like
omap_hsmmc does for erase commands. Maybe that is an okay limitation?
Option 2)
Use a R1 response instead if R1B to prevent the host from doing busy
detection. Then rely on the CMD13 to poll for completion instead.
Obviously we can then stop polling after some selected timeout is the
card don't complete it's operations.
I proposed the same way before:
https://www.mail-archive.com/linux-***@vger.kernel.org/msg23757.html
Two concerns remain:

"1) not waiting for R1B seems a bit violation with standard spec.
Also it increase complexity on handling the R1B of the same command
for two different
cases: using hw timeout or polling status for CMD38.

2) In current implementation, the data size to erase will not exceed
the max_discard_bytes
which is calculated based on max_discard_to of host.
Then how do we specify max_discard_to if want to use polling? UNIT_MAX?
Will it be too long to affect other activities in the same system?"

That means we should erase a proper size of data in case it affects the system
a lot(e.g. two partitions on the same card, discard one partition may
cause the code
on another partition has no chance to run, it may be serious if the
file system is in it.).

Regards
Dong Aisheng
Post by Ulf Hansson
Would be very interesting to know what option you prefer!?
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Ulf Hansson
2013-12-19 13:04:14 UTC
Permalink
Post by Dong Aisheng
Post by Ulf Hansson
Post by Adrian Hunter
Post by Vladimir Zapolskiy
Post by Adrian Hunter
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host,&cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card,
arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
But large erases will timeout when they should have been split into smaller
chunks.
A generic solution needs to be able to explain what happens when the host
controller *does* timeout.
Please correct me, but if Data Timeout Error is disabled, then this is not
an issue for most of the host controllers.
That is a very good point. My experience with SDHCI was that masking the
"Data Timeout Error Status Enable" and "Data Timeout Error Signal Enable
" bits did not disable the timeout i.e. the host controller would not
deliver a TC interrupt if the erase exceeded the timeout.
What happens on your board?
I posted a response yesterday for "[PATCH] mmc: core: don't decrement
qty when calculating max_discard", related to this. Please have a
look.
I think the interesting case to consider here is how we can handle
busy detection timeouts that is bigger than what the host hw can
support.
Option 1)
Should we tell the host to disable the timeout in this case? That
potentially means hanging forever - if the card misbehaves. Like
omap_hsmmc does for erase commands. Maybe that is an okay limitation?
Option 2)
Use a R1 response instead if R1B to prevent the host from doing busy
detection. Then rely on the CMD13 to poll for completion instead.
Obviously we can then stop polling after some selected timeout is the
card don't complete it's operations.
"1) not waiting for R1B seems a bit violation with standard spec.
R1B is the same as R1 but with _optional_ busy detection on DAT0. So I
don't think we will be violating the spec by only using CMD13.
Post by Dong Aisheng
Also it increase complexity on handling the R1B of the same command
for two different
cases: using hw timeout or polling status for CMD38.
I have started to craft to together a patch for how __mmc_switch()
would be changed in this respect.

I don't think it will become that complicated, and more importantly -
for the ERASE commands, it will be simplifier since there are not the
special case were CMD13 is _not_ allowed, which is the case for
SWITCH.
Post by Dong Aisheng
2) In current implementation, the data size to erase will not exceed
the max_discard_bytes
which is calculated based on max_discard_to of host.
I suggest we should move away from using max_discard_to to calculate
max_discard. This just don't work smoothly since we can end up in
cases were we get too few blocks or not any at all. Like what happen
for Stephen in the Tegra case.
Post by Dong Aisheng
Then how do we specify max_discard_to if want to use polling? UNIT_MAX?
Will it be too long to affect other activities in the same system?"
I would prefer if we could select a fixed number of max
blocks/eraseblocks, then calculate the timeout based on that.
Post by Dong Aisheng
That means we should erase a proper size of data in case it affects the system
a lot(e.g. two partitions on the same card, discard one partition may
cause the code
on another partition has no chance to run, it may be serious if the
file system is in it.).
So maybe the fixed numer of max blocks/eraseblocks should be just a few ones?
Post by Dong Aisheng
Regards
Dong Aisheng
Post by Ulf Hansson
Would be very interesting to know what option you prefer!?
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Adrian Hunter
2013-12-19 12:28:46 UTC
Permalink
Post by Ulf Hansson
Post by Adrian Hunter
Post by Vladimir Zapolskiy
Post by Adrian Hunter
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host,&cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
But large erases will timeout when they should have been split into smaller
chunks.
A generic solution needs to be able to explain what happens when the host
controller *does* timeout.
Please correct me, but if Data Timeout Error is disabled, then this is not
an issue for most of the host controllers.
That is a very good point. My experience with SDHCI was that masking the
"Data Timeout Error Status Enable" and "Data Timeout Error Signal Enable
" bits did not disable the timeout i.e. the host controller would not
deliver a TC interrupt if the erase exceeded the timeout.
What happens on your board?
I posted a response yesterday for "[PATCH] mmc: core: don't decrement
qty when calculating max_discard", related to this. Please have a
look.
I think the interesting case to consider here is how we can handle
busy detection timeouts that is bigger than what the host hw can
support.
Option 1)
Should we tell the host to disable the timeout in this case? That
potentially means hanging forever - if the card misbehaves. Like
omap_hsmmc does for erase commands. Maybe that is an okay limitation?
sdhci anyway has a 10 second timer to catch unresponsive host controllers.
I recently sent a patch to use the cmd_timeout_ms if it is bigger than 10
seconds.

http://permalink.gmane.org/gmane.linux.kernel.mmc/23557
Post by Ulf Hansson
Option 2)
Use a R1 response instead if R1B to prevent the host from doing busy
detection. Then rely on the CMD13 to poll for completion instead.
Obviously we can then stop polling after some selected timeout is the
card don't complete it's operations.
It would be nice to avoid polling when the timeout can be supported. Also
the polling should be periodic.
Post by Ulf Hansson
Would be very interesting to know what option you prefer!?
At least 1 of the host controllers I have seen does not support disabling
the timeout - so option 1) might not work in all cases. Although it is the
nicer option i.e. replace the hardware timeout with a software timeout.

So I would probably allow both options to co-exist.
Post by Ulf Hansson
Kind regards
Uffe
Ulf Hansson
2013-12-19 13:29:27 UTC
Permalink
Post by Adrian Hunter
Post by Ulf Hansson
Post by Adrian Hunter
Post by Vladimir Zapolskiy
Post by Adrian Hunter
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host,&cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card,
arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
But large erases will timeout when they should have been split into smaller
chunks.
A generic solution needs to be able to explain what happens when the host
controller *does* timeout.
Please correct me, but if Data Timeout Error is disabled, then this is not
an issue for most of the host controllers.
That is a very good point. My experience with SDHCI was that masking the
"Data Timeout Error Status Enable" and "Data Timeout Error Signal Enable
" bits did not disable the timeout i.e. the host controller would not
deliver a TC interrupt if the erase exceeded the timeout.
What happens on your board?
I posted a response yesterday for "[PATCH] mmc: core: don't decrement
qty when calculating max_discard", related to this. Please have a
look.
I think the interesting case to consider here is how we can handle
busy detection timeouts that is bigger than what the host hw can
support.
Option 1)
Should we tell the host to disable the timeout in this case? That
potentially means hanging forever - if the card misbehaves. Like
omap_hsmmc does for erase commands. Maybe that is an okay limitation?
sdhci anyway has a 10 second timer to catch unresponsive host controllers.
I recently sent a patch to use the cmd_timeout_ms if it is bigger than 10
seconds.
http://permalink.gmane.org/gmane.linux.kernel.mmc/23557
I see the reason behind your patch. Somehow, I don't like that host
drivers need to care about such things for specific commands.

The host driver should only tell it's maximum supported busy detection
timeout (max_discard_to) to the core layer, which should be needed
only of it supports MMC_CAP_WAIT_WHILE_BUSY.

Then the core layer should decide what to do depending on current
needed timeout.

BTW, do you know why sdhci haven't enabled MMC_CAP_WAIT_WHILE_BUSY. It
seems like it should be?
Post by Adrian Hunter
Post by Ulf Hansson
Option 2)
Use a R1 response instead if R1B to prevent the host from doing busy
detection. Then rely on the CMD13 to poll for completion instead.
Obviously we can then stop polling after some selected timeout is the
card don't complete it's operations.
It would be nice to avoid polling when the timeout can be supported. Also
the polling should be periodic.
Agree!
Post by Adrian Hunter
Post by Ulf Hansson
Would be very interesting to know what option you prefer!?
At least 1 of the host controllers I have seen does not support disabling
the timeout - so option 1) might not work in all cases. Although it is the
nicer option i.e. replace the hardware timeout with a software timeout.
So I would probably allow both options to co-exist.
Thanks for input Adrian!
Post by Adrian Hunter
Post by Ulf Hansson
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Adrian Hunter
2013-12-19 13:49:46 UTC
Permalink
Post by Ulf Hansson
Post by Adrian Hunter
Post by Ulf Hansson
Post by Adrian Hunter
Post by Vladimir Zapolskiy
Post by Adrian Hunter
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host,&cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card,
unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card,
arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
But large erases will timeout when they should have been split into smaller
chunks.
A generic solution needs to be able to explain what happens when the host
controller *does* timeout.
Please correct me, but if Data Timeout Error is disabled, then this is not
an issue for most of the host controllers.
That is a very good point. My experience with SDHCI was that masking the
"Data Timeout Error Status Enable" and "Data Timeout Error Signal Enable
" bits did not disable the timeout i.e. the host controller would not
deliver a TC interrupt if the erase exceeded the timeout.
What happens on your board?
I posted a response yesterday for "[PATCH] mmc: core: don't decrement
qty when calculating max_discard", related to this. Please have a
look.
I think the interesting case to consider here is how we can handle
busy detection timeouts that is bigger than what the host hw can
support.
Option 1)
Should we tell the host to disable the timeout in this case? That
potentially means hanging forever - if the card misbehaves. Like
omap_hsmmc does for erase commands. Maybe that is an okay limitation?
sdhci anyway has a 10 second timer to catch unresponsive host controllers.
I recently sent a patch to use the cmd_timeout_ms if it is bigger than 10
seconds.
http://permalink.gmane.org/gmane.linux.kernel.mmc/23557
I see the reason behind your patch. Somehow, I don't like that host
drivers need to care about such things for specific commands.
It is not for a specific command - the timer is used for all commands.
Post by Ulf Hansson
The host driver should only tell it's maximum supported busy detection
timeout (max_discard_to) to the core layer, which should be needed
only of it supports MMC_CAP_WAIT_WHILE_BUSY.
Then the core layer should decide what to do depending on current
needed timeout.
BTW, do you know why sdhci haven't enabled MMC_CAP_WAIT_WHILE_BUSY. It
seems like it should be?
Yes it should be. Just an oversight.
Post by Ulf Hansson
Post by Adrian Hunter
Post by Ulf Hansson
Option 2)
Use a R1 response instead if R1B to prevent the host from doing busy
detection. Then rely on the CMD13 to poll for completion instead.
Obviously we can then stop polling after some selected timeout is the
card don't complete it's operations.
It would be nice to avoid polling when the timeout can be supported. Also
the polling should be periodic.
Agree!
Post by Adrian Hunter
Post by Ulf Hansson
Would be very interesting to know what option you prefer!?
At least 1 of the host controllers I have seen does not support disabling
the timeout - so option 1) might not work in all cases. Although it is the
nicer option i.e. replace the hardware timeout with a software timeout.
So I would probably allow both options to co-exist.
Thanks for input Adrian!
Post by Adrian Hunter
Post by Ulf Hansson
Kind regards
Uffe
Stephen Warren
2013-12-19 19:11:28 UTC
Permalink
Post by Adrian Hunter
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host, &cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
But large erases will timeout when they should have been split into smaller
chunks.
A generic solution needs to be able to explain what happens when the host
controller *does* timeout.
I thought the whole point of this discussion was that the timeout in the
first code segment above only applies to command submission, whereas
completion of the erase operation itself was determined by polling using
CMD13, which still uses mmc_erase_timeout(). As such, I don't /think/
that "large erases will timeout" here, unless I'm significantly
misunderstanding something?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Adrian Hunter
2013-12-20 07:17:39 UTC
Permalink
Post by Stephen Warren
Post by Adrian Hunter
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host, &cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
But large erases will timeout when they should have been split into smaller
chunks.
A generic solution needs to be able to explain what happens when the host
controller *does* timeout.
I thought the whole point of this discussion was that the timeout in the
first code segment above only applies to command submission, whereas
completion of the erase operation itself was determined by polling using
CMD13, which still uses mmc_erase_timeout(). As such, I don't /think/
that "large erases will timeout" here, unless I'm significantly
misunderstanding something?
Setting cmd_timeout_ms to zero just makes the sdhci driver choose the
highest timeout.

Currently the sdhci driver does not have a facility to disable the timeout,
and, as I noted in another thread, masking the "Data Timeout Error Status
Enable" and "Data Timeout Error Signal Enable" bits did not work for me.
What Ulf is suggesting is to change the response type from R1B to R1.

But there is also a need to avoid polling when it is not necessary and to
make it periodic.

Ulf said he is working on a patch to doing something similar with mmc_switch.
Dong Aisheng
2013-12-19 09:05:23 UTC
Permalink
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host, &cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
Is the change you mean here only include above 3 lines changes?
If yes, it's strange to me how does this solve your issue?
It actually does not change the max_discard_to/max_discard_bytes.
It still discards only one sector one time if it does as before....

Regards
Dong Aisheng
Stephen Warren
2013-12-19 19:15:43 UTC
Permalink
Post by Dong Aisheng
Post by Stephen Warren
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
Post by Stephen Warren
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 050eb262485c..35c5b5d86c99 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
cmd.opcode = MMC_ERASE;
cmd.arg = arg;
cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
- cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
err = mmc_wait_for_cmd(card->host, &cmd, 0);
if (err) {
pr_err("mmc_erase: erase error %d, status %#x\n",
@@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
if (mmc_host_is_spi(card->host))
goto out;
- timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
+ timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg, qty));
do {
memset(&cmd, 0, sizeof(struct mmc_command));
cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
Is the change you mean here only include above 3 lines changes?
If yes, it's strange to me how does this solve your issue?
It actually does not change the max_discard_to/max_discard_bytes.
It still discards only one sector one time if it does as before....
It's a revert of the patch which introduced max_discard_to, so all that
logic goes away completely, *then* the 3 lines above, which move the
timeout away from command submission (which is what I believe is
implemented by the controller's timeout HW) and to the CMD13 polling
operation (where we can implement whatever timeout we want, in the kernel).
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Dong Aisheng
2013-12-19 08:39:47 UTC
Permalink
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Fixes: e056a1b5b67b "(mmc: queue: let host controllers specify maximum discard timeout")
---
drivers/mmc/core/core.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 57a2b403bf8e..eb952ca634ea 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2150,8 +2150,25 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
if (!qty)
return 0;
- if (qty == 1)
- return 1;
+ /*
+ * Discard operations may not be aligned to an erase block. In an
+ * unaligned case, we need to issue 1 more discard operation to HW
+ * sectors_to_erase / sectors_per_discard_operation
+ *
+ * To account for this in the timeout calculations, we assume we can
+ * actually discard one less erase block than fits into the HW
+ * timeout. This explains the --qty below.
+ *
+ * When only a single discard block operation fits into the timeout,
+ * disallow any discard operations at all. For example, discarding one
+ * sector at a time is so chronically slow as to be useless. However,
+ * make an exception for SD cards without an erase shift, since qty
+ * isn't multiplied up by an erase block size in the code below for
+ * that case.
+ */
+ if (qty == 1 && !(!card->erase_shift && mmc_card_sd(card)))
+ return 0;
How about when qty == 2?
Erase 2 sectors may have no much difference from 1 sector per one time
for a SD card,
it may still chronically slow, i guess.
So i wonder it may not sovle the issues totally.

Regards
Dong Aisheng
Post by Stephen Warren
/* Convert qty to sectors */
if (card->erase_shift)
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephen Warren
2013-12-19 19:08:10 UTC
Permalink
Post by Dong Aisheng
Post by Stephen Warren
In mmc_do_calc_max_discard(), if only a single erase block can be
discarded within the host controller's timeout, don't allow discard
operations at all.
Previously, the code allowed sector-at-a-time discard (rather than
erase-block-at-a-time), which was chronically slow.
Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
in qty == 1, which is immediately returned. This causes discard to
operate a single sector at a time, which is chronically slow. With this
patch in place, discard operates a single erase block at a time, which
is reasonably fast.
Fixes: e056a1b5b67b "(mmc: queue: let host controllers specify maximum discard timeout")
---
drivers/mmc/core/core.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 57a2b403bf8e..eb952ca634ea 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2150,8 +2150,25 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card,
if (!qty)
return 0;
- if (qty == 1)
- return 1;
+ /*
+ * Discard operations may not be aligned to an erase block. In an
+ * unaligned case, we need to issue 1 more discard operation to HW
+ * sectors_to_erase / sectors_per_discard_operation
+ *
+ * To account for this in the timeout calculations, we assume we can
+ * actually discard one less erase block than fits into the HW
+ * timeout. This explains the --qty below.
+ *
+ * When only a single discard block operation fits into the timeout,
+ * disallow any discard operations at all. For example, discarding one
+ * sector at a time is so chronically slow as to be useless. However,
+ * make an exception for SD cards without an erase shift, since qty
+ * isn't multiplied up by an erase block size in the code below for
+ * that case.
+ */
+ if (qty == 1 && !(!card->erase_shift && mmc_card_sd(card)))
+ return 0;
How about when qty == 2?
Erase 2 sectors may have no much difference from 1 sector per one time
for a SD card,
it may still chronically slow, i guess.
So i wonder it may not sovle the issues totally.
When qty==2, the number of sectors gets multiplied by the number of
sectors in an erase block, so it isn't chronically slow. It's only slow
with qty==1 because without this patch, the multiplication gets skipped
and "1" returned rather then "1 << card->erase_shift".
Continue reading on narkive:
Loading...