> > Hi, > Please see some comments below. > Please don't send a v2 yet - allow few days to look into the details of this > feature. Some more comments below. > > > From: Daniel Kucera <linux-mmc@xxxxxxxxx> > > > > Implemented locking/unlocking using CMD42 according to Micron > > Technical Note > > > > original link https://media-www.micron.com/- > > /media/client/global/documents/products/technical-note/sd- > > > cards/tnsd01_enable_sd_lock_unlock_in_linux.pdf?rev=03f03a6bc0f8435faf > > a93a > > 8fc8e88988 > > currently available at https://github.com/danielkucera/esp32- > > sdcard/blob/master/tnsd01_enable_sd_lock_unlock_in_linux.pdf > The above technical note specifically refers to SD, Is this designated for SD > only? > If yes please make note of that. > If not - Please relate in your commit log to the differences, if any, between > eMMC & SD. > > The above technical note contains an implementation for mmc-utils. > Are you the author of that code? > > > > > Signed-off-by: Daniel Kucera <linux-mmc@xxxxxxxxx> > > --- > > mmc.c | 11 +++++ > > mmc.h | 11 +++++ > > mmc_cmds.c | 117 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > mmc_cmds.h | 2 + > > 4 files changed, 141 insertions(+) > > > > diff --git a/mmc.c b/mmc.c > > index bc8f74e..3ff7308 100644 > > --- a/mmc.c > > +++ b/mmc.c > > @@ -250,6 +250,17 @@ static struct Command commands[] = { > > "be 1.", > > NULL > > }, > > + { do_lock_unlock, -3, > > + "cmd42", "<password> " "<parameter> " "<device>\n" > > + "Usage: mmc cmd42 <password> <s|c|l|u|e> <device>\n" > > + "s\tset password\n" > > + "c\tclear password\n" > > + "l\tlock\n" > > + "sl\tset password and lock\n" > > + "u\tunlock\n" > > + "e\tforce erase\n", > > + NULL > > + }, > > { do_softreset, -1, > > "softreset", "<device>\n" > > "Issues a CMD0 softreset, e.g. for testing if hardware reset > > for UHS works", diff --git a/mmc.h b/mmc.h index 6f1bf3e..f8bac22 > > 100644 > > --- a/mmc.h > > +++ b/mmc.h > > @@ -30,6 +30,7 @@ > > #define MMC_SEND_STATUS 13 /* ac [31:16] RCA R1 */ > > #define R1_SWITCH_ERROR (1 << 7) /* sx, c */ > > #define MMC_SWITCH_MODE_WRITE_BYTE 0x03 /* Set target to value > */ > > +#define MMC_SET_BLOCKLEN 16 /* ac [31:0] block len R1 */ > > #define MMC_READ_MULTIPLE_BLOCK 18 /* adtc [31:0] data addr R1 */ > > #define MMC_SET_BLOCK_COUNT 23 /* adtc [31:0] data addr R1 */ > > #define MMC_WRITE_BLOCK 24 /* adtc [31:0] data addr R1 > */ > > @@ -46,6 +47,7 @@ > > [1] Discard Enable > > [0] Identify Write Blocks for > > Erase (or TRIM Enable) > > R1b */ > > +#define MMC_LOCK_UNLOCK 42 /* adtc R1b */ > > #define MMC_GEN_CMD 56 /* adtc [31:1] stuff bits. > > [0]: RD/WR1 R1 */ > > > > @@ -70,6 +72,15 @@ > > #define R1_EXCEPTION_EVENT (1 << 6) /* sr, a */ > > #define R1_APP_CMD (1 << 5) /* sr, c */ > > > > +#define MMC_CMD42_UNLOCK 0x0 /* UNLOCK */ > > +#define MMC_CMD42_SET_PWD 0x1 /* SET_PWD */ > > +#define MMC_CMD42_CLR_PWD 0x2 /* CLR_PWD */ > > +#define MMC_CMD42_LOCK 0x4 /* LOCK */ > > +#define MMC_CMD42_SET_LOCK 0x5 /* SET_PWD & LOCK */ > > +#define MMC_CMD42_ERASE 0x8 /* ERASE */ There are 3 variants of sd cards that support lock/unlock. There is a warning in the spec - see note 2* to table 4-10 in sd spec 9.0: "The host should not issue a force erase command if the permanent write protect is set to 1, Otherwise, the type 1 cards can no longer be used even if the user remembers the password." For that reason, its destructive nature, and the complications of FEP with COP vs. non-COP cards - I think its better to leave FEP out for now. > > +#define MAX_PWD_LENGTH 32 /* max PWDS_LEN: old+new */ > > +#define MMC_BLOCK_SIZE 512 /* data blk size for cmd42 */ > > + > > /* > > * EXT_CSD fields > > */ > > diff --git a/mmc_cmds.c b/mmc_cmds.c > > index 936e0c5..07ba18a 100644 > > --- a/mmc_cmds.c > > +++ b/mmc_cmds.c > > @@ -129,6 +129,123 @@ int send_status(int fd, __u32 *response) > > return ret; > > } > > > > +//lock/unlock feature implementation > C99 style single line comments (//) should not be used. Prefer the block > comment style instead. > See: https://www.kernel.org/doc/html/latest/process/coding- > style.html#commenting > Please change throughout. > > > +int do_lock_unlock(int nargs, char **argv) { > > + int fd, ret = 0; > > + char *device; > > + __u8 data_block[MMC_BLOCK_SIZE] = {0}; > Just empty braces please. > > > + __u8 data_block_onebyte[1] = {0}; > > + int block_size = 0; > > + struct mmc_ioc_cmd idata; > > + int cmd42_para; //parameter of cmd42 > > + char pwd[MAX_PWD_LENGTH+1]; //password > > + int pwd_len; //password length > > + __u32 r1_response; //R1 response token > > + > > + if (nargs != 4) { > > + fprintf(stderr, "Usage: mmc cmd42 <password> > > + <s|c|l|u|e> > > <device>\n"); > > + exit(1); > > + } > > + > > + strcpy(pwd, argv[1]); Maybe strncpy up to MAX_PWD_LENGTH Also, in case of a password change, it makes more sense to capture the old and new as 2 different strings. > > + pwd_len = strlen(pwd); Maybe clarify in the usage, that passwords are not allowed to include '\0' > > + > > + if (!strcmp("s", argv[2])) { > > + cmd42_para = MMC_CMD42_SET_PWD; > > + printf("Set password: password=%s ...\n", pwd); > > + } else if (!strcmp("c", argv[2])) { > > + cmd42_para = MMC_CMD42_CLR_PWD; > > + printf("Clear password: password=%s ...\n", pwd); > > + } else if (!strcmp("l", argv[2])) { > > + cmd42_para = MMC_CMD42_LOCK; > > + printf("Lock the card: password=%s ...\n", pwd); > > + } else if (!strcmp("sl", argv[2])) { > > + cmd42_para = MMC_CMD42_SET_LOCK; > > + printf("Set password and lock the card: password - %s ...\n", pwd); > > + } else if (!strcmp("u", argv[2])) { > > + cmd42_para = MMC_CMD42_UNLOCK; > > + printf("Unlock the card: password=%s ...\n", pwd); > > + } else if (!strcmp("e", argv[2])) { > > + cmd42_para = MMC_CMD42_ERASE; > > + printf("Force erase (Warning: all card data will be > > + erased together with > > PWD!)\n"); > > + } else { > > + printf("Invalid parameter:\n" "s\tset password\n" > > + "c\tclear > > password\n" "l\tlock\n" > > + "sl\tset password and lock\n" "u\tunlock\n" "e\tforce > erase\n"); > > + exit(1); > > + } > > + > > + device = argv[nargs-1]; > > + > > + fd = open(device, O_RDWR); > > + if (fd < 0) { > > + perror("open"); > > + exit(1); > > + } > > + > > + if (cmd42_para == MMC_CMD42_ERASE) > > + block_size = 2; //set blk size to 2-byte for Force > > + Erase @DDR50 > > compatibility > > + else > > + block_size = MMC_BLOCK_SIZE; > > + > > + ret = set_block_len(fd, block_size); //set data block size prior to cmd42 > > + printf("Set to data block length = %d byte(s).\n", > > + block_size); On a failure here you should bail out. But it makes more sense to me that you would pack cmd16 & cmd42 in a multi-ioctl. > > + > > + if (cmd42_para == MMC_CMD42_ERASE) { > > + data_block_onebyte[0] = cmd42_para; > > + } else { > > + data_block[0] = cmd42_para; > > + data_block[1] = pwd_len; > > + memcpy((char *)(data_block+2), pwd, pwd_len); > > + } > > + > > + memset(&idata, 0, sizeof(idata)); Can be initialized with {} Thanks, Avri > > + idata.write_flag = 1; > > + idata.opcode = MMC_LOCK_UNLOCK; > > + idata.arg = 0; //set all 0 for cmd42 arg > > + idata.flags = MMC_RSP_R1 | MMC_CMD_AC | MMC_CMD_ADTC; > > + idata.blksz = block_size; > > + idata.blocks = 1; > > + > > + if (cmd42_para == MMC_CMD42_ERASE) > > + mmc_ioc_cmd_set_data(idata, data_block_onebyte); > > + else > > + mmc_ioc_cmd_set_data(idata, data_block); > > + > > + ret = ioctl(fd, MMC_IOC_CMD, &idata); //Issue CMD42 > > + > > + r1_response = idata.response[0]; > > + printf("cmd42 response: 0x%08x\n", r1_response); > > + if (r1_response & R1_ERROR) { //check CMD42 error > > + printf("cmd42 error! Error code: 0x%08x\n", r1_response & > R1_ERROR); > > + ret = -1; > > + } > > + if (r1_response & R1_LOCK_UNLOCK_FAILED) { //check lock/unlock > error > > + printf("Card lock/unlock fail! Error code: 0x%08x\n", > > + r1_response & R1_LOCK_UNLOCK_FAILED); > > + ret = -1; > > + } > > + > > + close(fd); > > + return ret; > > +} > > + > > +//change data block length > > +int set_block_len(int fd, int blk_len) { > > + int ret = 0; > > + struct mmc_ioc_cmd idata; > > + > > + memset(&idata, 0, sizeof(idata)); > > + idata.opcode = MMC_SET_BLOCKLEN; > > + idata.arg = blk_len; > > + idata.flags = MMC_RSP_R1 | MMC_CMD_AC; > > + > > + ret = ioctl(fd, MMC_IOC_CMD, &idata); > > + > > + return ret; > > +} > > + > > static __u32 get_size_in_blks(int fd) { > > int res; > > diff --git a/mmc_cmds.h b/mmc_cmds.h > > index 5f2bef1..9ee78a2 100644 > > --- a/mmc_cmds.h > > +++ b/mmc_cmds.h > > @@ -50,3 +50,5 @@ int do_general_cmd_read(int nargs, char **argv); > > int do_softreset(int nargs, char **argv); int do_preidle(int nargs, > > char **argv); int do_alt_boot_op(int nargs, char **argv); > > +int do_lock_unlock(int nargs, char **argv); int set_block_len(int fd, > > +int blk_len); > Please make set_block_len() static - as its only caller is do_lock_unlock. > > Thanks, > Avri > > > -- > > 2.34.1 > > >
