RE: [PATCH] mmc: sd: Add a variable to check a faulty device

Posted by 이승희 1 year, 10 months ago



> -----Original Message-----
> From: Avri Altman <Avri.Altman@wdc.com>
> Sent: Tuesday, February 13, 2024 5:42 PM
> To: Seunghui Lee <sh043.lee@samsung.com>; linux-mmc@vger.kernel.org;
> linux-kernel@vger.kernel.org; ulf.hansson@linaro.org;
> gregkh@linuxfoundation.org
> Cc: grant.jung@samsung.com; jt77.jang@samsung.com;
> dh0421.hwang@samsung.com; junwoo80.lee@samsung.com; jangsub.yi@samsung.com;
> cw9316.lee@samsung.com; sh8267.baek@samsung.com; wkon.kim@samsung.com
> Subject: RE: [PATCH] mmc: sd: Add a variable to check a faulty device
> 
> > In mobile devices, suspend/resume situations are frequent.
> > In the case of a defective SD card in which initialization fails,
> > unnecessary initialization time is consumed for each resume.
> > A field is needed to check that SD card initialization has failed on
> > the host. It could be used to remove unnecessary initialization.
> I don't see where you are using this new init_failed field?
> Maybe instead, elaborate the logic to free_card: to detect a broken sd.
> e.g. instead of just if (!oldcard), if (!oldcard || ! mmc_sd_alive(host))
> or something.
> 
> Thanks,
> Avri
> 
Thank you for your suggestion.
I'm going to use it in mmc_rescan as below.

e.g.
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index a8c17b4cd737..461cd75dc7ab 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2210,7 +2210,7 @@ void mmc_rescan(struct work_struct *work)
                container_of(work, struct mmc_host, detect.work);
        int i;
 
-       if (host->rescan_disable)
+       if (host->rescan_disable || host->init_failed)
                return;
