[3/3] mmc: core: Default to generic_cmd6_time as timeout in __mmc_switch()

Message ID 20200122142747.5690-4-ulf.hansson@linaro.org
State New
Headers show
Series
  • mmc: core: Update timeouts for __mmc_switch()
Related show

Commit Message

Ulf Hansson Jan. 22, 2020, 2:27 p.m.
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

Comments

Paul Fertser Feb. 22, 2021, 4:24 p.m. | #1
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
Paul Fertser Feb. 22, 2021, 8:12 p.m. | #2
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
Ulf Hansson Feb. 23, 2021, 9:23 a.m. | #3
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
Paul Fertser Feb. 23, 2021, 9:32 a.m. | #4
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
Ulf Hansson Feb. 23, 2021, 10:44 a.m. | #5
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
Adrian Hunter Feb. 23, 2021, 11:01 a.m. | #6
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.
Paul Fertser Feb. 23, 2021, 11:19 a.m. | #7
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
Ulf Hansson Feb. 23, 2021, 11:54 a.m. | #8
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
Adrian Hunter Feb. 23, 2021, 1:42 p.m. | #9
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;
Ulf Hansson Feb. 24, 2021, 11:09 a.m. | #10
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

Patch

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;