| Message ID | 20240606131222.1131880-1-linux-mmc@danman.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] mmc: core: allow detection of locked cards | expand |
On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: > > From: Daniel Kucera <linux-mmc@danman.eu> > > Locked SD card will not reply to SEND_SCR or SD_STATUS commands > so it was failing to initialize previously. When skipped, > the card will get initialized and CMD42 can be sent using > ioctl to unlock the card or remove password protection. > For eMMC, this is not necessary because all initialization > commands are allowed in locked state. > Until unlocked, all read/write calls will timeout. Skipping the commands above, only means the card gets partially initialized. Leaving a card in that state seems fragile. What will happen to upper block layers and filesystems when trying to access it? Several years ago Al Cooper made an attempt [1] to manage the unlocking of the card during initialization, by finding the password through the "keys" subsystem. The series didn't really make it to the upstream kernel, but overall the approach seemed to make sense to me. It should allow us to complete the initialization of the card inside the kernel and prevent unnecessary complexity for userspace to manage. Perhaps you can have a closer look at that series and see if it's possible to update it? Kind regards Uffe [1] https://lore.kernel.org/linux-mmc/1433526147-25941-1-git-send-email-alcooperx@gmail.com/ > > Signed-off-by: Daniel Kucera <linux-mmc@danman.eu> > --- > drivers/mmc/core/sd.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c > index 1c8148cdd..ae821df7d 100644 > --- a/drivers/mmc/core/sd.c > +++ b/drivers/mmc/core/sd.c > @@ -928,8 +928,19 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card, > bool reinit) > { > int err; > + u32 card_status; > > - if (!reinit) { > + err = mmc_send_status(card, &card_status); > + if (err){ > + pr_err("%s: unable to get card status\n", mmc_hostname(host)); > + return err; > + } > + > + if (card_status & R1_CARD_IS_LOCKED){ > + pr_warn("%s: card is locked\n", mmc_hostname(host)); > + } > + > + if (!reinit && !(card_status & R1_CARD_IS_LOCKED)) { > /* > * Fetch SCR from card. > */ > -- > 2.34.1 >
On 2024-06-20 14:38, Ulf Hansson wrote: > On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: >> >> From: Daniel Kucera <linux-mmc@danman.eu> >> >> Locked SD card will not reply to SEND_SCR or SD_STATUS commands >> so it was failing to initialize previously. When skipped, >> the card will get initialized and CMD42 can be sent using >> ioctl to unlock the card or remove password protection. >> For eMMC, this is not necessary because all initialization >> commands are allowed in locked state. >> Until unlocked, all read/write calls will timeout. > > Skipping the commands above, only means the card gets partially > initialized. Correct, but it's an improvement in comparison to current state. > Leaving a card in that state seems fragile. Fragile in what sense? Nothing can happen to the card as it is locked. > What will > happen to upper block layers and filesystems when trying to access it? Everything will simply time-out. > > Several years ago Al Cooper made an attempt [1] to manage the > unlocking of the card during initialization, by finding the password > through the "keys" subsystem. The series didn't really make it to the > upstream kernel, but overall the approach seemed to make sense to me. > It should allow us to complete the initialization of the card inside > the kernel and prevent unnecessary complexity for userspace to manage. Yes, he did a great work but obviously no-one got too excited to review/merge his work. His solution was complex. Mine targets the smallest change possible to make it work at least. I wanted to avoid a solution that would be hard to test, review and maintain. Especially when there is only a small interest in the feature. > Perhaps you can have a closer look at that series and see if it's > possible to update it? I don't think I have the skills to revive his work. > > Kind regards > Uffe BR Daniel > > [1] > https://lore.kernel.org/linux-mmc/1433526147-25941-1-git-send-email-alcooperx@gmail.com/ > >> >> Signed-off-by: Daniel Kucera <linux-mmc@danman.eu> >> --- >> drivers/mmc/core/sd.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c >> index 1c8148cdd..ae821df7d 100644 >> --- a/drivers/mmc/core/sd.c >> +++ b/drivers/mmc/core/sd.c >> @@ -928,8 +928,19 @@ int mmc_sd_setup_card(struct mmc_host *host, >> struct mmc_card *card, >> bool reinit) >> { >> int err; >> + u32 card_status; >> >> - if (!reinit) { >> + err = mmc_send_status(card, &card_status); >> + if (err){ >> + pr_err("%s: unable to get card status\n", >> mmc_hostname(host)); >> + return err; >> + } >> + >> + if (card_status & R1_CARD_IS_LOCKED){ >> + pr_warn("%s: card is locked\n", mmc_hostname(host)); >> + } >> + >> + if (!reinit && !(card_status & R1_CARD_IS_LOCKED)) { >> /* >> * Fetch SCR from card. >> */ >> -- >> 2.34.1 >>
On Thu, 20 Jun 2024 at 14:59, Daniel Kucera <linux-mmc@danman.eu> wrote: > > On 2024-06-20 14:38, Ulf Hansson wrote: > > On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: > >> > >> From: Daniel Kucera <linux-mmc@danman.eu> > >> > >> Locked SD card will not reply to SEND_SCR or SD_STATUS commands > >> so it was failing to initialize previously. When skipped, > >> the card will get initialized and CMD42 can be sent using > >> ioctl to unlock the card or remove password protection. > >> For eMMC, this is not necessary because all initialization > >> commands are allowed in locked state. > >> Until unlocked, all read/write calls will timeout. > > > > Skipping the commands above, only means the card gets partially > > initialized. > > Correct, but it's an improvement in comparison to current state. Not sure I agree with that, sorry. > > > Leaving a card in that state seems fragile. > > Fragile in what sense? Nothing can happen to the card as it is locked. We may end up having a card half-way initialized that we can't really communicate with in a stable manner. From a system point of view, I would be worried. I would rather just power off the card if initialization fails and remove its corresponding device from the system. > > > What will > > happen to upper block layers and filesystems when trying to access it? > > Everything will simply time-out. Yes, but it's uncertain what that could lead to? What will happen with power consumption and power management support, for example. > > > > > Several years ago Al Cooper made an attempt [1] to manage the > > unlocking of the card during initialization, by finding the password > > through the "keys" subsystem. The series didn't really make it to the > > upstream kernel, but overall the approach seemed to make sense to me. > > It should allow us to complete the initialization of the card inside > > the kernel and prevent unnecessary complexity for userspace to manage. > > Yes, he did a great work but obviously no-one got too excited to > review/merge > his work. His solution was complex. It's quite some amount of code. On the other hand, it seemed quite straight forward, not that complex to me. > > Mine targets the smallest change possible to make it work at least. > I wanted to avoid a solution that would be hard to test, review and > maintain. > Especially when there is only a small interest in the feature. > > > Perhaps you can have a closer look at that series and see if it's > > possible to update it? > > I don't think I have the skills to revive his work. I see, maybe we should ping Al and other interested folkz to see if there still is some interest to move this forward? [...] Kind regards Uffe
On 6/20/24 15:32, Ulf Hansson wrote: > On Thu, 20 Jun 2024 at 14:59, Daniel Kucera <linux-mmc@danman.eu> wrote: >> >> On 2024-06-20 14:38, Ulf Hansson wrote: >>> On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: >>>> >>>> From: Daniel Kucera <linux-mmc@danman.eu> >>>> >>>> Locked SD card will not reply to SEND_SCR or SD_STATUS commands >>>> so it was failing to initialize previously. When skipped, >>>> the card will get initialized and CMD42 can be sent using >>>> ioctl to unlock the card or remove password protection. >>>> For eMMC, this is not necessary because all initialization >>>> commands are allowed in locked state. >>>> Until unlocked, all read/write calls will timeout. >>> >>> Skipping the commands above, only means the card gets partially >>> initialized. >> >> Correct, but it's an improvement in comparison to current state. > > Not sure I agree with that, sorry. > >> >>> Leaving a card in that state seems fragile. >> >> Fragile in what sense? Nothing can happen to the card as it is locked. > > We may end up having a card half-way initialized that we can't really > communicate with in a stable manner. From a system point of view, I > would be worried. > > I would rather just power off the card if initialization fails and > remove its corresponding device from the system. > >> >>> What will >>> happen to upper block layers and filesystems when trying to access it? >> >> Everything will simply time-out. > > Yes, but it's uncertain what that could lead to? > > What will happen with power consumption and power management support, > for example. Definitely an aspect that needs to be considered, probably even just powering it off after 10ish seconds would be better, then you still get the chance of unlocking it without having a locked card unknowingly consuming power. Having a saved key and sending that to any card being plugged in seems wrong if you consider security, then again if you consider security you should probably somewhere else than the SD/MMC LOCK/UNLOCK ;) Kind Regards, Christian
On 2024-06-20 16:32, Ulf Hansson wrote: > On Thu, 20 Jun 2024 at 14:59, Daniel Kucera <linux-mmc@danman.eu> > wrote: >> >> On 2024-06-20 14:38, Ulf Hansson wrote: >> > On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: >> >> >> >> From: Daniel Kucera <linux-mmc@danman.eu> >> >> >> >> Locked SD card will not reply to SEND_SCR or SD_STATUS commands >> >> so it was failing to initialize previously. When skipped, >> >> the card will get initialized and CMD42 can be sent using >> >> ioctl to unlock the card or remove password protection. >> >> For eMMC, this is not necessary because all initialization >> >> commands are allowed in locked state. >> >> Until unlocked, all read/write calls will timeout. >> > >> > Skipping the commands above, only means the card gets partially >> > initialized. >> >> Correct, but it's an improvement in comparison to current state. > > Not sure I agree with that, sorry. Are you saying that that being able to work with locked card is not an improvement in comparison to not being able to? Or did I misunderstand that? > >> >> > Leaving a card in that state seems fragile. >> >> Fragile in what sense? Nothing can happen to the card as it is locked. > > We may end up having a card half-way initialized that we can't really > communicate with in a stable manner. From a system point of view, I > would be worried. It's not half-way initialized, it's initialized correctly, it's just not using the full potential of the card (higher speeds, etc.). The situation would be the same as it is currently with emmc. Locked emmc gets initialized correctly but reads/writes time-out. What is wrong in having the same result for both sd and emmc? > > I would rather just power off the card if initialization fails and > remove its corresponding device from the system. > >> >> > What will >> > happen to upper block layers and filesystems when trying to access it? >> >> Everything will simply time-out. > > Yes, but it's uncertain what that could lead to? > > What will happen with power consumption and power management support, > for example. Okay, this is a valid concern. Would it be acceptable if we just enabled this "feature" with a module parameter, something like "sd_initialize_locked=1" with default 0? > >> >> > >> > Several years ago Al Cooper made an attempt [1] to manage the >> > unlocking of the card during initialization, by finding the password >> > through the "keys" subsystem. The series didn't really make it to the >> > upstream kernel, but overall the approach seemed to make sense to me. >> > It should allow us to complete the initialization of the card inside >> > the kernel and prevent unnecessary complexity for userspace to manage. >> >> Yes, he did a great work but obviously no-one got too excited to >> review/merge >> his work. His solution was complex. > > It's quite some amount of code. On the other hand, it seemed quite > straight forward, not that complex to me. It doesn't solve the situation when you don't know the password and you just want to erase the card and continue using in. The (un)locking doesn't belong to kernel IMO, if it can be implemented in user-space, it should and it is more flexible that way. > >> >> Mine targets the smallest change possible to make it work at least. >> I wanted to avoid a solution that would be hard to test, review and >> maintain. >> Especially when there is only a small interest in the feature. >> >> > Perhaps you can have a closer look at that series and see if it's >> > possible to update it? >> >> I don't think I have the skills to revive his work. > > I see, maybe we should ping Al and other interested folkz to see if > there still is some interest to move this forward? Okay, Al, are you interested? > > [...] > > Kind regards > Uffe BR Daniel
> On 2024-06-20 16:32, Ulf Hansson wrote: > > On Thu, 20 Jun 2024 at 14:59, Daniel Kucera <linux-mmc@danman.eu> > > wrote: > >> > >> On 2024-06-20 14:38, Ulf Hansson wrote: > >> > On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: > >> >> > >> >> From: Daniel Kucera <linux-mmc@danman.eu> > >> >> > >> >> Locked SD card will not reply to SEND_SCR or SD_STATUS commands so > >> >> it was failing to initialize previously. When skipped, the card > >> >> will get initialized and CMD42 can be sent using ioctl to unlock > >> >> the card or remove password protection. > >> >> For eMMC, this is not necessary because all initialization > >> >> commands are allowed in locked state. > >> >> Until unlocked, all read/write calls will timeout. > >> > > >> > Skipping the commands above, only means the card gets partially > >> > initialized. > >> > >> Correct, but it's an improvement in comparison to current state. > > > > Not sure I agree with that, sorry. > > Are you saying that that being able to work with locked card is not an > improvement in comparison to not being able to? Or did I misunderstand > that? > > > > >> > >> > Leaving a card in that state seems fragile. > >> > >> Fragile in what sense? Nothing can happen to the card as it is locked. > > > > We may end up having a card half-way initialized that we can't really > > communicate with in a stable manner. From a system point of view, I > > would be worried. Actually, it's what the spec expects - the CARD_IS_LOCKED is carried in CMD7 response. Then the card responds to class 0, class 7, and ACMD41. > > It's not half-way initialized, it's initialized correctly, it's just not using the full > potential of the card (higher speeds, etc.). > > The situation would be the same as it is currently with emmc. Locked emmc > gets initialized correctly but reads/writes time-out. What is wrong in having > the same result for both sd and emmc? > > > > > I would rather just power off the card if initialization fails and > > remove its corresponding device from the system. > > > >> > >> > What will > >> > happen to upper block layers and filesystems when trying to access it? > >> > >> Everything will simply time-out. > > > > Yes, but it's uncertain what that could lead to? > > > > What will happen with power consumption and power management > support, > > for example. > > Okay, this is a valid concern. Would it be acceptable if we just enabled this > "feature" with a module parameter, something like "sd_initialize_locked=1" > with default 0? > > > > >> > >> > > >> > Several years ago Al Cooper made an attempt [1] to manage the > >> > unlocking of the card during initialization, by finding the > >> > password through the "keys" subsystem. The series didn't really > >> > make it to the upstream kernel, but overall the approach seemed to make > sense to me. > >> > It should allow us to complete the initialization of the card > >> > inside the kernel and prevent unnecessary complexity for userspace to > manage. > >> > >> Yes, he did a great work but obviously no-one got too excited to > >> review/merge his work. His solution was complex. > > > > It's quite some amount of code. On the other hand, it seemed quite > > straight forward, not that complex to me. > > It doesn't solve the situation when you don't know the password and you just > want to erase the card and continue using in. The (un)locking doesn't belong > to kernel IMO, if it can be implemented in user-space, it should and it is more > flexible that way. I also see little value in handling the unlocking process in the kernel. I find the proposed approach, e.g. co-exists with its sibling mmc-utils patch, straight forward and simpler. Thanks, Avri > > > > >> > >> Mine targets the smallest change possible to make it work at least. > >> I wanted to avoid a solution that would be hard to test, review and > >> maintain. > >> Especially when there is only a small interest in the feature. > >> > >> > Perhaps you can have a closer look at that series and see if it's > >> > possible to update it? > >> > >> I don't think I have the skills to revive his work. > > > > I see, maybe we should ping Al and other interested folkz to see if > > there still is some interest to move this forward? > > Okay, Al, are you interested? > > > > > [...] > > > > Kind regards > > Uffe > > BR > Daniel >
Hi Ulf, could you please reconsider this? Or give some tips how to make this acceptable for merging? Thank you, Daniel. On 2024-06-21 09:16, Avri Altman wrote: >> On 2024-06-20 16:32, Ulf Hansson wrote: >> > On Thu, 20 Jun 2024 at 14:59, Daniel Kucera <linux-mmc@danman.eu> >> > wrote: >> >> >> >> On 2024-06-20 14:38, Ulf Hansson wrote: >> >> > On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: >> >> >> >> >> >> From: Daniel Kucera <linux-mmc@danman.eu> >> >> >> >> >> >> Locked SD card will not reply to SEND_SCR or SD_STATUS commands so >> >> >> it was failing to initialize previously. When skipped, the card >> >> >> will get initialized and CMD42 can be sent using ioctl to unlock >> >> >> the card or remove password protection. >> >> >> For eMMC, this is not necessary because all initialization >> >> >> commands are allowed in locked state. >> >> >> Until unlocked, all read/write calls will timeout. >> >> > >> >> > Skipping the commands above, only means the card gets partially >> >> > initialized. >> >> >> >> Correct, but it's an improvement in comparison to current state. >> > >> > Not sure I agree with that, sorry. >> >> Are you saying that that being able to work with locked card is not an >> improvement in comparison to not being able to? Or did I misunderstand >> that? >> >> > >> >> >> >> > Leaving a card in that state seems fragile. >> >> >> >> Fragile in what sense? Nothing can happen to the card as it is locked. >> > >> > We may end up having a card half-way initialized that we can't really >> > communicate with in a stable manner. From a system point of view, I >> > would be worried. > Actually, it's what the spec expects - the CARD_IS_LOCKED is carried > in CMD7 response. > Then the card responds to class 0, class 7, and ACMD41. > >> >> It's not half-way initialized, it's initialized correctly, it's just >> not using the full >> potential of the card (higher speeds, etc.). >> >> The situation would be the same as it is currently with emmc. Locked >> emmc >> gets initialized correctly but reads/writes time-out. What is wrong in >> having >> the same result for both sd and emmc? >> >> > >> > I would rather just power off the card if initialization fails and >> > remove its corresponding device from the system. >> > >> >> >> >> > What will >> >> > happen to upper block layers and filesystems when trying to access it? >> >> >> >> Everything will simply time-out. >> > >> > Yes, but it's uncertain what that could lead to? >> > >> > What will happen with power consumption and power management >> support, >> > for example. >> >> Okay, this is a valid concern. Would it be acceptable if we just >> enabled this >> "feature" with a module parameter, something like >> "sd_initialize_locked=1" >> with default 0? >> >> > >> >> >> >> > >> >> > Several years ago Al Cooper made an attempt [1] to manage the >> >> > unlocking of the card during initialization, by finding the >> >> > password through the "keys" subsystem. The series didn't really >> >> > make it to the upstream kernel, but overall the approach seemed to make >> sense to me. >> >> > It should allow us to complete the initialization of the card >> >> > inside the kernel and prevent unnecessary complexity for userspace to >> manage. >> >> >> >> Yes, he did a great work but obviously no-one got too excited to >> >> review/merge his work. His solution was complex. >> > >> > It's quite some amount of code. On the other hand, it seemed quite >> > straight forward, not that complex to me. >> >> It doesn't solve the situation when you don't know the password and >> you just >> want to erase the card and continue using in. The (un)locking doesn't >> belong >> to kernel IMO, if it can be implemented in user-space, it should and >> it is more >> flexible that way. > I also see little value in handling the unlocking process in the > kernel. > I find the proposed approach, e.g. co-exists with its sibling > mmc-utils patch, straight forward and simpler. > > Thanks, > Avri >> >> > >> >> >> >> Mine targets the smallest change possible to make it work at least. >> >> I wanted to avoid a solution that would be hard to test, review and >> >> maintain. >> >> Especially when there is only a small interest in the feature. >> >> >> >> > Perhaps you can have a closer look at that series and see if it's >> >> > possible to update it? >> >> >> >> I don't think I have the skills to revive his work. >> > >> > I see, maybe we should ping Al and other interested folkz to see if >> > there still is some interest to move this forward? >> >> Okay, Al, are you interested? >> >> > >> > [...] >> > >> > Kind regards >> > Uffe >> >> BR >> Daniel >>
On Thu, 20 Jun 2024 at 20:15, Daniel Kucera <linux-mmc@danman.eu> wrote: > > On 2024-06-20 16:32, Ulf Hansson wrote: > > On Thu, 20 Jun 2024 at 14:59, Daniel Kucera <linux-mmc@danman.eu> > > wrote: > >> > >> On 2024-06-20 14:38, Ulf Hansson wrote: > >> > On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: > >> >> > >> >> From: Daniel Kucera <linux-mmc@danman.eu> > >> >> > >> >> Locked SD card will not reply to SEND_SCR or SD_STATUS commands > >> >> so it was failing to initialize previously. When skipped, > >> >> the card will get initialized and CMD42 can be sent using > >> >> ioctl to unlock the card or remove password protection. > >> >> For eMMC, this is not necessary because all initialization > >> >> commands are allowed in locked state. > >> >> Until unlocked, all read/write calls will timeout. > >> > > >> > Skipping the commands above, only means the card gets partially > >> > initialized. > >> > >> Correct, but it's an improvement in comparison to current state. > > > > Not sure I agree with that, sorry. > > Are you saying that that being able to work with locked card is not an > improvement in comparison to not being able to? Or did I misunderstand > that? > > > > >> > >> > Leaving a card in that state seems fragile. > >> > >> Fragile in what sense? Nothing can happen to the card as it is locked. > > > > We may end up having a card half-way initialized that we can't really > > communicate with in a stable manner. From a system point of view, I > > would be worried. > > It's not half-way initialized, it's initialized correctly, it's just not > using the full potential of the card (higher speeds, etc.). > > The situation would be the same as it is currently with emmc. Locked > emmc gets initialized correctly but reads/writes time-out. What is wrong > in having the same result for both sd and emmc? Very good point! > > > > > I would rather just power off the card if initialization fails and > > remove its corresponding device from the system. > > > >> > >> > What will > >> > happen to upper block layers and filesystems when trying to access it? > >> > >> Everything will simply time-out. > > > > Yes, but it's uncertain what that could lead to? > > > > What will happen with power consumption and power management support, > > for example. > > Okay, this is a valid concern. Would it be acceptable if we just enabled > this "feature" with a module parameter, something like > "sd_initialize_locked=1" with default 0? That would be an option, but I don't quite like that either. I guess using locked cards isn't that terribly common. So, perhaps we should simply leave this to be managed entirely by userspace, for now. As pointed out by Avri too. We should still be able to power on/off the card during suspend/resume, which I think is the most important part here. [...] Let me get back to reviewing the actual code for $subject patch, to see how we can move this forward then. Kind regards Uffe
On Thu, 6 Jun 2024 at 15:12, <linux-mmc@danman.eu> wrote: > > From: Daniel Kucera <linux-mmc@danman.eu> > > Locked SD card will not reply to SEND_SCR or SD_STATUS commands > so it was failing to initialize previously. When skipped, > the card will get initialized and CMD42 can be sent using > ioctl to unlock the card or remove password protection. > For eMMC, this is not necessary because all initialization > commands are allowed in locked state. > Until unlocked, all read/write calls will timeout. > > Signed-off-by: Daniel Kucera <linux-mmc@danman.eu> > --- > drivers/mmc/core/sd.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c > index 1c8148cdd..ae821df7d 100644 > --- a/drivers/mmc/core/sd.c > +++ b/drivers/mmc/core/sd.c > @@ -928,8 +928,19 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card, > bool reinit) > { > int err; > + u32 card_status; > > - if (!reinit) { > + err = mmc_send_status(card, &card_status); > + if (err){ > + pr_err("%s: unable to get card status\n", mmc_hostname(host)); > + return err; > + } > + > + if (card_status & R1_CARD_IS_LOCKED){ > + pr_warn("%s: card is locked\n", mmc_hostname(host)); > + } If I understand correctly, there is no point in sending the CMD13 above, unless this is the first attempt to initialize the card. Therefore, it's better to move the whole part above, inside the below if-clause too, otherwise we would end up sending a CMD13 in cases when it's not needed. > + > + if (!reinit && !(card_status & R1_CARD_IS_LOCKED)) { > /* > * Fetch SCR from card. > */ > -- > 2.34.1 > Kind regards Uffe
> If I understand correctly, there is no point in sending the CMD13 above, unless > this is the first attempt to initialize the card. > Therefore, it's better to move the whole part above, inside the below if-clause > too, otherwise we would end up sending a CMD13 in cases when it's not > needed. R1_CARD_IS_LOCKED is CMD13 response, but already in CMD7 response as well, So theoretically you want to skip mmc_sd_setup_card altogether. ACMD6, CMD6, and CMD19 should wait for CMD42 to unlock the card. Thanks, Avri > > > + > > + if (!reinit && !(card_status & R1_CARD_IS_LOCKED)) { > > /* > > * Fetch SCR from card. > > */ > > -- > > 2.34.1 > > > > Kind regards > Uffe
On 2024-07-09 22:06, Avri Altman wrote: >> If I understand correctly, there is no point in sending the CMD13 >> above, unless >> this is the first attempt to initialize the card. >> Therefore, it's better to move the whole part above, inside the below >> if-clause >> too, otherwise we would end up sending a CMD13 in cases when it's not >> needed. > R1_CARD_IS_LOCKED is CMD13 response, but already in CMD7 response as > well, > So theoretically you want to skip mmc_sd_setup_card altogether. Do you mean to modify: mmc_select_card(struct mmc_card *card) to somehow return or save the R1 response to card struct? Because currently, it is not available. Thank you, Daniel. > ACMD6, CMD6, and CMD19 should wait for CMD42 to unlock the card. > > Thanks, > Avri > >> >> > + >> > + if (!reinit && !(card_status & R1_CARD_IS_LOCKED)) { >> > /* >> > * Fetch SCR from card. >> > */ >> > -- >> > 2.34.1 >> > >> >> Kind regards >> Uffe
> On 2024-07-09 22:06, Avri Altman wrote: > >> If I understand correctly, there is no point in sending the CMD13 > >> above, unless this is the first attempt to initialize the card. > >> Therefore, it's better to move the whole part above, inside the below > >> if-clause too, otherwise we would end up sending a CMD13 in cases > >> when it's not needed. > > R1_CARD_IS_LOCKED is CMD13 response, but already in CMD7 response as > > well, So theoretically you want to skip mmc_sd_setup_card altogether. > > Do you mean to modify: > mmc_select_card(struct mmc_card *card) > to somehow return or save the R1 response to card struct? > > Because currently, it is not available. Its just a suggestion - CMD13 does carry this bit as well. Either way, the card will not respond to ACMD6, CMD6, and CMD19 while waiting for CMD42 to unlock the card, Hence failing those should not eventually cause mmc_remove_card(). Thanks, Avri > > Thank you, > Daniel. > > > ACMD6, CMD6, and CMD19 should wait for CMD42 to unlock the card. > > > > Thanks, > > Avri > > > >> > >> > + > >> > + if (!reinit && !(card_status & R1_CARD_IS_LOCKED)) { > >> > /* > >> > * Fetch SCR from card. > >> > */ > >> > -- > >> > 2.34.1 > >> > > >> > >> Kind regards > >> Uffe
On Wed, 10 Jul 2024 at 07:21, Daniel Kucera <linux-mmc@danman.eu> wrote: > > On 2024-07-09 22:06, Avri Altman wrote: > >> If I understand correctly, there is no point in sending the CMD13 > >> above, unless > >> this is the first attempt to initialize the card. > >> Therefore, it's better to move the whole part above, inside the below > >> if-clause > >> too, otherwise we would end up sending a CMD13 in cases when it's not > >> needed. > > R1_CARD_IS_LOCKED is CMD13 response, but already in CMD7 response as > > well, > > So theoretically you want to skip mmc_sd_setup_card altogether. > > Do you mean to modify: > mmc_select_card(struct mmc_card *card) > to somehow return or save the R1 response to card struct? I quite like this, as it avoids us from sending an unnecessary command during initialization. A suggestion is to let _mmc_select_card() take an additional out-parameter to provide the card's status. Then we can let mmc_select_card() parse the status - and if it finds that the card is locked, it can set a new state in card->state (similar to how we use MMC_STATE_BLOCKADDR, for example). In another future step, we may also want to keep track of whether a locked card becomes unlocked. Using the card->state should work fine for that too, I think. [...] Kind regards Uffe
On 2024-07-10 15:26, Ulf Hansson wrote: > On Wed, 10 Jul 2024 at 07:21, Daniel Kucera <linux-mmc@danman.eu> > wrote: >> >> On 2024-07-09 22:06, Avri Altman wrote: >> >> If I understand correctly, there is no point in sending the CMD13 >> >> above, unless >> >> this is the first attempt to initialize the card. >> >> Therefore, it's better to move the whole part above, inside the below >> >> if-clause >> >> too, otherwise we would end up sending a CMD13 in cases when it's not >> >> needed. >> > R1_CARD_IS_LOCKED is CMD13 response, but already in CMD7 response as >> > well, >> > So theoretically you want to skip mmc_sd_setup_card altogether. >> >> Do you mean to modify: >> mmc_select_card(struct mmc_card *card) >> to somehow return or save the R1 response to card struct? > > I quite like this, as it avoids us from sending an unnecessary command > during initialization. Okay, but what if the host is SPI? if (!mmc_host_is_spi(host)) { err = mmc_select_card(card); if (err) goto free_card; } err = mmc_sd_setup_card(host, card, oldcard != NULL); > > A suggestion is to let _mmc_select_card() take an additional > out-parameter to provide the card's status. Then we can let > mmc_select_card() parse the status - and if it finds that the card is > locked, it can set a new state in card->state (similar to how we use > MMC_STATE_BLOCKADDR, for example). > > In another future step, we may also want to keep track of whether a > locked card becomes unlocked. Using the card->state should work fine > for that too, I think. > > [...] > > Kind regards > Uffe
> > On 2024-07-10 15:26, Ulf Hansson wrote: > > On Wed, 10 Jul 2024 at 07:21, Daniel Kucera <linux-mmc@danman.eu> > > wrote: > >> > >> On 2024-07-09 22:06, Avri Altman wrote: > >> >> If I understand correctly, there is no point in sending the CMD13 > >> >> above, unless this is the first attempt to initialize the card. > >> >> Therefore, it's better to move the whole part above, inside the > >> >> below if-clause too, otherwise we would end up sending a CMD13 in > >> >> cases when it's not needed. > >> > R1_CARD_IS_LOCKED is CMD13 response, but already in CMD7 response > >> > as well, So theoretically you want to skip mmc_sd_setup_card > >> > altogether. > >> > >> Do you mean to modify: > >> mmc_select_card(struct mmc_card *card) to somehow return or save the > >> R1 response to card struct? > > > > I quite like this, as it avoids us from sending an unnecessary command > > during initialization. > > Okay, but what if the host is SPI? Looking in the spec, In SPI mode, the card moves to transfer state post ACMD41+CMD58. On the other hand, it does support lock/unlock functionality - identical to SD mode, And the R1_CARD_IS_LOCKED should be obtained from the results of CMD13 - Just like you proposed. So, looks like your original proposal is more inclusive, but let me ask around and get back to you. Thanks, Avri > > if (!mmc_host_is_spi(host)) { > err = mmc_select_card(card); > if (err) > goto free_card; > } > > err = mmc_sd_setup_card(host, card, oldcard != NULL); > > > > > A suggestion is to let _mmc_select_card() take an additional > > out-parameter to provide the card's status. Then we can let > > mmc_select_card() parse the status - and if it finds that the card is > > locked, it can set a new state in card->state (similar to how we use > > MMC_STATE_BLOCKADDR, for example). > > > > In another future step, we may also want to keep track of whether a > > locked card becomes unlocked. Using the card->state should work fine > > for that too, I think. > > > > [...] > > > > Kind regards > > Uffe
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c index 1c8148cdd..ae821df7d 100644 --- a/drivers/mmc/core/sd.c +++ b/drivers/mmc/core/sd.c @@ -928,8 +928,19 @@ int mmc_sd_setup_card(struct mmc_host *host, struct mmc_card *card, bool reinit) { int err; + u32 card_status; - if (!reinit) { + err = mmc_send_status(card, &card_status); + if (err){ + pr_err("%s: unable to get card status\n", mmc_hostname(host)); + return err; + } + + if (card_status & R1_CARD_IS_LOCKED){ + pr_warn("%s: card is locked\n", mmc_hostname(host)); + } + + if (!reinit && !(card_status & R1_CARD_IS_LOCKED)) { /* * Fetch SCR from card. */