[PATCH v3] mmc: core: Issue power off notification in mmc_remove()

[PATCH v3] mmc: core: Issue power off notification in mmc_remove()

[Yoshihiro Shimoda]

User is possible to turn the power off after a host was removed.
So, call mmc_poweroff_notify(EXT_CSD_NO_POWER_NOTIFICATION)
in mmc_remove(). Note that the mmc and host driver will be
in the following modes when mmc_remove() is called:

 1. mmc_card_suspended() == false &&
    power_off_notification == EXT_CSD_POWER_ON
 2. mmc_card_suspended() == true &&
    power_off_notification == EXT_CSD_POWER_OFF_{SHORT,LONG}
 3. mmc_card_suspended() == true && mmc_sleep() was called

So, mmc_remove() calls _mmc_resume() anyway for the cases.
Otherwise:

 - _mmc_resume will be called via mmc_runtime_resume() and then
   power_off_notification will be set to EXT_CSD_POWER_ON.
 - timeout will happen in mmc_blk_part_switch() via mmc_blk_remove()
   if "part_curr" of mmc block is not set to default.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Changes from v2:
 - Fix an issue which timeout happens if part_curr is not default.
 https://patchwork.kernel.org/project/linux-renesas-soc/patch/1604311475-15307-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/

 Changes from v1:
 - Reuse _mmc_suspend() instead of direct mmc_poweroff_notify() calling
  to check suspended flag while removing.
  https://patchwork.kernel.org/project/linux-renesas-soc/patch/1602581312-23607-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/


 drivers/mmc/core/mmc.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index ff3063c..18413f2 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1983,11 +1983,35 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
 	return err;
 }
 
+static int _mmc_resume(struct mmc_host *host);
 /*
  * Host is being removed. Free up the current card.
  */
 static void mmc_remove(struct mmc_host *host)
 {
+	/*
+	 * The mmc and host driver will be in the following modes here:
+	 *  1. mmc_card_suspended() == false &&
+	 *     power_off_notification == EXT_CSD_POWER_ON
+	 *  2. mmc_card_suspended() == true &&
+	 *     power_off_notification == EXT_CSD_POWER_OFF_{SHORT,LONG}
+	 *  3. mmc_card_suspended() == true && mmc_sleep() was called
+	 *
+	 * So, call _mmc_resume() here anyway for the cases. Otherwise:
+	 *  - _mmc_resume will be called via mmc_runtime_resume() and then
+	 *    power_off_notification will be set to EXT_CSD_POWER_ON.
+	 *  - timeout will happen in mmc_blk_part_switch() via mmc_blk_remove()
+	 *    if "part_curr" of mmc block is not set to default.
+	 */
+	_mmc_resume(host);
+
+	/* Disable power_off_notification byte in the ext_csd register */
+	if (host->card->ext_csd.rev >= 6) {
+		mmc_claim_host(host);
+		mmc_poweroff_notify(host->card, EXT_CSD_NO_POWER_NOTIFICATION);
+		mmc_release_host(host);
+	}
+
 	mmc_remove_card(host->card);
 	host->card = NULL;
 }
-- 
2.7.4

Re: [PATCH v3] mmc: core: Issue power off notification in mmc_remove()

[Ulf Hansson]

On Tue, 10 Nov 2020 at 11:48, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>
> User is possible to turn the power off after a host was removed.
> So, call mmc_poweroff_notify(EXT_CSD_NO_POWER_NOTIFICATION)
> in mmc_remove(). Note that the mmc and host driver will be
> in the following modes when mmc_remove() is called:
>
>  1. mmc_card_suspended() == false &&
>     power_off_notification == EXT_CSD_POWER_ON
>  2. mmc_card_suspended() == true &&
>     power_off_notification == EXT_CSD_POWER_OFF_{SHORT,LONG}
>  3. mmc_card_suspended() == true && mmc_sleep() was called
>
> So, mmc_remove() calls _mmc_resume() anyway for the cases.
> Otherwise:
>
>  - _mmc_resume will be called via mmc_runtime_resume() and then
>    power_off_notification will be set to EXT_CSD_POWER_ON.
>  - timeout will happen in mmc_blk_part_switch() via mmc_blk_remove()
>    if "part_curr" of mmc block is not set to default.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  Changes from v2:
>  - Fix an issue which timeout happens if part_curr is not default.
>  https://patchwork.kernel.org/project/linux-renesas-soc/patch/1604311475-15307-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/
>
>  Changes from v1:
>  - Reuse _mmc_suspend() instead of direct mmc_poweroff_notify() calling
>   to check suspended flag while removing.
>   https://patchwork.kernel.org/project/linux-renesas-soc/patch/1602581312-23607-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/
>
>
>  drivers/mmc/core/mmc.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index ff3063c..18413f2 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1983,11 +1983,35 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
>         return err;
>  }
>
> +static int _mmc_resume(struct mmc_host *host);
>  /*
>   * Host is being removed. Free up the current card.
>   */
>  static void mmc_remove(struct mmc_host *host)
>  {
> +       /*
> +        * The mmc and host driver will be in the following modes here:
> +        *  1. mmc_card_suspended() == false &&
> +        *     power_off_notification == EXT_CSD_POWER_ON
> +        *  2. mmc_card_suspended() == true &&
> +        *     power_off_notification == EXT_CSD_POWER_OFF_{SHORT,LONG}
> +        *  3. mmc_card_suspended() == true && mmc_sleep() was called
> +        *
> +        * So, call _mmc_resume() here anyway for the cases. Otherwise:
> +        *  - _mmc_resume will be called via mmc_runtime_resume() and then
> +        *    power_off_notification will be set to EXT_CSD_POWER_ON.
> +        *  - timeout will happen in mmc_blk_part_switch() via mmc_blk_remove()
> +        *    if "part_curr" of mmc block is not set to default.
> +        */
> +       _mmc_resume(host);
> +
> +       /* Disable power_off_notification byte in the ext_csd register */
> +       if (host->card->ext_csd.rev >= 6) {
> +               mmc_claim_host(host);
> +               mmc_poweroff_notify(host->card, EXT_CSD_NO_POWER_NOTIFICATION);
> +               mmc_release_host(host);
> +       }

Unfortunate, I think there is even more complexity involved here. I
don't think the above work for all cases.

Let me try to elaborate - there are two scenarios of when mmc_remove()
is called.

1)
When the card becomes removed, likely not the case for eMMC but it may
happen for a legacy MMC card, for example. In this case, there is not
much we can do to fix the problem, as the card is already "dead".

2)
The card is working properly (it may be suspended though) while
mmc_remove_host() gets called because the host driver is being
unbinded.

For 1)
We should only clean up and remove the card structs, which the current
code already does.

For 2)
We want to support a graceful power off sequence or the card (to
prevent data corruption for example). However, depending on the
platform and host, it may not be possible to power off both VCC and
VCCQ. For example, it's quite common that VCCQ remains powered on,
while only VCC can be power gated. Just disabling the power off
notification of the eMMC card (as you suggest above), doesn't really
help. In fact, it could mean that we may violate the eMMC spec when
power gating VCC through mmc_power_off().

I am thinking of a few possible solutions. Perhaps easier if I post a
patch that you try - unless you have ideas yourself of how to fix
this.

> +
>         mmc_remove_card(host->card);
>         host->card = NULL;
>  }
> --
> 2.7.4
>

Kind regards
Uffe

RE: [PATCH v3] mmc: core: Issue power off notification in mmc_remove()

[Yoshihiro Shimoda]

Hello Ulf,

> From: Ulf Hansson, Sent: Tuesday, November 17, 2020 1:47 AM
> 
> On Tue, 10 Nov 2020 at 11:48, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> >
> > User is possible to turn the power off after a host was removed.
> > So, call mmc_poweroff_notify(EXT_CSD_NO_POWER_NOTIFICATION)
> > in mmc_remove(). Note that the mmc and host driver will be
> > in the following modes when mmc_remove() is called:
> >
> >  1. mmc_card_suspended() == false &&
> >     power_off_notification == EXT_CSD_POWER_ON
> >  2. mmc_card_suspended() == true &&
> >     power_off_notification == EXT_CSD_POWER_OFF_{SHORT,LONG}
> >  3. mmc_card_suspended() == true && mmc_sleep() was called
> >
> > So, mmc_remove() calls _mmc_resume() anyway for the cases.
> > Otherwise:
> >
> >  - _mmc_resume will be called via mmc_runtime_resume() and then
> >    power_off_notification will be set to EXT_CSD_POWER_ON.
> >  - timeout will happen in mmc_blk_part_switch() via mmc_blk_remove()
> >    if "part_curr" of mmc block is not set to default.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
<snip>
> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > index ff3063c..18413f2 100644
> > --- a/drivers/mmc/core/mmc.c
> > +++ b/drivers/mmc/core/mmc.c
> > @@ -1983,11 +1983,35 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
> >         return err;
> >  }
> >
> > +static int _mmc_resume(struct mmc_host *host);
> >  /*
> >   * Host is being removed. Free up the current card.
> >   */
> >  static void mmc_remove(struct mmc_host *host)
> >  {
> > +       /*
> > +        * The mmc and host driver will be in the following modes here:
> > +        *  1. mmc_card_suspended() == false &&
> > +        *     power_off_notification == EXT_CSD_POWER_ON
> > +        *  2. mmc_card_suspended() == true &&
> > +        *     power_off_notification == EXT_CSD_POWER_OFF_{SHORT,LONG}
> > +        *  3. mmc_card_suspended() == true && mmc_sleep() was called
> > +        *
> > +        * So, call _mmc_resume() here anyway for the cases. Otherwise:
> > +        *  - _mmc_resume will be called via mmc_runtime_resume() and then
> > +        *    power_off_notification will be set to EXT_CSD_POWER_ON.
> > +        *  - timeout will happen in mmc_blk_part_switch() via mmc_blk_remove()
> > +        *    if "part_curr" of mmc block is not set to default.
> > +        */
> > +       _mmc_resume(host);
> > +
> > +       /* Disable power_off_notification byte in the ext_csd register */
> > +       if (host->card->ext_csd.rev >= 6) {
> > +               mmc_claim_host(host);
> > +               mmc_poweroff_notify(host->card, EXT_CSD_NO_POWER_NOTIFICATION);
> > +               mmc_release_host(host);
> > +       }
> 
> Unfortunate, I think there is even more complexity involved here. I
> don't think the above work for all cases.
> 
> Let me try to elaborate - there are two scenarios of when mmc_remove()
> is called.
> 
> 1)
> When the card becomes removed, likely not the case for eMMC but it may
> happen for a legacy MMC card, for example. In this case, there is not
> much we can do to fix the problem, as the card is already "dead".
> 
> 2)
> The card is working properly (it may be suspended though) while
> mmc_remove_host() gets called because the host driver is being
> unbinded.

Thank you for your review and comments! I understood my patch was not
enough for these scenarios.

> For 1)
> We should only clean up and remove the card structs, which the current
> code already does.
> 
> For 2)
> We want to support a graceful power off sequence or the card (to
> prevent data corruption for example). However, depending on the
> platform and host, it may not be possible to power off both VCC and
> VCCQ. For example, it's quite common that VCCQ remains powered on,
> while only VCC can be power gated. Just disabling the power off
> notification of the eMMC card (as you suggest above), doesn't really
> help. In fact, it could mean that we may violate the eMMC spec when
> power gating VCC through mmc_power_off().
> 
> I am thinking of a few possible solutions. Perhaps easier if I post a
> patch that you try - unless you have ideas yourself of how to fix
> this.

Thank you! Unfortunately, I don't have any idea how to fix this now.
So, if you post a patch, I'll try.

Best regards,
Yoshihiro Shimoda