mmc_rescan failure in case of CONFIG_MMC_UNSAFE_RESUME

mmc_rescan failure in case of CONFIG_MMC_UNSAFE_RESUME

[Dmitry Shmidt]

Hello,

Recently new check was added to core.c function mmc_rescan():
    if (host->bus_ops && host->bus_ops->detect && !host->bus_dead
        && mmc_card_is_removable(host)) <<<< This one
        host->bus_ops->detect(host);
mmc_card_is_removable() is checking
 !(host->caps & MMC_CAP_NONREMOVABLE) && mmc_assume_removable;

If we use CONFIG_MMC_UNSAFE_RESUME then
  mmc_assume_removable will be 0 and any card will be always considered
as non-removable. And host->bus_ops->detect() will not be called on card
removal.

Am I missing something?
Thanks,

Dmitry

Re: mmc_rescan failure in case of CONFIG_MMC_UNSAFE_RESUME

[Chris Ball]

Hi Dmitry,

On Sat, Feb 12, 2011 at 12:33:33AM +0000, Dmitry Shmidt wrote:
> Recently new check was added to core.c function mmc_rescan():
>     if (host->bus_ops && host->bus_ops->detect && !host->bus_dead
>         && mmc_card_is_removable(host)) <<<< This one
>         host->bus_ops->detect(host);
> mmc_card_is_removable() is checking
>  !(host->caps & MMC_CAP_NONREMOVABLE) && mmc_assume_removable;
> 
> If we use CONFIG_MMC_UNSAFE_RESUME then
>   mmc_assume_removable will be 0 and any card will be always considered
> as non-removable. And host->bus_ops->detect() will not be called on card
> removal.

I agree that we've changed the behavior to avoid running ->detect
in this case, but that was intentional -- you should not be using
CONFIG_MMC_UNSAFE_RESUME on a card that is physically removable.
Why are you trying to?

config MMC_UNSAFE_RESUME
        bool "Assume MMC/SD cards are non-removable (DANGEROUS)"
        ...

Thanks,

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: mmc_rescan failure in case of CONFIG_MMC_UNSAFE_RESUME

[Dmitry Shmidt]

Hi Chris,

On Sat, Feb 12, 2011 at 9:22 AM, Chris Ball <cjb@laptop.org> wrote:
> Hi Dmitry,
>
> On Sat, Feb 12, 2011 at 12:33:33AM +0000, Dmitry Shmidt wrote:
>> Recently new check was added to core.c function mmc_rescan():
>>     if (host->bus_ops && host->bus_ops->detect && !host->bus_dead
>>         && mmc_card_is_removable(host)) <<<< This one
>>         host->bus_ops->detect(host);
>> mmc_card_is_removable() is checking
>>  !(host->caps & MMC_CAP_NONREMOVABLE) && mmc_assume_removable;
>>
>> If we use CONFIG_MMC_UNSAFE_RESUME then
>>   mmc_assume_removable will be 0 and any card will be always considered
>> as non-removable. And host->bus_ops->detect() will not be called on card
>> removal.
>
> I agree that we've changed the behavior to avoid running ->detect
> in this case, but that was intentional -- you should not be using
> CONFIG_MMC_UNSAFE_RESUME on a card that is physically removable.
> Why are you trying to?

MMC_UNSAFE_RESUME is affecting mmc_sdio_resume() sequence. If it is
not defined then sdio card will be considered
"removable" and on resume mmc_sdio_init_card() will be always called.

