| Message ID | 20200122142747.5690-4-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: core: Update timeouts for __mmc_switch() | expand |
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
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
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
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
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
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.
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
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
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;
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
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 > >
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;
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(-) -- 2.17.1