| Message ID | 20240213051716.6596-1-sh043.lee@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: sd: Add a variable to check a faulty device | expand |
> -----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
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
> -----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.
On Tue, 13 Feb 2024 at 06:13, Seunghui Lee <sh043.lee@samsung.com> wrote: > > 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. It's not clear to me, under what circumstance you want to optimize for. Is the SD card ever getting properly initialized during boot? Kind regards Uffe > > 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 >
> -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Wednesday, February 14, 2024 8:27 PM > To: Seunghui Lee <sh043.lee@samsung.com> > Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; > gregkh@linuxfoundation.org; avri.altman@wdc.com; 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 Tue, 13 Feb 2024 at 06:13, Seunghui Lee <sh043.lee@samsung.com> wrote: > > > > 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. > > It's not clear to me, under what circumstance you want to optimize for. > > Is the SD card ever getting properly initialized during boot? > > Kind regards > Uffe > We receive a lot of reports about SD card issues in the market. There was no problem with the first time at the time of use, and there are many cases where people recognize that it is not recognized later on. In most cases, this is a problem with the SD card itself. SD card users cannot determine whether or not an SD card is a problem, so they should be guided in this regard. It is necessary to distinguish whether the SD card is inserted but unrecognized or the SD card itself is not inserted, and if there is a field that can check for initialization failure, it will facilitate guidance, so we considered the patch. The variable's usage is expected to be used through the sysfs node in the vendor module. > > > > 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 > >
On Thu, Feb 15, 2024 at 10:03:45AM +0900, 이승희 wrote: > > -----Original Message----- > > From: Ulf Hansson <ulf.hansson@linaro.org> > > Sent: Wednesday, February 14, 2024 8:27 PM > > To: Seunghui Lee <sh043.lee@samsung.com> > > Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; > > gregkh@linuxfoundation.org; avri.altman@wdc.com; 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 Tue, 13 Feb 2024 at 06:13, Seunghui Lee <sh043.lee@samsung.com> wrote: > > > > > > 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. > > > > It's not clear to me, under what circumstance you want to optimize for. > > > > Is the SD card ever getting properly initialized during boot? > > > > Kind regards > > Uffe > > > We receive a lot of reports about SD card issues in the market. > There was no problem with the first time at the time of use, and there are many cases where people recognize that it is not recognized later on. In most cases, this is a problem with the SD card itself. > > SD card users cannot determine whether or not an SD card is a problem, so they should be guided in this regard. > It is necessary to distinguish whether the SD card is inserted but unrecognized or the SD card itself is not inserted, and if there is a field that can check for initialization failure, it will facilitate guidance, so we considered the patch. > > The variable's usage is expected to be used through the sysfs node in the vendor module. What "vendor module"? You need to include all of the needed code here please. thanks, greg k-h
On Thu, 15 Feb 2024 at 02:03, 이승희 <sh043.lee@samsung.com> wrote: > > > -----Original Message----- > > From: Ulf Hansson <ulf.hansson@linaro.org> > > Sent: Wednesday, February 14, 2024 8:27 PM > > To: Seunghui Lee <sh043.lee@samsung.com> > > Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; > > gregkh@linuxfoundation.org; avri.altman@wdc.com; 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 Tue, 13 Feb 2024 at 06:13, Seunghui Lee <sh043.lee@samsung.com> wrote: > > > > > > 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. > > > > It's not clear to me, under what circumstance you want to optimize for. > > > > Is the SD card ever getting properly initialized during boot? > > > > Kind regards > > Uffe > > > We receive a lot of reports about SD card issues in the market. > There was no problem with the first time at the time of use, and there are many cases where people recognize that it is not recognized later on. In most cases, this is a problem with the SD card itself. Right. Thanks for clarifying. A follow up question from me is then, do you know more exactly *why* the SD cards encounter problems? In the past people have told me that powering on/off an SD card during system suspend/resume, too frequently, could damage the card. For non-removable cards, the card stays powered-off even after a system resume, but instead gets powered-on (and re-initialized) when there is a new request for it. In other words, if the problem is related to too frequent powering on/off the card, my advice would be to test with having the card set non-removable (the DT property for this is "non-removable"), to see if that can help. If that solves the problem, we can work towards another common solution instead. > > SD card users cannot determine whether or not an SD card is a problem, so they should be guided in this regard. > It is necessary to distinguish whether the SD card is inserted but unrecognized or the SD card itself is not inserted, and if there is a field that can check for initialization failure, it will facilitate guidance, so we considered the patch. > > The variable's usage is expected to be used through the sysfs node in the vendor module. As Greg said, please provide the complete code. Although, I want to stress already at this point, I don't see a solution with sysfs being the correct thing to do here. The kernel should be able to manage this problem itself. [...] Kind regards Uffe
> -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Thursday, February 15, 2024 5:07 PM > To: 이승희 <sh043.lee@samsung.com> > Cc: 'Ulf Hansson' <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org; > linux-kernel@vger.kernel.org; avri.altman@wdc.com; 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 Thu, Feb 15, 2024 at 10:03:45AM +0900, 이승희 wrote: > > > -----Original Message----- > > > From: Ulf Hansson <ulf.hansson@linaro.org> > > > Sent: Wednesday, February 14, 2024 8:27 PM > > > To: Seunghui Lee <sh043.lee@samsung.com> > > > Cc: linux-mmc@vger.kernel.org; linux-kernel@vger.kernel.org; > > > gregkh@linuxfoundation.org; avri.altman@wdc.com; > > > 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 Tue, 13 Feb 2024 at 06:13, Seunghui Lee <sh043.lee@samsung.com> > wrote: > > > > > > > > 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. > > > > > > It's not clear to me, under what circumstance you want to optimize for. > > > > > > Is the SD card ever getting properly initialized during boot? > > > > > > Kind regards > > > Uffe > > > > > We receive a lot of reports about SD card issues in the market. > > There was no problem with the first time at the time of use, and there > are many cases where people recognize that it is not recognized later on. > In most cases, this is a problem with the SD card itself. > > > > SD card users cannot determine whether or not an SD card is a problem, > so they should be guided in this regard. > > It is necessary to distinguish whether the SD card is inserted but > unrecognized or the SD card itself is not inserted, and if there is a > field that can check for initialization failure, it will facilitate > guidance, so we considered the patch. > > > > The variable's usage is expected to be used through the sysfs node in > the vendor module. > > What "vendor module"? You need to include all of the needed code here > please. > > thanks, > > greg k-h This only purpose of this variable is to identify a faulty card on host side. In the past, we can identify that the card is inserted or not with reading get_cd() function. But now, most mobile devices use hybrid type of SD card tray. If the tray is inserted, the return value of get_cd is the same whatever the SD card is inserted or not. It can help us diagonose the status of a SD card as well. Here is the example of usage. static ssize_t status_show(struct device *dev, struct device_attribute *attr, char *buf) { struct mmc_host *host = dev_get_drvdata(dev); struct mmc_card *card = host->card; char *status = NULL; if (card) status = mmc_card_readonly(card) ? "PERMWP" : "NORMAL"; else status = host->init_failed ? "INITFAIL" : "NOCARD"; return sysfs_emit(buf, "%s\n", status); } As for the sysfs node, it should keep the path of node with or without the SD card. That's why I mention the vendor module. If I need to update the commit's comment, I'll fix it. If someone has the better idea(e.g. code, comment), please suggest it. Thank you for reviewing this. Seunghui Lee.
On Thu, Feb 15, 2024 at 08:15:45PM +0900, 이승희 wrote: > > -----Original Message----- > > From: Greg KH <gregkh@linuxfoundation.org> > > Sent: Thursday, February 15, 2024 5:07 PM > > To: 이승희 <sh043.lee@samsung.com> > > Cc: 'Ulf Hansson' <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org; > > linux-kernel@vger.kernel.org; avri.altman@wdc.com; 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 Does this really belong in the body of the email? You might want a nicer email client :) > > > The variable's usage is expected to be used through the sysfs node in > > the vendor module. > > > > What "vendor module"? You need to include all of the needed code here > > please. > > > > thanks, > > > > greg k-h > > This only purpose of this variable is to identify a faulty card on host side. > > In the past, we can identify that the card is inserted or not with reading get_cd() function. > But now, most mobile devices use hybrid type of SD card tray. > If the tray is inserted, the return value of get_cd is the same whatever the SD card is inserted or not. > It can help us diagonose the status of a SD card as well. > > Here is the example of usage. > > static ssize_t status_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct mmc_host *host = dev_get_drvdata(dev); > struct mmc_card *card = host->card; > char *status = NULL; > > if (card) > status = mmc_card_readonly(card) ? "PERMWP" : "NORMAL"; > else > status = host->init_failed ? "INITFAIL" : "NOCARD"; > > return sysfs_emit(buf, "%s\n", status); > } What will userspace do with this information? And why isn't this part of the patch you submitted? > As for the sysfs node, it should keep the path of node with or without the SD card. > That's why I mention the vendor module. What vendor module? What do you mean by vendor module? You know we can't add code to the kernel that is only used by code that is NOT in our kernel tree. You don't want us to take stuff like that, so why is it being proposed here? confused, greg k-h
> -----Original Message----- > > > On Tue, 13 Feb 2024 at 06:13, Seunghui Lee <sh043.lee@samsung.com> > wrote: > > > > > > > > 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. > > > > > > It's not clear to me, under what circumstance you want to optimize for. > > > > > > Is the SD card ever getting properly initialized during boot? > > > > > > Kind regards > > > Uffe > > > > > We receive a lot of reports about SD card issues in the market. > > There was no problem with the first time at the time of use, and there > are many cases where people recognize that it is not recognized later on. > In most cases, this is a problem with the SD card itself. > > Right. Thanks for clarifying. > > A follow up question from me is then, do you know more exactly *why* the > SD cards encounter problems? > > In the past people have told me that powering on/off an SD card during > system suspend/resume, too frequently, could damage the card. For non- > removable cards, the card stays powered-off even after a system resume, > but instead gets powered-on (and re-initialized) when there is a new > request for it. > > In other words, if the problem is related to too frequent powering on/off > the card, my advice would be to test with having the card set non- > removable (the DT property for this is "non-removable"), to see if that > can help. If that solves the problem, we can work towards another common > solution instead. > I understand your focus on finding the root cause of the problem. However, unlike internal storage, there is a limit to analyzing cards that have problems in the market. This is because there are many different SD card manufacturers and many manufacturers leave it to OEMs. For deferred resume, a responsiveness problem occurs on the user side on mobile devices. The response time of the initializing SD card initialization in the application seems to be slow. Currently, it seems to be a good structure for the first initialization at runtime resume. Regarding non-removable, We will test if we are given an opportunity to further analyze the cards occurring in the market. However, SD card detection is also used as a wakeup source and must be inserted/removed so I'll consider it for testing purposes. Thank you for the good suggestions though. > > > > SD card users cannot determine whether or not an SD card is a problem, > so they should be guided in this regard. > > It is necessary to distinguish whether the SD card is inserted but > unrecognized or the SD card itself is not inserted, and if there is a > field that can check for initialization failure, it will facilitate > guidance, so we considered the patch. > > > > The variable's usage is expected to be used through the sysfs node in > the vendor module. > > As Greg said, please provide the complete code. > > Although, I want to stress already at this point, I don't see a solution > with sysfs being the correct thing to do here. The kernel should be able > to manage this problem itself. > > [...] > > Kind regards > Uffe I understand it and reconsider this commit. Thank you for reviewing this. Seunghui Lee.
> -----Original Message----- > On Thu, Feb 15, 2024 at 08:15:45PM +0900, 이승희 wrote: > > > Subject: Re: [PATCH] mmc: sd: Add a variable to check a faulty > > > device > > Does this really belong in the body of the email? You might want a nicer > email client :) > It was caused by unfamiliarity with upstream. Sorry for the inconvenience. > > > > The variable's usage is expected to be used through the sysfs node > > > > in > > > the vendor module. > > > > > > What "vendor module"? You need to include all of the needed code > > > here please. > > > > > > thanks, > > > > > > greg k-h > > > > This only purpose of this variable is to identify a faulty card on host > side. > > > > In the past, we can identify that the card is inserted or not with > reading get_cd() function. > > But now, most mobile devices use hybrid type of SD card tray. > > If the tray is inserted, the return value of get_cd is the same whatever > the SD card is inserted or not. > > It can help us diagonose the status of a SD card as well. > > > > Here is the example of usage. > > > > static ssize_t status_show(struct device *dev, > > struct device_attribute *attr, char *buf) { > > struct mmc_host *host = dev_get_drvdata(dev); > > struct mmc_card *card = host->card; > > char *status = NULL; > > > > if (card) > > status = mmc_card_readonly(card) ? "PERMWP" : "NORMAL"; > > else > > status = host->init_failed ? "INITFAIL" : "NOCARD"; > > > > return sysfs_emit(buf, "%s\n", status); } > > What will userspace do with this information? > > And why isn't this part of the patch you submitted? > > > As for the sysfs node, it should keep the path of node with or without > the SD card. > > That's why I mention the vendor module. > > What vendor module? What do you mean by vendor module? You know we can't > add code to the kernel that is only used by code that is NOT in our kernel > tree. You don't want us to take stuff like that, so why is it being > proposed here? > > confused, > > greg k-h We need to inform users that there is a problem with the SD card. There is a diagnostic tool in the customer service app, Adding a sysfs node is under consideration so that the tool can diagnose the SD card. To do this, a node capable of diagnosing an SD card is needed regardless of whether an SD card is present or not. Since I understand that the proposed sysfs node is difficult to apply to the kernel, no separate commit was posted. So, I created this PR because I needed a variable to identify the faulty device. I will consider this part again. And if you have any other good ideas, please feel free to suggest them. Thank you for your review. Seunghui Lee.
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 */
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. 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(-)