| Message ID | 1527229083-217091-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25 May 2018 at 08:18, Shawn Lin <shawn.lin@rock-chips.com> wrote: > mmc_do_erase() gives up checking for bit 17 and bit 18 in device > status, as the spec asks hosts to ignore these two bits according > to JESD84-B51 spec, section 6.13, table 68(Device status). Also, > erase need the device in unlocked status, so we add checking for > bit 25. > > The most important part is checking for bit 15, WP_ERASE_SKIP. The > spec says "Only partial address space was erased due to existing write > protected blocks.", which obviously means we should fail this I/O. > Otherwise, the partial erased data stored in nonvalatile flash violates > the data integrity from the view of I/O owner, which probably confuse > it when further used. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/mmc/core/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 42cfcb6..2cb8450 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -2077,7 +2077,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from, > cmd.flags = MMC_RSP_R1 | MMC_CMD_AC; > /* Do not retry else we can't see errors */ > err = mmc_wait_for_cmd(card->host, &cmd, 0); > - if (err || (cmd.resp[0] & 0xFDF92000)) { > + if (err || (cmd.resp[0] & 0xFFF9A000)) { Well, changing from one magic to another doesn't improve the readability here. Could you possibly invent a "define" for the bitmask instead and then state at the definition why and what bit we care about? > pr_err("error %d requesting status %#x\n", > err, cmd.resp[0]); > err = -EIO; > -- Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018/5/25 16:54, Ulf Hansson wrote: > On 25 May 2018 at 08:18, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> mmc_do_erase() gives up checking for bit 17 and bit 18 in device >> status, as the spec asks hosts to ignore these two bits according >> to JESD84-B51 spec, section 6.13, table 68(Device status). Also, >> erase need the device in unlocked status, so we add checking for >> bit 25. >> >> The most important part is checking for bit 15, WP_ERASE_SKIP. The >> spec says "Only partial address space was erased due to existing write >> protected blocks.", which obviously means we should fail this I/O. >> Otherwise, the partial erased data stored in nonvalatile flash violates >> the data integrity from the view of I/O owner, which probably confuse >> it when further used. >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> drivers/mmc/core/core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index 42cfcb6..2cb8450 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -2077,7 +2077,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from, >> cmd.flags = MMC_RSP_R1 | MMC_CMD_AC; >> /* Do not retry else we can't see errors */ >> err = mmc_wait_for_cmd(card->host, &cmd, 0); >> - if (err || (cmd.resp[0] & 0xFDF92000)) { >> + if (err || (cmd.resp[0] & 0xFFF9A000)) { > > Well, changing from one magic to another doesn't improve the readability here. > > Could you possibly invent a "define" for the bitmask instead and then > state at the definition why and what bit we care about? Finally it's gone with using R1_STATUS(x). I just try to make every step easy to review. For sure, I will adjust the patch sequence and make it look better. > >> pr_err("error %d requesting status %#x\n", >> err, cmd.resp[0]); >> err = -EIO; >> -- > > Kind regards > Uffe > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 42cfcb6..2cb8450 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2077,7 +2077,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from, cmd.flags = MMC_RSP_R1 | MMC_CMD_AC; /* Do not retry else we can't see errors */ err = mmc_wait_for_cmd(card->host, &cmd, 0); - if (err || (cmd.resp[0] & 0xFDF92000)) { + if (err || (cmd.resp[0] & 0xFFF9A000)) { pr_err("error %d requesting status %#x\n", err, cmd.resp[0]); err = -EIO;
mmc_do_erase() gives up checking for bit 17 and bit 18 in device status, as the spec asks hosts to ignore these two bits according to JESD84-B51 spec, section 6.13, table 68(Device status). Also, erase need the device in unlocked status, so we add checking for bit 25. The most important part is checking for bit 15, WP_ERASE_SKIP. The spec says "Only partial address space was erased due to existing write protected blocks.", which obviously means we should fail this I/O. Otherwise, the partial erased data stored in nonvalatile flash violates the data integrity from the view of I/O owner, which probably confuse it when further used. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/mmc/core/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)