* [PATCH] mmc: core: sd: check card write-protect lock while resuming @ 2014-08-18 10:00 Barry Song 2014-08-18 11:57 ` Ulf Hansson 0 siblings, 1 reply; 6+ messages in thread From: Barry Song @ 2014-08-18 10:00 UTC (permalink / raw) To: chris, ulf.hansson Cc: Barry Song, linux-mmc, workgroup.linux, linux-arm-kernel, Minda Chen From: Minda Chen <Minda.Chen@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@csr.com> Signed-off-by: Barry Song <Baohua.Song@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; + } card = oldcard; } else { /* -- 2.0.4 Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook, www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at www.twitter.com/CSR_plc. New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: core: sd: check card write-protect lock while resuming 2014-08-18 10:00 [PATCH] mmc: core: sd: check card write-protect lock while resuming Barry Song @ 2014-08-18 11:57 ` Ulf Hansson 2014-08-22 6:55 ` Barry Song 0 siblings, 1 reply; 6+ messages in thread From: Ulf Hansson @ 2014-08-18 11:57 UTC (permalink / raw) To: Barry Song Cc: Chris Ball, linux-mmc, linux-arm-kernel@lists.infradead.org, workgroup.linux, Minda Chen, Barry Song On 18 August 2014 12:00, Barry Song <Barry.Song@csr.com> wrote: > From: Minda Chen <Minda.Chen@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@csr.com> > Signed-off-by: Barry Song <Baohua.Song@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. Kind regards Uffe > card = oldcard; > } else { > /* > -- > 2.0.4 > > > > Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom > More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook, www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at www.twitter.com/CSR_plc. > New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com. ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] mmc: core: sd: check card write-protect lock while resuming 2014-08-18 11:57 ` Ulf Hansson @ 2014-08-22 6:55 ` Barry Song 2014-08-22 10:00 ` Ulf Hansson 0 siblings, 1 reply; 6+ messages in thread From: Barry Song @ 2014-08-22 6:55 UTC (permalink / raw) To: Ulf Hansson Cc: Chris Ball, linux-mmc, linux-arm-kernel@lists.infradead.org, Minda Chen, DL-SHA-WorkGroupLinux > -----Original Message----- > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Monday, August 18, 2014 7:58 PM > To: Barry Song > Cc: Chris Ball; linux-mmc; linux-arm-kernel@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@csr.com> wrote: > > From: Minda Chen <Minda.Chen@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@csr.com> > > Signed-off-by: Barry Song <Baohua.Song@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)); + + 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; } return 0; > > Kind regards > Uffe > > > card = oldcard; > > } else { > > /* -barry Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook, www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at www.twitter.com/CSR_plc. New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: core: sd: check card write-protect lock while resuming 2014-08-22 6:55 ` Barry Song @ 2014-08-22 10:00 ` Ulf Hansson [not found] ` <7960C73727803F45B9F4D2B2441DA8AC0CE80220@SHAASIEXM01.ASIA.ROOT.PRI> 0 siblings, 1 reply; 6+ messages in thread From: Ulf Hansson @ 2014-08-22 10:00 UTC (permalink / raw) To: Barry Song Cc: Chris Ball, linux-mmc, linux-arm-kernel@lists.infradead.org, Minda Chen, DL-SHA-WorkGroupLinux On 22 August 2014 08:55, Barry Song <Barry.Song@csr.com> wrote: >> -----Original Message----- >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> Sent: Monday, August 18, 2014 7:58 PM >> To: Barry Song >> Cc: Chris Ball; linux-mmc; linux-arm-kernel@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@csr.com> wrote: >> > From: Minda Chen <Minda.Chen@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@csr.com> >> > Signed-off-by: Barry Song <Baohua.Song@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <7960C73727803F45B9F4D2B2441DA8AC0CE80220@SHAASIEXM01.ASIA.ROOT.PRI>]
* Re: FW: [PATCH] mmc: core: sd: check card write-protect lock while resuming [not found] ` <7960C73727803F45B9F4D2B2441DA8AC0CE80220@SHAASIEXM01.ASIA.ROOT.PRI> @ 2014-08-28 2:02 ` minda chen 2014-08-28 6:25 ` Ulf Hansson 0 siblings, 1 reply; 6+ messages in thread From: minda chen @ 2014-08-28 2:02 UTC (permalink / raw) To: Ulf Hansson, linux-mmc, chris, workgroup.linux 2014-08-26 13:01 GMT+08:00 Minda Chen <Minda.Chen@csr.com>: > > > -----Original Message----- > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Friday, August 22, 2014 6:00 PM > To: Barry Song > Cc: Chris Ball; linux-mmc; linux-arm-kernel@lists.infradead.org; Minda Chen; DL-SHA-WorkGroupLinux > Subject: Re: [PATCH] mmc: core: sd: check card write-protect lock while resuming > > On 22 August 2014 08:55, Barry Song <Barry.Song@csr.com> wrote: >>> -----Original Message----- >>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >>> Sent: Monday, August 18, 2014 7:58 PM >>> To: Barry Song >>> Cc: Chris Ball; linux-mmc; linux-arm-kernel@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@csr.com> wrote: >>> > From: Minda Chen <Minda.Chen@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@csr.com> >>> > Signed-off-by: Barry Song <Baohua.Song@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? > ok >> + >> + 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? Sorry, I didn't check it carefully. > > 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 > > To report this email as spam click https://www.mailcontrol.com/sr/MZbqvYs5QwJvpeaetUwhCQ== . > > > Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom > More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook, www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at www.twitter.com/CSR_plc. > New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com. I have a suggestion. Compared with first patch ,mmc_sd_setup_card function select card command (cmd7) is executed. So In mmc_rescan function the card can not be removed. How about reset the card in this case? diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index 0c44510..7db61dc 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) { /* @@ -858,18 +859,16 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card, return err; } + if (host->ops->get_ro) { + mmc_host_clk_hold(card->host); + ro = host->ops->get_ro(host); + mmc_host_clk_release(card->host); + } + /* * 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 (ro < 0) { pr_warning("%s: host does not " "support reading read-only " @@ -878,6 +877,15 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card, } else if (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) { + mmc_go_idle(host); + return -ENOENT; + } } return 0; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: FW: [PATCH] mmc: core: sd: check card write-protect lock while resuming 2014-08-28 2:02 ` FW: " minda chen @ 2014-08-28 6:25 ` Ulf Hansson 0 siblings, 0 replies; 6+ messages in thread From: Ulf Hansson @ 2014-08-28 6:25 UTC (permalink / raw) To: minda chen; +Cc: linux-mmc, Chris Ball, workgroup.linux On 28 August 2014 04:02, minda chen <mdchen8791@gmail.com> wrote: > 2014-08-26 13:01 GMT+08:00 Minda Chen <Minda.Chen@csr.com>: >> >> >> -----Original Message----- >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> Sent: Friday, August 22, 2014 6:00 PM >> To: Barry Song >> Cc: Chris Ball; linux-mmc; linux-arm-kernel@lists.infradead.org; Minda Chen; DL-SHA-WorkGroupLinux >> Subject: Re: [PATCH] mmc: core: sd: check card write-protect lock while resuming >> >> On 22 August 2014 08:55, Barry Song <Barry.Song@csr.com> wrote: >>>> -----Original Message----- >>>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >>>> Sent: Monday, August 18, 2014 7:58 PM >>>> To: Barry Song >>>> Cc: Chris Ball; linux-mmc; linux-arm-kernel@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@csr.com> wrote: >>>> > From: Minda Chen <Minda.Chen@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@csr.com> >>>> > Signed-off-by: Barry Song <Baohua.Song@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? >> > ok >>> + >>> + 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? > Sorry, I didn't check it carefully. > >> >> 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 >> >> To report this email as spam click https://www.mailcontrol.com/sr/MZbqvYs5QwJvpeaetUwhCQ== . >> >> >> Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom >> More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook, www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at www.twitter.com/CSR_plc. >> New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com. > > I have a suggestion. > > Compared with first patch ,mmc_sd_setup_card function select card > command (cmd7) is executed. So In mmc_rescan function the card can not > be removed. How about reset the card in this case? So, in principle that should have the same effect. Maybe we should actually do _both_ what you suggest and set the card state to MMC_CARD_REMOVED, by invoking mmc_card_set_removed(). That will let mmc_rescan remove the card as soon as possible without sending additional cmds and if there shows up any blk requests from the block layer, these will for sure also fail. What do you think? Can you test this carefully? Kind regards Uffe > > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c > index 0c44510..7db61dc 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) { > /* > @@ -858,18 +859,16 @@ int mmc_sd_setup_card(struct mmc_host *host, > struct mmc_card *card, > return err; > } > > + if (host->ops->get_ro) { > + mmc_host_clk_hold(card->host); > + ro = host->ops->get_ro(host); > + mmc_host_clk_release(card->host); > + } > + > /* > * 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 (ro < 0) { > pr_warning("%s: host does not " > "support reading read-only " > @@ -878,6 +877,15 @@ int mmc_sd_setup_card(struct mmc_host *host, > struct mmc_card *card, > } else if (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) { > + mmc_go_idle(host); > + return -ENOENT; > + } > } > > return 0; ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-28 6:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-18 10:00 [PATCH] mmc: core: sd: check card write-protect lock while resuming Barry Song
2014-08-18 11:57 ` Ulf Hansson
2014-08-22 6:55 ` Barry Song
2014-08-22 10:00 ` Ulf Hansson
[not found] ` <7960C73727803F45B9F4D2B2441DA8AC0CE80220@SHAASIEXM01.ASIA.ROOT.PRI>
2014-08-28 2:02 ` FW: " minda chen
2014-08-28 6:25 ` Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox