[PATCH] mmc: core: sd: check card write-protect lock while resuming

[Ulf Hansson]

On 22 August 2014 08:55, Barry Song <Barry.Song at csr.com> wrote:
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson at linaro.org]
>> Sent: Monday, August 18, 2014 7:58 PM
>> To: Barry Song
>> Cc: Chris Ball; linux-mmc; linux-arm-kernel at lists.infradead.org; DL-SHA-
>> WorkGroupLinux; Minda Chen; Barry Song
>> Subject: Re: [PATCH] mmc: core: sd: check card write-protect lock while
>> resuming
>>
>> On 18 August 2014 12:00, Barry Song <Barry.Song at csr.com> wrote:
>> > From: Minda Chen <Minda.Chen at csr.com>
>> >
>> > After suspending, unplug the sdcard, and set sd WP lock, insert it
>> > again, then resume the system. resume codes do not check the the
>> > sdcard write-proctect lock. now check it.
>> >
>> > Signed-off-by: Minda Chen <Minda.Chen at csr.com>
>> > Signed-off-by: Barry Song <Baohua.Song at csr.com>
>> > ---
>> >  drivers/mmc/core/sd.c | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index
>> > 0c44510..890557a 100644
>> > --- a/drivers/mmc/core/sd.c
>> > +++ b/drivers/mmc/core/sd.c
>> > @@ -910,6 +910,7 @@ static int mmc_sd_init_card(struct mmc_host
>> *host, u32 ocr,
>> >         int err;
>> >         u32 cid[4];
>> >         u32 rocr = 0;
>> > +       bool oldro, ro;
>> >
>> >         BUG_ON(!host);
>> >         WARN_ON(!host->claimed);
>> > @@ -922,6 +923,12 @@ static int mmc_sd_init_card(struct mmc_host
>> *host, u32 ocr,
>> >                 if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0)
>> >                         return -ENOENT;
>> >
>> > +               if (host->ops->get_ro) {
>> > +                       ro = host->ops->get_ro(host) ? true : false;
>> > +                       oldro = mmc_card_readonly(oldcard) ? true : false;
>> > +                       if (oldro ^ ro)
>> > +                               return -ENOENT;
>> > +               }
>>
>> Seems like this code belongs better in mmc_sd_setup_card(). Could you try
>> moving it there.
> [Barry Song]
> Well. How about:
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 0c44510..643bc82d8 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -815,6 +815,7 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
>         bool reinit)
>  {
>         int err;
> +       int oldro, ro = -1;
>
>         if (!reinit) {
>                 /*
> @@ -861,23 +862,28 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card,
>         /*
>          * Check if read-only switch is active.
>          */
> -       if (!reinit) {
> -               int ro = -1;
>
> -               if (host->ops->get_ro) {
> -                       mmc_host_clk_hold(card->host);
> -                       ro = host->ops->get_ro(host);
> -                       mmc_host_clk_release(card->host);
> -               }
> +       if (host->ops->get_ro) {
> +               mmc_host_clk_hold(card->host);
> +               ro = host->ops->get_ro(host);
> +               mmc_host_clk_release(card->host);
> +       }
>
> -               if (ro < 0) {
> -                       pr_warning("%s: host does not "
> -                               "support reading read-only "
> -                               "switch. assuming write-enable.\n",
> -                               mmc_hostname(host));
> -               } else if (ro > 0) {
> -                       mmc_card_set_readonly(card);
> -               }
> +       if (ro < 0)
> +               pr_warning("%s: host does not "
> +                       "support reading read-only "
> +                       "switch. assuming write-enable.\n",
> +                       mmc_hostname(host));

I don't wont this pr_warning to spam the log each resume.
I suppose we could print it only for !reinit, right?

> +
> +       if (!reinit && (ro > 0))
> +               mmc_card_set_readonly(card);
> +       else if (reinit && (ro >= 0)) {
> +               /* check if the write-protection lock is changed */
> +               oldro = mmc_card_readonly(card) ? 1 : 0;
> +               ro = (ro > 0) ? 1 : 0;
> +
> +               if (oldro ^ ro)
> +                       return -ENOENT;

This will have the effect of quickly returning an error to the mmc
bus' ->suspend callback. That path are not able remove the card
device, which I guess is what you would like to happen, right?

I am not completely sure, but I believe the card will still be
operational after resuming. Moreover it will be using the
initialization frequency and 1-bit bus width and thus performing
poorly. This is definitely not what we want.

How did you test this patch? Can you confirm my theory above?

The card may only be removed from the mmc_rescan work, scheduled at
PM_POST_SUSPEND phase. What I think you should do here, is to also set
the card state into MMC_CARD_REMOVED, by invoking
mmc_card_set_removed(). Thus the SD's bus_ops->detect callback will
remove the card during this phase.

Kind regards
Uffe