> >
> > Signed-off-by: Seunghui Lee <sh043.lee@samsung.com>
> > ---
> >  drivers/mmc/core/sd.c        | 12 +++++++++++-
> >  drivers/mmc/core/slot-gpio.c |  1 +
> >  include/linux/mmc/host.h     |  1 +
> >  3 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index
> > c3e554344c99..f0eb3864dc24 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -1410,6 +1410,7 @@ static int mmc_sd_init_card(struct mmc_host
> > *host,
> > u32 ocr,
> >         bool v18_fixup_failed = false;
> >
> >         WARN_ON(!host->claimed);
> > +       host->init_failed = false;
> >  retry:
> >         err = mmc_sd_get_cid(host, ocr, cid, &rocr);
> >         if (err)
> > @@ -1752,6 +1753,8 @@ static int _mmc_sd_resume(struct mmc_host *host)
> >
> >         mmc_power_up(host, host->card->ocr);
> >         err = mmc_sd_init_card(host, host->card->ocr, host->card);
> > +       if (err)
> > +               host->init_failed = true;
> >         mmc_card_clr_suspended(host->card);
> >
> >  out:
> > @@ -1803,8 +1806,12 @@ static int mmc_sd_runtime_resume(struct
> > mmc_host *host)
> >
> >  static int mmc_sd_hw_reset(struct mmc_host *host)  {
> > +       int err;
> >         mmc_power_cycle(host, host->card->ocr);
> > -       return mmc_sd_init_card(host, host->card->ocr, host->card);
> > +       err = mmc_sd_init_card(host, host->card->ocr, host->card);
> > +       if (err)
> > +               host->init_failed = true;
> > +       return err;
> >  }
> >
> >  static const struct mmc_bus_ops mmc_sd_ops = { @@ -1891,5 +1898,8 @@
> > int mmc_attach_sd(struct mmc_host *host)
> >         pr_err("%s: error %d whilst initialising SD card\n",
> >                 mmc_hostname(host), err);
> >
> > +       if (err)
> > +               host->init_failed = true;
> > +
> >         return err;
> >  }
> > diff --git a/drivers/mmc/core/slot-gpio.c
> > b/drivers/mmc/core/slot-gpio.c index
> > 2a2d949a9344..93d081c7dd53 100644
> > --- a/drivers/mmc/core/slot-gpio.c
> > +++ b/drivers/mmc/core/slot-gpio.c
> > @@ -33,6 +33,7 @@ static irqreturn_t mmc_gpio_cd_irqt(int irq, void
> *dev_id)
> >         struct mmc_gpio *ctx = host->slot.handler_priv;
> >
> >         host->trigger_card_event = true;
> > +       host->init_failed = false;
> >         mmc_detect_change(host,
> > msecs_to_jiffies(ctx->cd_debounce_delay_ms));
> >
> >         return IRQ_HANDLED;
> > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index
> > 2f445c651742..1d75cfdbf981 100644
> > --- a/include/linux/mmc/host.h
> > +++ b/include/linux/mmc/host.h
> > @@ -467,6 +467,7 @@ struct mmc_host {
> >         struct timer_list       retune_timer;   /* for periodic re-tuning */
> >
> >         bool                    trigger_card_event; /* card_event necessary */
> > +       bool                    init_failed;    /* check if failed to
> initialize */
> >
> >         struct mmc_card         *card;          /* device attached to this host
> */
> >
> > --
> > 2.29.0

Re: [PATCH] mmc: sd: Add a variable to check a faulty device

Posted by Christian Loehle 1 year, 10 months ago

On 13/02/2024 09:49, 이승희 wrote:
> 
> 
>> -----Original Message-----
>> From: Avri Altman <Avri.Altman@wdc.com>
>> Sent: Tuesday, February 13, 2024 5:42 PM
>> To: Seunghui Lee <sh043.lee@samsung.com>; linux-mmc@vger.kernel.org;
>> linux-kernel@vger.kernel.org; ulf.hansson@linaro.org;
>> gregkh@linuxfoundation.org
>> Cc: grant.jung@samsung.com; jt77.jang@samsung.com;
>> dh0421.hwang@samsung.com; junwoo80.lee@samsung.com; jangsub.yi@samsung.com;
>> cw9316.lee@samsung.com; sh8267.baek@samsung.com; wkon.kim@samsung.com
>> Subject: RE: [PATCH] mmc: sd: Add a variable to check a faulty device
>>
>>> In mobile devices, suspend/resume situations are frequent.
>>> In the case of a defective SD card in which initialization fails,
>>> unnecessary initialization time is consumed for each resume.
>>> A field is needed to check that SD card initialization has failed on
>>> the host. It could be used to remove unnecessary initialization.
>> I don't see where you are using this new init_failed field?
>> Maybe instead, elaborate the logic to free_card: to detect a broken sd.
>> e.g. instead of just if (!oldcard), if (!oldcard || ! mmc_sd_alive(host))
>> or something.
>>
>> Thanks,
>> Avri
>>
> Thank you for your suggestion.
> I'm going to use it in mmc_rescan as below.
> 
> e.g.
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index a8c17b4cd737..461cd75dc7ab 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2210,7 +2210,7 @@ void mmc_rescan(struct work_struct *work)
>                 container_of(work, struct mmc_host, detect.work);
>         int i;
>  
> -       if (host->rescan_disable)
> +       if (host->rescan_disable || host->init_failed)
>                 return;

I've seen SD cards that fail the first initialization attempt for both
'valid' reasons (e.g. weird insertion timing) and things like out-of-spec
initialization time from the card, outright disabling these on the first
fail is a bit too much IMO.

Kind Regards,
Christian

RE: [PATCH] mmc: sd: Add a variable to check a faulty device

Posted by 이승희 1 year, 10 months ago

> -----Original Message-----
> From: Christian Loehle <christian.loehle@arm.com>
> Sent: Tuesday, February 13, 2024 10:35 PM
> To: 이승희 <sh043.lee@samsung.com>; 'Avri Altman' <Avri.Altman@wdc.com>;
> linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org;
> ulf.hansson@linaro.org; gregkh@linuxfoundation.org
> Cc: grant.jung@samsung.com; jt77.jang@samsung.com;
> dh0421.hwang@samsung.com; junwoo80.lee@samsung.com; jangsub.yi@samsung.com;
> cw9316.lee@samsung.com; sh8267.baek@samsung.com; wkon.kim@samsung.com
> Subject: Re: [PATCH] mmc: sd: Add a variable to check a faulty device
> 
> On 13/02/2024 09:49, 이승희 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Avri Altman <Avri.Altman@wdc.com>
> >> Sent: Tuesday, February 13, 2024 5:42 PM
> >> To: Seunghui Lee <sh043.lee@samsung.com>; linux-mmc@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; ulf.hansson@linaro.org;
> >> gregkh@linuxfoundation.org
> >> Cc: grant.jung@samsung.com; jt77.jang@samsung.com;
> >> dh0421.hwang@samsung.com; junwoo80.lee@samsung.com;
> >> jangsub.yi@samsung.com; cw9316.lee@samsung.com;
> >> sh8267.baek@samsung.com; wkon.kim@samsung.com
> >> Subject: RE: [PATCH] mmc: sd: Add a variable to check a faulty device
> >>
> >>> In mobile devices, suspend/resume situations are frequent.
> >>> In the case of a defective SD card in which initialization fails,
> >>> unnecessary initialization time is consumed for each resume.
> >>> A field is needed to check that SD card initialization has failed on
> >>> the host. It could be used to remove unnecessary initialization.
> >> I don't see where you are using this new init_failed field?
> >> Maybe instead, elaborate the logic to free_card: to detect a broken sd.
> >> e.g. instead of just if (!oldcard), if (!oldcard || !
> >> mmc_sd_alive(host)) or something.
> >>
> >> Thanks,
> >> Avri
> >>
> > Thank you for your suggestion.
> > I'm going to use it in mmc_rescan as below.
> >
> > e.g.
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index
> > a8c17b4cd737..461cd75dc7ab 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2210,7 +2210,7 @@ void mmc_rescan(struct work_struct *work)
> >                 container_of(work, struct mmc_host, detect.work);
> >         int i;
> >
> > -       if (host->rescan_disable)
> > +       if (host->rescan_disable || host->init_failed)
> >                 return;
> 
> I've seen SD cards that fail the first initialization attempt for both
> 'valid' reasons (e.g. weird insertion timing) and things like out-of-spec
> initialization time from the card, outright disabling these on the first
> fail is a bit too much IMO.
> 
> Kind Regards,
> Christian

I agree with you. It's a bit too much.
It's just simple example.

Anyway, It's difficult to distinguish between NO card and a faulty card.
That's why I'd like to merge this.
It helps that we check if it's a faulty card or not in a dump.