static int mmc_sdio_resume(struct mmc_host *host)
{
     ...
     if (mmc_card_is_removable(host) || !mmc_card_is_powered_resumed(host))
          err = mmc_sdio_init_card(host, host->ocr, host->card,
                        (host->pm_flags & MMC_PM_KEEP_POWER));


>
> config MMC_UNSAFE_RESUME
>        bool "Assume MMC/SD cards are non-removable (DANGEROUS)"
>        ...
>
> Thanks,
>
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
>

Thanks,
Dmitry

Re: mmc_rescan failure in case of CONFIG_MMC_UNSAFE_RESUME

[Chris Ball]

Hi Dmitry,

[Cc += Nico]

On Mon, Feb 14, 2011 at 11:04:13AM -0800, Dmitry Shmidt wrote:
> MMC_UNSAFE_RESUME is affecting mmc_sdio_resume() sequence. If it is
> not defined then sdio card will be considered
> "removable" and on resume mmc_sdio_init_card() will be always called.
> 
> static int mmc_sdio_resume(struct mmc_host *host)
> {
>      ...
>      if (mmc_card_is_removable(host) || !mmc_card_is_powered_resumed(host))
>           err = mmc_sdio_init_card(host, host->ocr, host->card,
>                         (host->pm_flags & MMC_PM_KEEP_POWER));

mmc_sdio_init_card() is supposed to be called if your card is removable,
because the card might have changed.  It has a fast path that's enabled
by MMC_PM_KEEP_POWER.  (Nicolas explained this back in October.)

You absolutely should not enable MMC_UNSAFE_RESUME if your host has a
removable card.  It's an awful hack, and you just found an example of
where it breaks.

If you need mmc_sdio_resume() to have an even faster path for you, we can
talk about that and see if it makes sense.  Misusing MMC_UNSAFE_RESUME to
get a powered SDIO resume is just wrong.

Thanks,

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: mmc_rescan failure in case of CONFIG_MMC_UNSAFE_RESUME

[Dmitry Shmidt]

Hi Chris,

On Mon, Feb 14, 2011 at 11:40 AM, Chris Ball <cjb@laptop.org> wrote:
> Hi Dmitry,
>
> [Cc += Nico]
>
> On Mon, Feb 14, 2011 at 11:04:13AM -0800, Dmitry Shmidt wrote:
>> MMC_UNSAFE_RESUME is affecting mmc_sdio_resume() sequence. If it is
>> not defined then sdio card will be considered
>> "removable" and on resume mmc_sdio_init_card() will be always called.
>>
>> static int mmc_sdio_resume(struct mmc_host *host)
>> {
>>      ...
>>      if (mmc_card_is_removable(host) || !mmc_card_is_powered_resumed(host))
>>           err = mmc_sdio_init_card(host, host->ocr, host->card,
>>                         (host->pm_flags & MMC_PM_KEEP_POWER));
>
> mmc_sdio_init_card() is supposed to be called if your card is removable,
> because the card might have changed.  It has a fast path that's enabled
> by MMC_PM_KEEP_POWER.  (Nicolas explained this back in October.)
>
> You absolutely should not enable MMC_UNSAFE_RESUME if your host has a
> removable card.  It's an awful hack, and you just found an example of
> where it breaks.

My main concern was that here we are mixing two things: removable
cards and cards that stay powered and need quick resume.
It is possible to assume that last ones are not removable, but I feel
it is not 100% correct. For example you may have
removable wlan sdio adaptor.

> If you need mmc_sdio_resume() to have an even faster path for you, we can
> talk about that and see if it makes sense.  Misusing MMC_UNSAFE_RESUME to
> get a powered SDIO resume is just wrong.

So what can we do to suppress resume for removable sdio card ?

>
> Thanks,
>
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
>

Thanks,

Dmitry

Re: mmc_rescan failure in case of CONFIG_MMC_UNSAFE_RESUME

[Dmitry Shmidt]

Hi,

[Cc += Ohad]
On Mon, Feb 14, 2011 at 1:54 PM, Dmitry Shmidt <dimitrysh@android.com> wrote:
> Hi Chris,
>
> On Mon, Feb 14, 2011 at 11:40 AM, Chris Ball <cjb@laptop.org> wrote:
>> Hi Dmitry,
>>
>> [Cc += Nico]
>>
>> On Mon, Feb 14, 2011 at 11:04:13AM -0800, Dmitry Shmidt wrote:
>>> MMC_UNSAFE_RESUME is affecting mmc_sdio_resume() sequence. If it is
>>> not defined then sdio card will be considered
>>> "removable" and on resume mmc_sdio_init_card() will be always called.
>>>
>>> static int mmc_sdio_resume(struct mmc_host *host)
>>> {
>>>      ...
>>>      if (mmc_card_is_removable(host) || !mmc_card_is_powered_resumed(host))
>>>           err = mmc_sdio_init_card(host, host->ocr, host->card,
>>>                         (host->pm_flags & MMC_PM_KEEP_POWER));
>>
>> mmc_sdio_init_card() is supposed to be called if your card is removable,
>> because the card might have changed.  It has a fast path that's enabled
>> by MMC_PM_KEEP_POWER.  (Nicolas explained this back in October.)
>>
>> You absolutely should not enable MMC_UNSAFE_RESUME if your host has a
>> removable card.  It's an awful hack, and you just found an example of
>> where it breaks.
>
> My main concern was that here we are mixing two things: removable
> cards and cards that stay powered and need quick resume.
> It is possible to assume that last ones are not removable, but I feel
> it is not 100% correct. For example you may have
> removable wlan sdio adaptor.
>
>> If you need mmc_sdio_resume() to have an even faster path for you, we can
>> talk about that and see if it makes sense.  Misusing MMC_UNSAFE_RESUME to
>> get a powered SDIO resume is just wrong.
>
> So what can we do to suppress resume for removable sdio card ?
>
>>
>> Thanks,
>>
>> --
>> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
>> One Laptop Per Child
>>
As I wrote, Ohad's patch resolves quick resume situation for
non-removable cards, but in case of
removable - it is not working.

Thanks,

Dmitry

Re: mmc_rescan failure in case of CONFIG_MMC_UNSAFE_RESUME

[Dmitry Shmidt]

Hi,

On Tue, Feb 15, 2011 at 10:06 AM, Dmitry Shmidt <dimitrysh@android.com> wrote:
> Hi,
>
> [Cc += Ohad]
> On Mon, Feb 14, 2011 at 1:54 PM, Dmitry Shmidt <dimitrysh@android.com> wrote:
>> Hi Chris,
>>
>> On Mon, Feb 14, 2011 at 11:40 AM, Chris Ball <cjb@laptop.org> wrote:
>>> Hi Dmitry,
>>>
>>> [Cc += Nico]
>>>
>>> On Mon, Feb 14, 2011 at 11:04:13AM -0800, Dmitry Shmidt wrote:
>>>> MMC_UNSAFE_RESUME is affecting mmc_sdio_resume() sequence. If it is
>>>> not defined then sdio card will be considered
>>>> "removable" and on resume mmc_sdio_init_card() will be always called.
>>>>
>>>> static int mmc_sdio_resume(struct mmc_host *host)
>>>> {
>>>>      ...
>>>>      if (mmc_card_is_removable(host) || !mmc_card_is_powered_resumed(host))
>>>>           err = mmc_sdio_init_card(host, host->ocr, host->card,
>>>>                         (host->pm_flags & MMC_PM_KEEP_POWER));
>>>
>>> mmc_sdio_init_card() is supposed to be called if your card is removable,
>>> because the card might have changed.  It has a fast path that's enabled
>>> by MMC_PM_KEEP_POWER.  (Nicolas explained this back in October.)
>>>
>>> You absolutely should not enable MMC_UNSAFE_RESUME if your host has a
>>> removable card.  It's an awful hack, and you just found an example of
>>> where it breaks.
>>
>> My main concern was that here we are mixing two things: removable
>> cards and cards that stay powered and need quick resume.
>> It is possible to assume that last ones are not removable, but I feel
>> it is not 100% correct. For example you may have
>> removable wlan sdio adaptor.
>>
>>> If you need mmc_sdio_resume() to have an even faster path for you, we can
>>> talk about that and see if it makes sense.  Misusing MMC_UNSAFE_RESUME to
>>> get a powered SDIO resume is just wrong.
>>
>> So what can we do to suppress resume for removable sdio card ?
>>
>>>
>>> Thanks,
>>>
>>> --
>>> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
>>> One Laptop Per Child
>>>
> As I wrote, Ohad's patch resolves quick resume situation for
> non-removable cards, but in case of
> removable - it is not working.
>
> Thanks,
>
> Dmitry
>
As another point of inconsistency, the flag that is set by
MMC_UNSAFE_RESUME is global,
meaning that when set to 0, it makes all cards non-removable,
regardless of caps property.

Thanks,

Dmitry