* [PATCH 0/3] mmc: core: Update timeouts for __mmc_switch()
@ 2020-01-22 14:27 Ulf Hansson
2020-01-22 14:27 ` [PATCH 1/3] mmc: core: Specify timeouts for BKOPS and CACHE_FLUSH for eMMC Ulf Hansson
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Ulf Hansson @ 2020-01-22 14:27 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Adrian Hunter, Chaotian Jing, Shawn Lin, mirq-linux
This small series updates timeouts used when sending CMD6 for eMMC, through the
__mmc_switch() function.
Ulf Hansson (3):
mmc: core: Specify timeouts for BKOPS and CACHE_FLUSH for eMMC
mmc: block: Use generic_cmd6_time when modifying
INAND_CMD38_ARG_EXT_CSD
mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
drivers/mmc/core/block.c | 6 +++---
drivers/mmc/core/mmc_ops.c | 34 +++++++++++++++++-----------------
2 files changed, 20 insertions(+), 20 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] mmc: core: Specify timeouts for BKOPS and CACHE_FLUSH for eMMC
2020-01-22 14:27 [PATCH 0/3] mmc: core: Update timeouts for __mmc_switch() Ulf Hansson
@ 2020-01-22 14:27 ` Ulf Hansson
2020-01-22 14:27 ` [PATCH 2/3] mmc: block: Use generic_cmd6_time when modifying INAND_CMD38_ARG_EXT_CSD Ulf Hansson
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Ulf Hansson @ 2020-01-22 14:27 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Adrian Hunter, Chaotian Jing, Shawn Lin, mirq-linux
The timeout values used while waiting for a CMD6 for BKOPS or a CACHE_FLUSH
to complete, are not defined by the eMMC spec. However, a timeout of 10
minutes as is currently being used, is just silly for both of these cases.
Instead, let's specify more reasonable timeouts, 120s for BKOPS and 30s for
CACHE_FLUSH.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/mmc_ops.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 09113b9ad679..1966abcbc7c0 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -19,7 +19,9 @@
#include "host.h"
#include "mmc_ops.h"
-#define MMC_OPS_TIMEOUT_MS (10 * 60 * 1000) /* 10 minute timeout */
+#define MMC_OPS_TIMEOUT_MS (10 * 60 * 1000) /* 10min*/
+#define MMC_BKOPS_TIMEOUT_MS (120 * 1000) /* 120s */
+#define MMC_CACHE_FLUSH_TIMEOUT_MS (30 * 1000) /* 30s */
static const u8 tuning_blk_pattern_4bit[] = {
0xff, 0x0f, 0xff, 0x00, 0xff, 0xcc, 0xc3, 0xcc,
@@ -941,7 +943,7 @@ void mmc_run_bkops(struct mmc_card *card)
* urgent levels by using an asynchronous background task, when idle.
*/
err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
- EXT_CSD_BKOPS_START, 1, MMC_OPS_TIMEOUT_MS);
+ EXT_CSD_BKOPS_START, 1, MMC_BKOPS_TIMEOUT_MS);
if (err)
pr_warn("%s: Error %d starting bkops\n",
mmc_hostname(card->host), err);
@@ -961,7 +963,8 @@ int mmc_flush_cache(struct mmc_card *card)
(card->ext_csd.cache_size > 0) &&
(card->ext_csd.cache_ctrl & 1)) {
err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
- EXT_CSD_FLUSH_CACHE, 1, 0);
+ EXT_CSD_FLUSH_CACHE, 1,
+ MMC_CACHE_FLUSH_TIMEOUT_MS);
if (err)
pr_err("%s: cache flush error %d\n",
mmc_hostname(card->host), err);
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/3] mmc: block: Use generic_cmd6_time when modifying INAND_CMD38_ARG_EXT_CSD
2020-01-22 14:27 [PATCH 0/3] mmc: core: Update timeouts for __mmc_switch() Ulf Hansson
2020-01-22 14:27 ` [PATCH 1/3] mmc: core: Specify timeouts for BKOPS and CACHE_FLUSH for eMMC Ulf Hansson
@ 2020-01-22 14:27 ` Ulf Hansson
2020-01-22 14:27 ` [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch() Ulf Hansson
2020-01-24 11:25 ` [PATCH 0/3] mmc: core: Update timeouts for __mmc_switch() Ulf Hansson
3 siblings, 0 replies; 17+ messages in thread
From: Ulf Hansson @ 2020-01-22 14:27 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Adrian Hunter, Chaotian Jing, Shawn Lin, mirq-linux
The INAND_CMD38_ARG_EXT_CSD is a vendor specific EXT_CSD register, which is
used to prepare an erase/trim operation. However, it doesn't make sense to
use a timeout of 10 minutes while updating the register, which becomes the
case when the timeout_ms argument for mmc_switch() is set to zero.
Instead, let's use the generic_cmd6_time, as that seems like a reasonable
timeout to use for these cases.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/block.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 95b41c0891d0..663d87924e5e 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1107,7 +1107,7 @@ static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
card->erase_arg == MMC_TRIM_ARG ?
INAND_CMD38_ARG_TRIM :
INAND_CMD38_ARG_ERASE,
- 0);
+ card->ext_csd.generic_cmd6_time);
}
if (!err)
err = mmc_erase(card, from, nr, card->erase_arg);
@@ -1149,7 +1149,7 @@ static void mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
arg == MMC_SECURE_TRIM1_ARG ?
INAND_CMD38_ARG_SECTRIM1 :
INAND_CMD38_ARG_SECERASE,
- 0);
+ card->ext_csd.generic_cmd6_time);
if (err)
goto out_retry;
}
@@ -1167,7 +1167,7 @@ static void mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
INAND_CMD38_ARG_EXT_CSD,
INAND_CMD38_ARG_SECTRIM2,
- 0);
+ card->ext_csd.generic_cmd6_time);
if (err)
goto out_retry;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
2020-01-22 14:27 [PATCH 0/3] mmc: core: Update timeouts for __mmc_switch() Ulf Hansson
2020-01-22 14:27 ` [PATCH 1/3] mmc: core: Specify timeouts for BKOPS and CACHE_FLUSH for eMMC Ulf Hansson
2020-01-22 14:27 ` [PATCH 2/3] mmc: block: Use generic_cmd6_time when modifying INAND_CMD38_ARG_EXT_CSD Ulf Hansson
@ 2020-01-22 14:27 ` Ulf Hansson
2021-02-22 16:24 ` Paul Fertser
2020-01-24 11:25 ` [PATCH 0/3] mmc: core: Update timeouts for __mmc_switch() Ulf Hansson
3 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2020-01-22 14:27 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Adrian Hunter, Chaotian Jing, Shawn Lin, mirq-linux
All callers of __mmc_switch() should now be specifying a valid timeout for
the CMD6 command. However, to sure let's print a warning and default to use
the generic_cmd6_time in case the provided timeout_ms argument is zero.
In this context, let's also simplify some of corresponding code and clarify
some related comments.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/mmc_ops.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 1966abcbc7c0..da425ee2d9bf 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -460,10 +460,6 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
bool expired = false;
bool busy = false;
- /* We have an unspecified cmd timeout, use the fallback value. */
- if (!timeout_ms)
- timeout_ms = MMC_OPS_TIMEOUT_MS;
-
/*
* In cases when not allowed to poll by using CMD13 or because we aren't
* capable of polling by using ->card_busy(), then rely on waiting the
@@ -536,14 +532,19 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
mmc_retune_hold(host);
+ if (!timeout_ms) {
+ pr_warn("%s: unspecified timeout for CMD6 - use generic\n",
+ mmc_hostname(host));
+ timeout_ms = card->ext_csd.generic_cmd6_time;
+ }
+
/*
- * If the cmd timeout and the max_busy_timeout of the host are both
- * specified, let's validate them. A failure means we need to prevent
- * the host from doing hw busy detection, which is done by converting
- * to a R1 response instead of a R1B.
+ * If the max_busy_timeout of the host is specified, make sure it's
+ * enough to fit the used timeout_ms. In case it's not, let's instruct
+ * the host to avoid HW busy detection, by converting to a R1 response
+ * instead of a R1B.
*/
- if (timeout_ms && host->max_busy_timeout &&
- (timeout_ms > host->max_busy_timeout))
+ if (host->max_busy_timeout && (timeout_ms > host->max_busy_timeout))
use_r1b_resp = false;
cmd.opcode = MMC_SWITCH;
@@ -554,10 +555,6 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
cmd.flags = MMC_CMD_AC;
if (use_r1b_resp) {
cmd.flags |= MMC_RSP_SPI_R1B | MMC_RSP_R1B;
- /*
- * A busy_timeout of zero means the host can decide to use
- * whatever value it finds suitable.
- */
cmd.busy_timeout = timeout_ms;
} else {
cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] mmc: core: Update timeouts for __mmc_switch()
2020-01-22 14:27 [PATCH 0/3] mmc: core: Update timeouts for __mmc_switch() Ulf Hansson
` (2 preceding siblings ...)
2020-01-22 14:27 ` [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch() Ulf Hansson
@ 2020-01-24 11:25 ` Ulf Hansson
3 siblings, 0 replies; 17+ messages in thread
From: Ulf Hansson @ 2020-01-24 11:25 UTC (permalink / raw)
To: linux-mmc
Cc: Adrian Hunter, Chaotian Jing, Shawn Lin,
Michał Mirosław, Ulf Hansson
On Wed, 22 Jan 2020 at 15:27, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> This small series updates timeouts used when sending CMD6 for eMMC, through the
> __mmc_switch() function.
>
> Ulf Hansson (3):
> mmc: core: Specify timeouts for BKOPS and CACHE_FLUSH for eMMC
> mmc: block: Use generic_cmd6_time when modifying
> INAND_CMD38_ARG_EXT_CSD
> mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
>
> drivers/mmc/core/block.c | 6 +++---
> drivers/mmc/core/mmc_ops.c | 34 +++++++++++++++++-----------------
> 2 files changed, 20 insertions(+), 20 deletions(-)
>
> --
> 2.17.1
>
To widen the test cases, I have queued up this for next, also by
amending patch3 for fixing some spelling mistakes in the changelog.
Please tell if you see any problems with this and feel free to provide
tested by tags.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
2020-01-22 14:27 ` [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch() Ulf Hansson
@ 2021-02-22 16:24 ` Paul Fertser
2021-02-22 20:12 ` Paul Fertser
0 siblings, 1 reply; 17+ messages in thread
From: Paul Fertser @ 2021-02-22 16:24 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Adrian Hunter, Chaotian Jing, Shawn Lin, mirq-linux
Hello,
On Wed, Jan 22, 2020 at 03:27:47PM +0100, Ulf Hansson wrote:
> All callers of __mmc_switch() should now be specifying a valid timeout for
> the CMD6 command.
I'm running a kernel based on linux-next on a Tegra2 system (Toshiba
ac100 aka paz00, on-board eMMC) and seeing plenty of these warnings on
boot. I added WARN_ON_ONCE to see the backtrace and here's what I get:
[ 1.931816] mmc1: power class selection to bus width 8 ddr 0 failed
[ 1.931867] mmc1: new high speed MMC card at address 0001
[ 1.937795] mmcblk1: mmc1:0001 MMC32G 29.8 GiB
[ 1.942372] mmcblk1boot0: mmc1:0001 MMC32G partition 1 2.00 MiB
[ 1.947318] mmcblk1boot1: mmc1:0001 MMC32G partition 2 2.00 MiB
[ 1.948004] mmcblk1rpmb: mmc1:0001 MMC32G partition 3 256 KiB, chardev (249:0)
[ 1.959161] mmcblk1: p1 p2
...
[ 3.209874] mmc1: unspecified timeout for CMD6 - use generic
[ 3.222780] ------------[ cut here ]------------
[ 3.233363] WARNING: CPU: 1 PID: 111 at drivers/mmc/core/mmc_ops.c:575 __mmc_switch+0x200/0x204
[ 3.251583] Modules linked in: evdev nvec(C)
[ 3.261750] CPU: 1 PID: 111 Comm: systemd-udevd Tainted: G C 5.11.0-next-20210222+ #34
[ 3.282397] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[ 3.292242] [<c010ebcc>] (unwind_backtrace) from [<c010a4bc>] (show_stack+0x10/0x14)
[ 3.316951] [<c010a4bc>] (show_stack) from [<c07e0308>] (dump_stack+0xc8/0xdc)
[ 3.316976] [<c07e0308>] (dump_stack) from [<c07ddc84>] (__warn+0xc0/0xd8)
[ 3.316990] [<c07ddc84>] (__warn) from [<c07ddcfc>] (warn_slowpath_fmt+0x60/0xbc)
[ 3.338419] [<c07ddcfc>] (warn_slowpath_fmt) from [<c063d878>] (__mmc_switch+0x200/0x204)
[ 3.338441] [<c063d878>] (__mmc_switch) from [<c063d8a4>] (mmc_switch+0x28/0x30)
[ 3.338454] [<c063d8a4>] (mmc_switch) from [<c0648f48>] (mmc_blk_mq_issue_rq+0x22c/0x900)
[ 3.396728] [<c0648f48>] (mmc_blk_mq_issue_rq) from [<c064998c>] (mmc_mq_queue_rq+0x124/0x258)
[ 3.409215] [<c064998c>] (mmc_mq_queue_rq) from [<c039a9e8>] (__blk_mq_try_issue_directly+0x140/0x1cc)
[ 3.422431] [<c039a9e8>] (__blk_mq_try_issue_directly) from [<c039bfbc>] (blk_mq_request_issue_directly+0x48/0x78)
[ 3.436719] [<c039bfbc>] (blk_mq_request_issue_directly) from [<c039c040>] (blk_mq_try_issue_list_directly+0x54/0xd8)
[ 3.451310] [<c039c040>] (blk_mq_try_issue_list_directly) from [<c03a0a90>] (blk_mq_sched_insert_requests+0xd8/0x158)
[ 3.465949] [<c03a0a90>] (blk_mq_sched_insert_requests) from [<c039bf28>] (blk_mq_flush_plug_list+0x12c/0x178)
[ 3.480021] [<c039bf28>] (blk_mq_flush_plug_list) from [<c0390904>] (blk_flush_plug_list+0xc8/0xe4)
[ 3.493173] [<c0390904>] (blk_flush_plug_list) from [<c039094c>] (blk_finish_plug+0x2c/0x48)
[ 3.505748] [<c039094c>] (blk_finish_plug) from [<c01f00a4>] (read_pages+0x15c/0x2bc)
[ 3.517783] [<c01f00a4>] (read_pages) from [<c01f0548>] (page_cache_ra_unbounded+0x120/0x208)
[ 3.530544] [<c01f0548>] (page_cache_ra_unbounded) from [<c01e84dc>] (filemap_read+0x1e4/0x9c0)
[ 3.543498] [<c01e84dc>] (filemap_read) from [<c02476e0>] (vfs_read+0x204/0x330)
[ 3.555208] [<c02476e0>] (vfs_read) from [<c0247cf0>] (ksys_read+0x58/0xd0)
[ 3.566506] [<c0247cf0>] (ksys_read) from [<c01000c0>] (ret_fast_syscall+0x0/0x58)
[ 3.578425] Exception stack(0xc357dfa8 to 0xc357dff0)
[ 3.587793] dfa0: 00000074 00000000 0000000c 004cb990 00000040 00000000
[ 3.600416] dfc0: 00000074 00000000 004d2f68 00000003 004cb970 004cb988 b6de11e0 00000000
[ 3.613026] dfe0: 00000003 bed66c30 b6ea652f b6e2f746
[ 3.623960] ---[ end trace 74a276127e5a089a ]---
/sys/kernel/debug/mmc1/mmc1:0001/ext_csd:00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000e80e000300000000000000020000000000000100000000000100000001000000000000050002000700000077773333001e003c003c00000000ba030011000709011002080710000742101504001e00000077330064000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
/sys/devices/soc0/c8000600.mmc/mmc_host/mmc1/mmc1:0001/csd:900e00320f5903ffffffffe796400000
Do I need to provide any additional information for the bug to be
pin-pointed or is this enough?
--
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
2021-02-22 16:24 ` Paul Fertser
@ 2021-02-22 20:12 ` Paul Fertser
2021-02-23 9:23 ` Ulf Hansson
0 siblings, 1 reply; 17+ messages in thread
From: Paul Fertser @ 2021-02-22 20:12 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Adrian Hunter, Chaotian Jing, Shawn Lin, mirq-linux
On Mon, Feb 22, 2021 at 07:24:06PM +0300, Paul Fertser wrote:
> On Wed, Jan 22, 2020 at 03:27:47PM +0100, Ulf Hansson wrote:
> > All callers of __mmc_switch() should now be specifying a valid timeout for
> > the CMD6 command.
>
> I'm running a kernel based on linux-next on a Tegra2 system (Toshiba
> ac100 aka paz00, on-board eMMC) and seeing plenty of these warnings on
> boot. I added WARN_ON_ONCE to see the backtrace and here's what I get:
...
> [ 3.338454] [<c063d8a4>] (mmc_switch) from [<c0648f48>] (mmc_blk_mq_issue_rq+0x22c/0x900)
> [ 3.396728] [<c0648f48>] (mmc_blk_mq_issue_rq) from [<c064998c>] (mmc_mq_queue_rq+0x124/0x258)
> [ 3.409215] [<c064998c>] (mmc_mq_queue_rq) from [<c039a9e8>] (__blk_mq_try_issue_directly+0x140/0x1cc)
FWIW, with
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index f5dedb7f9b27..9adf735391fa 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -426,8 +426,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
/* EXT_CSD value is in units of 10ms, but we store in ms */
card->ext_csd.part_time = 10 * ext_csd[EXT_CSD_PART_SWITCH_TIME];
/* Some eMMC set the value too low so set a minimum */
- if (card->ext_csd.part_time &&
- card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
+ if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
/* Sleep / awake timeout in 100ns units */
I do not see any more warnings on my system.
--
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
2021-02-22 20:12 ` Paul Fertser
@ 2021-02-23 9:23 ` Ulf Hansson
2021-02-23 9:32 ` Paul Fertser
0 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2021-02-23 9:23 UTC (permalink / raw)
To: Paul Fertser
Cc: linux-mmc, Adrian Hunter, Chaotian Jing, Shawn Lin,
Michał Mirosław
On Mon, 22 Feb 2021 at 21:12, Paul Fertser <fercerpav@gmail.com> wrote:
>
> On Mon, Feb 22, 2021 at 07:24:06PM +0300, Paul Fertser wrote:
> > On Wed, Jan 22, 2020 at 03:27:47PM +0100, Ulf Hansson wrote:
> > > All callers of __mmc_switch() should now be specifying a valid timeout for
> > > the CMD6 command.
> >
> > I'm running a kernel based on linux-next on a Tegra2 system (Toshiba
> > ac100 aka paz00, on-board eMMC) and seeing plenty of these warnings on
> > boot. I added WARN_ON_ONCE to see the backtrace and here's what I get:
> ...
> > [ 3.338454] [<c063d8a4>] (mmc_switch) from [<c0648f48>] (mmc_blk_mq_issue_rq+0x22c/0x900)
> > [ 3.396728] [<c0648f48>] (mmc_blk_mq_issue_rq) from [<c064998c>] (mmc_mq_queue_rq+0x124/0x258)
> > [ 3.409215] [<c064998c>] (mmc_mq_queue_rq) from [<c039a9e8>] (__blk_mq_try_issue_directly+0x140/0x1cc)
>
> FWIW, with
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index f5dedb7f9b27..9adf735391fa 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -426,8 +426,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
> /* EXT_CSD value is in units of 10ms, but we store in ms */
> card->ext_csd.part_time = 10 * ext_csd[EXT_CSD_PART_SWITCH_TIME];
> /* Some eMMC set the value too low so set a minimum */
> - if (card->ext_csd.part_time &&
> - card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> + if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
>
> /* Sleep / awake timeout in 100ns units */
>
> I do not see any more warnings on my system.
That looks like the correct fix to the problem. Do you want to send a
proper patch that I can pick up or do you prefer if help to do it?
Seems like we should add the following fixes tag as well.
Fixes: 1c447116d017 ("mmc: mmc: Fix partition switch timeout for some eMMCs")
Kind regards
Uffe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
2021-02-23 9:23 ` Ulf Hansson
@ 2021-02-23 9:32 ` Paul Fertser
2021-02-23 10:44 ` Ulf Hansson
2021-02-23 11:01 ` Adrian Hunter
0 siblings, 2 replies; 17+ messages in thread
From: Paul Fertser @ 2021-02-23 9:32 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-mmc, Adrian Hunter, Chaotian Jing, Shawn Lin,
Michał Mirosław
Hello Ulf,
On Tue, Feb 23, 2021 at 10:23:28AM +0100, Ulf Hansson wrote:
> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > index f5dedb7f9b27..9adf735391fa 100644
> > --- a/drivers/mmc/core/mmc.c
> > +++ b/drivers/mmc/core/mmc.c
> > @@ -426,8 +426,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
> > /* EXT_CSD value is in units of 10ms, but we store in ms */
> > card->ext_csd.part_time = 10 * ext_csd[EXT_CSD_PART_SWITCH_TIME];
> > /* Some eMMC set the value too low so set a minimum */
> > - if (card->ext_csd.part_time &&
> > - card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> > + if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> > card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
> >
> > /* Sleep / awake timeout in 100ns units */
> >
> > I do not see any more warnings on my system.
>
> That looks like the correct fix to the problem. Do you want to send a
> proper patch that I can pick up or do you prefer if help to do it?
I've sent this as a diff precisely because 1c447116d017 was so
explicit about special-casing zero ext_csd timeout value, so I thought
probably Adrian can provide the rationale for that. I'd prefer to wait
for his feedback before sending a formal patch. Does this make sense?
--
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
2021-02-23 9:32 ` Paul Fertser
@ 2021-02-23 10:44 ` Ulf Hansson
2021-02-23 11:01 ` Adrian Hunter
1 sibling, 0 replies; 17+ messages in thread
From: Ulf Hansson @ 2021-02-23 10:44 UTC (permalink / raw)
To: Paul Fertser
Cc: linux-mmc, Adrian Hunter, Chaotian Jing, Shawn Lin,
Michał Mirosław
On Tue, 23 Feb 2021 at 10:32, Paul Fertser <fercerpav@gmail.com> wrote:
>
> Hello Ulf,
>
> On Tue, Feb 23, 2021 at 10:23:28AM +0100, Ulf Hansson wrote:
> > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > > index f5dedb7f9b27..9adf735391fa 100644
> > > --- a/drivers/mmc/core/mmc.c
> > > +++ b/drivers/mmc/core/mmc.c
> > > @@ -426,8 +426,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
> > > /* EXT_CSD value is in units of 10ms, but we store in ms */
> > > card->ext_csd.part_time = 10 * ext_csd[EXT_CSD_PART_SWITCH_TIME];
> > > /* Some eMMC set the value too low so set a minimum */
> > > - if (card->ext_csd.part_time &&
> > > - card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> > > + if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> > > card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
> > >
> > > /* Sleep / awake timeout in 100ns units */
> > >
> > > I do not see any more warnings on my system.
> >
> > That looks like the correct fix to the problem. Do you want to send a
> > proper patch that I can pick up or do you prefer if help to do it?
>
> I've sent this as a diff precisely because 1c447116d017 was so
> explicit about special-casing zero ext_csd timeout value, so I thought
> probably Adrian can provide the rationale for that. I'd prefer to wait
> for his feedback before sending a formal patch. Does this make sense?
I think the rationale was not to set a default timeout if the value
from the register is zero (because there is a fallback in
__mmc_switch() for this case). The problem with the fallback is that
it's one timeout value for all types of commands. It's better to
specify a default value, based upon what command it's for - along the
line of what your diff suggests.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
2021-02-23 9:32 ` Paul Fertser
2021-02-23 10:44 ` Ulf Hansson
@ 2021-02-23 11:01 ` Adrian Hunter
2021-02-23 11:19 ` Paul Fertser
1 sibling, 1 reply; 17+ messages in thread
From: Adrian Hunter @ 2021-02-23 11:01 UTC (permalink / raw)
To: Paul Fertser, Ulf Hansson
Cc: linux-mmc, Chaotian Jing, Shawn Lin, Michał Mirosław
On 23/02/21 11:32 am, Paul Fertser wrote:
> Hello Ulf,
>
> On Tue, Feb 23, 2021 at 10:23:28AM +0100, Ulf Hansson wrote:
>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>> index f5dedb7f9b27..9adf735391fa 100644
>>> --- a/drivers/mmc/core/mmc.c
>>> +++ b/drivers/mmc/core/mmc.c
>>> @@ -426,8 +426,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>>> /* EXT_CSD value is in units of 10ms, but we store in ms */
>>> card->ext_csd.part_time = 10 * ext_csd[EXT_CSD_PART_SWITCH_TIME];
>>> /* Some eMMC set the value too low so set a minimum */
>>> - if (card->ext_csd.part_time &&
>>> - card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
>>> + if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
>>> card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
>>>
>>> /* Sleep / awake timeout in 100ns units */
>>>
>>> I do not see any more warnings on my system.
>>
>> That looks like the correct fix to the problem. Do you want to send a
>> proper patch that I can pick up or do you prefer if help to do it?
>
> I've sent this as a diff precisely because 1c447116d017 was so
> explicit about special-casing zero ext_csd timeout value, so I thought
> probably Adrian can provide the rationale for that. I'd prefer to wait
> for his feedback before sending a formal patch. Does this make sense?
Zero means indefinite. Might be safer to use a higher value than
MMC_MIN_PART_SWITCH_TIME for that case. The maximum GENERIC_CMD6_TIME is
2550 ms.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
2021-02-23 11:01 ` Adrian Hunter
@ 2021-02-23 11:19 ` Paul Fertser
2021-02-23 11:54 ` Ulf Hansson
0 siblings, 1 reply; 17+ messages in thread
From: Paul Fertser @ 2021-02-23 11:19 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, linux-mmc, Chaotian Jing, Shawn Lin,
Michał Mirosław
Hello Adrian,
On Tue, Feb 23, 2021 at 01:01:09PM +0200, Adrian Hunter wrote:
> On 23/02/21 11:32 am, Paul Fertser wrote:
> > Hello Ulf,
> >
> > On Tue, Feb 23, 2021 at 10:23:28AM +0100, Ulf Hansson wrote:
> >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> >>> index f5dedb7f9b27..9adf735391fa 100644
> >>> --- a/drivers/mmc/core/mmc.c
> >>> +++ b/drivers/mmc/core/mmc.c
> >>> @@ -426,8 +426,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
> >>> /* EXT_CSD value is in units of 10ms, but we store in ms */
> >>> card->ext_csd.part_time = 10 * ext_csd[EXT_CSD_PART_SWITCH_TIME];
> >>> /* Some eMMC set the value too low so set a minimum */
> >>> - if (card->ext_csd.part_time &&
> >>> - card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> >>> + if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> >>> card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
> >>>
> >>> /* Sleep / awake timeout in 100ns units */
> >>>
> >>> I do not see any more warnings on my system.
> >>
> >> That looks like the correct fix to the problem. Do you want to send a
> >> proper patch that I can pick up or do you prefer if help to do it?
> >
> > I've sent this as a diff precisely because 1c447116d017 was so
> > explicit about special-casing zero ext_csd timeout value, so I thought
> > probably Adrian can provide the rationale for that. I'd prefer to wait
> > for his feedback before sending a formal patch. Does this make sense?
>
> Zero means indefinite. Might be safer to use a higher value than
> MMC_MIN_PART_SWITCH_TIME for that case. The maximum GENERIC_CMD6_TIME is
> 2550 ms.
Thanks for the clarification! I would guess that most likely than not
when whoever defines that value to be zero it means "I do not
care/know" rather than "the timeout must be set to more than 2550 ms,
too bad 8 bits are not enough to represent that". I'd say setting it
to DEFAULT_CMD6_TIMEOUT_MS should be safe enough since it worked
before.
--
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
2021-02-23 11:19 ` Paul Fertser
@ 2021-02-23 11:54 ` Ulf Hansson
2021-02-23 13:42 ` Adrian Hunter
0 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2021-02-23 11:54 UTC (permalink / raw)
To: Paul Fertser
Cc: Adrian Hunter, linux-mmc, Chaotian Jing, Shawn Lin,
Michał Mirosław
On Tue, 23 Feb 2021 at 12:19, Paul Fertser <fercerpav@gmail.com> wrote:
>
> Hello Adrian,
>
> On Tue, Feb 23, 2021 at 01:01:09PM +0200, Adrian Hunter wrote:
> > On 23/02/21 11:32 am, Paul Fertser wrote:
> > > Hello Ulf,
> > >
> > > On Tue, Feb 23, 2021 at 10:23:28AM +0100, Ulf Hansson wrote:
> > >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > >>> index f5dedb7f9b27..9adf735391fa 100644
> > >>> --- a/drivers/mmc/core/mmc.c
> > >>> +++ b/drivers/mmc/core/mmc.c
> > >>> @@ -426,8 +426,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
> > >>> /* EXT_CSD value is in units of 10ms, but we store in ms */
> > >>> card->ext_csd.part_time = 10 * ext_csd[EXT_CSD_PART_SWITCH_TIME];
> > >>> /* Some eMMC set the value too low so set a minimum */
> > >>> - if (card->ext_csd.part_time &&
> > >>> - card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> > >>> + if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> > >>> card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
> > >>>
> > >>> /* Sleep / awake timeout in 100ns units */
> > >>>
> > >>> I do not see any more warnings on my system.
> > >>
> > >> That looks like the correct fix to the problem. Do you want to send a
> > >> proper patch that I can pick up or do you prefer if help to do it?
> > >
> > > I've sent this as a diff precisely because 1c447116d017 was so
> > > explicit about special-casing zero ext_csd timeout value, so I thought
> > > probably Adrian can provide the rationale for that. I'd prefer to wait
> > > for his feedback before sending a formal patch. Does this make sense?
> >
> > Zero means indefinite. Might be safer to use a higher value than
> > MMC_MIN_PART_SWITCH_TIME for that case. The maximum GENERIC_CMD6_TIME is
> > 2550 ms.
>
> Thanks for the clarification! I would guess that most likely than not
> when whoever defines that value to be zero it means "I do not
> care/know" rather than "the timeout must be set to more than 2550 ms,
> too bad 8 bits are not enough to represent that". I'd say setting it
> to DEFAULT_CMD6_TIMEOUT_MS should be safe enough since it worked
> before.
Hmm.
The DEFAULT_CMD6_TIMEOUT_MS is intended to override the
ext_csd->generic_cmd6_time, in case it's not defined in the register.
Perhaps it's reasonable to think that eMMC vendors specify the
GENERIC_CMD6_TIME, but may skip to specify other timeouts, like the
PARTITION_SWITCH_TIME. In that case, should we use the specified
GENERIC_CMD6_TIME, rather than always default to
DEFAULT_CMD6_TIMEOUT_MS?
Kind regards
Uffe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
2021-02-23 11:54 ` Ulf Hansson
@ 2021-02-23 13:42 ` Adrian Hunter
2021-02-24 11:09 ` Ulf Hansson
0 siblings, 1 reply; 17+ messages in thread
From: Adrian Hunter @ 2021-02-23 13:42 UTC (permalink / raw)
To: Ulf Hansson, Paul Fertser
Cc: linux-mmc, Chaotian Jing, Shawn Lin, Michał Mirosław
On 23/02/21 1:54 pm, Ulf Hansson wrote:
> On Tue, 23 Feb 2021 at 12:19, Paul Fertser <fercerpav@gmail.com> wrote:
>>
>> Hello Adrian,
>>
>> On Tue, Feb 23, 2021 at 01:01:09PM +0200, Adrian Hunter wrote:
>>> On 23/02/21 11:32 am, Paul Fertser wrote:
>>>> Hello Ulf,
>>>>
>>>> On Tue, Feb 23, 2021 at 10:23:28AM +0100, Ulf Hansson wrote:
>>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>>>>>> index f5dedb7f9b27..9adf735391fa 100644
>>>>>> --- a/drivers/mmc/core/mmc.c
>>>>>> +++ b/drivers/mmc/core/mmc.c
>>>>>> @@ -426,8 +426,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>>>>>> /* EXT_CSD value is in units of 10ms, but we store in ms */
>>>>>> card->ext_csd.part_time = 10 * ext_csd[EXT_CSD_PART_SWITCH_TIME];
>>>>>> /* Some eMMC set the value too low so set a minimum */
>>>>>> - if (card->ext_csd.part_time &&
>>>>>> - card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
>>>>>> + if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
>>>>>> card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
>>>>>>
>>>>>> /* Sleep / awake timeout in 100ns units */
>>>>>>
>>>>>> I do not see any more warnings on my system.
>>>>>
>>>>> That looks like the correct fix to the problem. Do you want to send a
>>>>> proper patch that I can pick up or do you prefer if help to do it?
>>>>
>>>> I've sent this as a diff precisely because 1c447116d017 was so
>>>> explicit about special-casing zero ext_csd timeout value, so I thought
>>>> probably Adrian can provide the rationale for that. I'd prefer to wait
>>>> for his feedback before sending a formal patch. Does this make sense?
>>>
>>> Zero means indefinite. Might be safer to use a higher value than
>>> MMC_MIN_PART_SWITCH_TIME for that case. The maximum GENERIC_CMD6_TIME is
>>> 2550 ms.
>>
>> Thanks for the clarification! I would guess that most likely than not
>> when whoever defines that value to be zero it means "I do not
>> care/know" rather than "the timeout must be set to more than 2550 ms,
>> too bad 8 bits are not enough to represent that". I'd say setting it
>> to DEFAULT_CMD6_TIMEOUT_MS should be safe enough since it worked
>> before.
>
> Hmm.
>
> The DEFAULT_CMD6_TIMEOUT_MS is intended to override the
> ext_csd->generic_cmd6_time, in case it's not defined in the register.
>
> Perhaps it's reasonable to think that eMMC vendors specify the
> GENERIC_CMD6_TIME, but may skip to specify other timeouts, like the
> PARTITION_SWITCH_TIME. In that case, should we use the specified
> GENERIC_CMD6_TIME, rather than always default to
> DEFAULT_CMD6_TIMEOUT_MS?
Sounds reasonable, but perhaps still enforce a minimum, for some of the same
reasons as commit 1c447116d017 ?
e.g.
if (!card->ext_csd.part_time)
card->ext_csd.part_time = card->ext_csd.generic_cmd6_time;
if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()
2021-02-23 13:42 ` Adrian Hunter
@ 2021-02-24 11:09 ` Ulf Hansson
2021-03-03 9:26 ` [PATCH] mmc: mmc: Fix partition switch time Adrian Hunter
0 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2021-02-24 11:09 UTC (permalink / raw)
To: Adrian Hunter
Cc: Paul Fertser, linux-mmc, Chaotian Jing, Shawn Lin,
Michał Mirosław
On Tue, 23 Feb 2021 at 14:42, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 23/02/21 1:54 pm, Ulf Hansson wrote:
> > On Tue, 23 Feb 2021 at 12:19, Paul Fertser <fercerpav@gmail.com> wrote:
> >>
> >> Hello Adrian,
> >>
> >> On Tue, Feb 23, 2021 at 01:01:09PM +0200, Adrian Hunter wrote:
> >>> On 23/02/21 11:32 am, Paul Fertser wrote:
> >>>> Hello Ulf,
> >>>>
> >>>> On Tue, Feb 23, 2021 at 10:23:28AM +0100, Ulf Hansson wrote:
> >>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> >>>>>> index f5dedb7f9b27..9adf735391fa 100644
> >>>>>> --- a/drivers/mmc/core/mmc.c
> >>>>>> +++ b/drivers/mmc/core/mmc.c
> >>>>>> @@ -426,8 +426,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
> >>>>>> /* EXT_CSD value is in units of 10ms, but we store in ms */
> >>>>>> card->ext_csd.part_time = 10 * ext_csd[EXT_CSD_PART_SWITCH_TIME];
> >>>>>> /* Some eMMC set the value too low so set a minimum */
> >>>>>> - if (card->ext_csd.part_time &&
> >>>>>> - card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> >>>>>> + if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> >>>>>> card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
> >>>>>>
> >>>>>> /* Sleep / awake timeout in 100ns units */
> >>>>>>
> >>>>>> I do not see any more warnings on my system.
> >>>>>
> >>>>> That looks like the correct fix to the problem. Do you want to send a
> >>>>> proper patch that I can pick up or do you prefer if help to do it?
> >>>>
> >>>> I've sent this as a diff precisely because 1c447116d017 was so
> >>>> explicit about special-casing zero ext_csd timeout value, so I thought
> >>>> probably Adrian can provide the rationale for that. I'd prefer to wait
> >>>> for his feedback before sending a formal patch. Does this make sense?
> >>>
> >>> Zero means indefinite. Might be safer to use a higher value than
> >>> MMC_MIN_PART_SWITCH_TIME for that case. The maximum GENERIC_CMD6_TIME is
> >>> 2550 ms.
> >>
> >> Thanks for the clarification! I would guess that most likely than not
> >> when whoever defines that value to be zero it means "I do not
> >> care/know" rather than "the timeout must be set to more than 2550 ms,
> >> too bad 8 bits are not enough to represent that". I'd say setting it
> >> to DEFAULT_CMD6_TIMEOUT_MS should be safe enough since it worked
> >> before.
> >
> > Hmm.
> >
> > The DEFAULT_CMD6_TIMEOUT_MS is intended to override the
> > ext_csd->generic_cmd6_time, in case it's not defined in the register.
> >
> > Perhaps it's reasonable to think that eMMC vendors specify the
> > GENERIC_CMD6_TIME, but may skip to specify other timeouts, like the
> > PARTITION_SWITCH_TIME. In that case, should we use the specified
> > GENERIC_CMD6_TIME, rather than always default to
> > DEFAULT_CMD6_TIMEOUT_MS?
>
> Sounds reasonable, but perhaps still enforce a minimum, for some of the same
> reasons as commit 1c447116d017 ?
> e.g.
>
> if (!card->ext_csd.part_time)
> card->ext_csd.part_time = card->ext_csd.generic_cmd6_time;
> if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
>
Makes perfect sense to me!
Kind regards
Uffe
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] mmc: mmc: Fix partition switch time
2021-02-24 11:09 ` Ulf Hansson
@ 2021-03-03 9:26 ` Adrian Hunter
2021-03-04 13:50 ` Ulf Hansson
0 siblings, 1 reply; 17+ messages in thread
From: Adrian Hunter @ 2021-03-03 9:26 UTC (permalink / raw)
To: Ulf Hansson
Cc: Paul Fertser, linux-mmc, Chaotian Jing, Shawn Lin,
Michał Mirosław
Avoid the following warning by always defining partition switch time:
[ 3.209874] mmc1: unspecified timeout for CMD6 - use generic
[ 3.222780] ------------[ cut here ]------------
[ 3.233363] WARNING: CPU: 1 PID: 111 at drivers/mmc/core/mmc_ops.c:575 __mmc_switch+0x200/0x204
Reported-by: Paul Fertser <fercerpav@gmail.com>
Fixes: 1c447116d017 ("mmc: mmc: Fix partition switch timeout for some eMMCs")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/core/mmc.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 0d80b72ddde8..8741271d3971 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -423,10 +423,6 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
/* EXT_CSD value is in units of 10ms, but we store in ms */
card->ext_csd.part_time = 10 * ext_csd[EXT_CSD_PART_SWITCH_TIME];
- /* Some eMMC set the value too low so set a minimum */
- if (card->ext_csd.part_time &&
- card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
- card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
/* Sleep / awake timeout in 100ns units */
if (sa_shift > 0 && sa_shift <= 0x17)
@@ -616,6 +612,17 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
card->ext_csd.data_sector_size = 512;
}
+ /*
+ * GENERIC_CMD6_TIME is to be used "unless a specific timeout is defined
+ * when accessing a specific field", so use it here if there is no
+ * PARTITION_SWITCH_TIME.
+ */
+ if (!card->ext_csd.part_time)
+ card->ext_csd.part_time = card->ext_csd.generic_cmd6_time;
+ /* Some eMMC set the value too low so set a minimum */
+ if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
+ card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
+
/* eMMC v5 or later */
if (card->ext_csd.rev >= 7) {
memcpy(card->ext_csd.fwrev, &ext_csd[EXT_CSD_FIRMWARE_VERSION],
--
2.17.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] mmc: mmc: Fix partition switch time
2021-03-03 9:26 ` [PATCH] mmc: mmc: Fix partition switch time Adrian Hunter
@ 2021-03-04 13:50 ` Ulf Hansson
0 siblings, 0 replies; 17+ messages in thread
From: Ulf Hansson @ 2021-03-04 13:50 UTC (permalink / raw)
To: Adrian Hunter
Cc: Paul Fertser, linux-mmc, Chaotian Jing, Shawn Lin,
Michał Mirosław
On Wed, 3 Mar 2021 at 10:26, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> Avoid the following warning by always defining partition switch time:
>
> [ 3.209874] mmc1: unspecified timeout for CMD6 - use generic
> [ 3.222780] ------------[ cut here ]------------
> [ 3.233363] WARNING: CPU: 1 PID: 111 at drivers/mmc/core/mmc_ops.c:575 __mmc_switch+0x200/0x204
>
> Reported-by: Paul Fertser <fercerpav@gmail.com>
> Fixes: 1c447116d017 ("mmc: mmc: Fix partition switch timeout for some eMMCs")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Applied for next and by adding a stable tag, thanks!
Kind regards
Uffe
> ---
> drivers/mmc/core/mmc.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 0d80b72ddde8..8741271d3971 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -423,10 +423,6 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>
> /* EXT_CSD value is in units of 10ms, but we store in ms */
> card->ext_csd.part_time = 10 * ext_csd[EXT_CSD_PART_SWITCH_TIME];
> - /* Some eMMC set the value too low so set a minimum */
> - if (card->ext_csd.part_time &&
> - card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> - card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
>
> /* Sleep / awake timeout in 100ns units */
> if (sa_shift > 0 && sa_shift <= 0x17)
> @@ -616,6 +612,17 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
> card->ext_csd.data_sector_size = 512;
> }
>
> + /*
> + * GENERIC_CMD6_TIME is to be used "unless a specific timeout is defined
> + * when accessing a specific field", so use it here if there is no
> + * PARTITION_SWITCH_TIME.
> + */
> + if (!card->ext_csd.part_time)
> + card->ext_csd.part_time = card->ext_csd.generic_cmd6_time;
> + /* Some eMMC set the value too low so set a minimum */
> + if (card->ext_csd.part_time < MMC_MIN_PART_SWITCH_TIME)
> + card->ext_csd.part_time = MMC_MIN_PART_SWITCH_TIME;
> +
> /* eMMC v5 or later */
> if (card->ext_csd.rev >= 7) {
> memcpy(card->ext_csd.fwrev, &ext_csd[EXT_CSD_FIRMWARE_VERSION],
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-03-04 13:53 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-22 14:27 [PATCH 0/3] mmc: core: Update timeouts for __mmc_switch() Ulf Hansson
2020-01-22 14:27 ` [PATCH 1/3] mmc: core: Specify timeouts for BKOPS and CACHE_FLUSH for eMMC Ulf Hansson
2020-01-22 14:27 ` [PATCH 2/3] mmc: block: Use generic_cmd6_time when modifying INAND_CMD38_ARG_EXT_CSD Ulf Hansson
2020-01-22 14:27 ` [PATCH 3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch() Ulf Hansson
2021-02-22 16:24 ` Paul Fertser
2021-02-22 20:12 ` Paul Fertser
2021-02-23 9:23 ` Ulf Hansson
2021-02-23 9:32 ` Paul Fertser
2021-02-23 10:44 ` Ulf Hansson
2021-02-23 11:01 ` Adrian Hunter
2021-02-23 11:19 ` Paul Fertser
2021-02-23 11:54 ` Ulf Hansson
2021-02-23 13:42 ` Adrian Hunter
2021-02-24 11:09 ` Ulf Hansson
2021-03-03 9:26 ` [PATCH] mmc: mmc: Fix partition switch time Adrian Hunter
2021-03-04 13:50 ` Ulf Hansson
2020-01-24 11:25 ` [PATCH 0/3] mmc: core: Update timeouts for __mmc_switch() Ulf Hansson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).