Message ID | 20181018093225.21477-1-peron.clem@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] mmc-utils: RPMB fails with status 0x0001 on system eMMC chips | expand |
Hi, > -----Original Message----- > From: Clément Péron <peron.clem@gmail.com> > Sent: Thursday, October 18, 2018 12:32 PM > To: Ulf Hansson <ulf.hansson@linaro.org>; Avri Altman > <Avri.Altman@wdc.com>; linux-mmc@vger.kernel.org > Cc: Nikita Maslov <wkernelteam@gmail.com>; Clément Péron > <peron.clem@gmail.com> > Subject: [PATCH v2] mmc-utils: RPMB fails with status 0x0001 on system eMMC > chips > > From: Nikita Maslov <wkernelteam@gmail.com> > > Use MMC_IOC_MULTI_CMD for RPMB access > > On some systems which use MMC as a main storage device > it is possible that RPMB commands are mixed with > generic MMC access commands which invalidates RPMB. > This patch uses MMC_IOC_MULTI_CMD. I was hoping that this patch would be part of a series that rectifies some more of mmc-utils rpmb deficiencies: Allowing multiple frame writes, and adding Authenticated Device Configuration {Read/Write}. But if you don't have time to attend all 3, fixing just this one is fine too. > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- > > Hi, > > this patch was firstly proposed by Nikita, as some modification was requested > and Nikita didn't answer, I propose this new version. > > I didn't add the Nikita signature has it wasn't signed on the first patch and > I don't think I'm allowed to do that. > > I still have some trouble to access the RPMB partition with controller that > have the auto-cmd12 feature. > > So consider this patch as untested, I will be able to perform test on other > controller by the end of the next week. I can help you test the write/write key operations if you don't have a key To your device > > Thanks, > Clement > > mmc.h | 10 +++++++ > mmc_cmds.c | 84 ++++++++++++++++++++++++++---------------------------- > 2 files changed, 50 insertions(+), 44 deletions(-) > > diff --git a/mmc.h b/mmc.h > index 285c1f1..e5d92a0 100644 > --- a/mmc.h > +++ b/mmc.h > @@ -194,3 +194,13 @@ > > #define MMC_RSP_R1 > (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE) > #define MMC_RSP_R1B > (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE|MMC_RS > P_BUSY) > + > +static inline void set_single_cmd(struct mmc_ioc_cmd *ioc, __u32 opcode, > + int write_flag) > +{ > + ioc->opcode = opcode; > + ioc->write_flag = write_flag; > + ioc->arg = 0x0; > + ioc->blksz = 512; > + ioc->flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC; > +} Any particular reason why it is not part of mmc_cmds.c ? > diff --git a/mmc_cmds.c b/mmc_cmds.c > index 44623fe..5544e4e 100644 > --- a/mmc_cmds.c > +++ b/mmc_cmds.c > @@ -1825,6 +1825,8 @@ int do_sanitize(int nargs, char **argv) > ret; > \ > }) > > +#define RMPB_MULTI_CMD_MAX_CMDS 3 > + > enum rpmb_op_type { > MMC_RPMB_WRITE_KEY = 0x01, > MMC_RPMB_READ_CNT = 0x02, > @@ -1864,19 +1866,20 @@ static int do_rpmb_op(int fd, > int err; > u_int16_t rpmb_type; > > - struct mmc_ioc_cmd ioc = { > - .arg = 0x0, > - .blksz = 512, > - .blocks = 1, > - .write_flag = 1, > - .opcode = MMC_WRITE_MULTIPLE_BLOCK, > - .flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | > MMC_CMD_ADTC, > - .data_ptr = (uintptr_t)frame_in > - }; > + struct mmc_ioc_multi_cmd *mioc; If <linux/mmc/ioctl.h> does not includes MMC_IOC_MULTI_CMD, Which is the case sometimes - this will not compile. > + struct mmc_ioc_cmd *ioc; > > if (!frame_in || !frame_out || !out_cnt) > return -EINVAL; > > + /* prepare arguments for MMC_IOC_MUTLI_CMD ioctl */ > + mioc = (struct mmc_ioc_multi_cmd *) > + malloc(sizeof (struct mmc_ioc_multi_cmd) + > + RMPB_MULTI_CMD_MAX_CMDS * sizeof (struct > mmc_ioc_cmd)); > + if (!mioc) { > + return -ENOMEM; > + } > + > rpmb_type = be16toh(frame_in->req_resp); > > switch(rpmb_type) { > @@ -1887,33 +1890,27 @@ static int do_rpmb_op(int fd, > goto out; > } > > + mioc->num_of_cmds = 3; > + > /* Write request */ > - ioc.write_flag |= (1<<31); > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > - if (err < 0) { > - err = -errno; > - goto out; > - } > + ioc = &mioc->cmds[0]; > + set_single_cmd(ioc, MMC_WRITE_MULTIPLE_BLOCK, (1 << 31) > | 1); > + ioc->blocks = 1; Can this be part of set_single_cmd? > + mmc_ioc_cmd_set_data((*ioc), frame_in); > > /* Result request */ > + ioc = &mioc->cmds[1]; > memset(frame_out, 0, sizeof(*frame_out)); > frame_out->req_resp = htobe16(MMC_RPMB_READ_RESP); > - ioc.write_flag = 1; > - ioc.data_ptr = (uintptr_t)frame_out; > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > - if (err < 0) { > - err = -errno; > - goto out; > - } > + set_single_cmd(ioc, MMC_WRITE_MULTIPLE_BLOCK, 1); > + ioc->blocks = 1; > + mmc_ioc_cmd_set_data((*ioc), frame_out); > > /* Get response */ > - ioc.write_flag = 0; > - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > - if (err < 0) { > - err = -errno; > - goto out; > - } > + ioc = &mioc->cmds[2]; > + set_single_cmd(ioc, MMC_READ_MULTIPLE_BLOCK, 0); > + ioc->blocks = 1; > + mmc_ioc_cmd_set_data((*ioc), frame_out); You are using the same frame for both the result request and the response - Expects troubles... > > break; > case MMC_RPMB_READ_CNT: > @@ -1924,23 +1921,19 @@ static int do_rpmb_op(int fd, > /* fall through */ > > case MMC_RPMB_READ: > - /* Request */ > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > - if (err < 0) { > - err = -errno; > - goto out; > - } > + mioc->num_of_cmds = 2; > + > + /* Read request */ > + ioc = &mioc->cmds[0]; > + set_single_cmd(ioc, MMC_WRITE_MULTIPLE_BLOCK, 1); > + ioc->blocks = 1; > + mmc_ioc_cmd_set_data((*ioc), frame_in); > > /* Get response */ > - ioc.write_flag = 0; > - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; > - ioc.blocks = out_cnt; > - ioc.data_ptr = (uintptr_t)frame_out; > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > - if (err < 0) { > - err = -errno; > - goto out; > - } > + ioc = &mioc->cmds[1]; > + set_single_cmd(ioc, MMC_READ_MULTIPLE_BLOCK, 0); > + ioc->blocks = out_cnt; > + mmc_ioc_cmd_set_data((*ioc), frame_out); > > break; > default: > @@ -1948,7 +1941,10 @@ static int do_rpmb_op(int fd, > goto out; > } > > + err = ioctl(fd, MMC_IOC_MULTI_CMD, mioc); > + > out: > + free(mioc); > return err; > } Thanks, Avri > > -- > 2.17.1
Hi Avri, On Thu, 18 Oct 2018 at 18:28, Avri Altman <Avri.Altman@wdc.com> wrote: > > Hi, > > > -----Original Message----- > > From: Clément Péron <peron.clem@gmail.com> > > Sent: Thursday, October 18, 2018 12:32 PM > > To: Ulf Hansson <ulf.hansson@linaro.org>; Avri Altman > > <Avri.Altman@wdc.com>; linux-mmc@vger.kernel.org > > Cc: Nikita Maslov <wkernelteam@gmail.com>; Clément Péron > > <peron.clem@gmail.com> > > Subject: [PATCH v2] mmc-utils: RPMB fails with status 0x0001 on system eMMC > > chips > > > > From: Nikita Maslov <wkernelteam@gmail.com> > > > > Use MMC_IOC_MULTI_CMD for RPMB access > > > > On some systems which use MMC as a main storage device > > it is possible that RPMB commands are mixed with > > generic MMC access commands which invalidates RPMB. > > This patch uses MMC_IOC_MULTI_CMD. > I was hoping that this patch would be part of a series that rectifies > some more of mmc-utils rpmb deficiencies: Allowing multiple frame writes, > and adding Authenticated Device Configuration {Read/Write}. > But if you don't have time to attend all 3, fixing just this one is fine too. > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com> > > --- > > > > Hi, > > > > this patch was firstly proposed by Nikita, as some modification was requested > > and Nikita didn't answer, I propose this new version. > > > > I didn't add the Nikita signature has it wasn't signed on the first patch and > > I don't think I'm allowed to do that. > > > > I still have some trouble to access the RPMB partition with controller that > > have the auto-cmd12 feature. > > > > So consider this patch as untested, I will be able to perform test on other > > controller by the end of the next week. > I can help you test the write/write key operations if you don't have a key > To your device Yes please, > > > > > Thanks, > > Clement > > > > mmc.h | 10 +++++++ > > mmc_cmds.c | 84 ++++++++++++++++++++++++++---------------------------- > > 2 files changed, 50 insertions(+), 44 deletions(-) > > > > diff --git a/mmc.h b/mmc.h > > index 285c1f1..e5d92a0 100644 > > --- a/mmc.h > > +++ b/mmc.h > > @@ -194,3 +194,13 @@ > > > > #define MMC_RSP_R1 > > (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE) > > #define MMC_RSP_R1B > > (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE|MMC_RS > > P_BUSY) > > + > > +static inline void set_single_cmd(struct mmc_ioc_cmd *ioc, __u32 opcode, > > + int write_flag) > > +{ > > + ioc->opcode = opcode; > > + ioc->write_flag = write_flag; > > + ioc->arg = 0x0; > > + ioc->blksz = 512; > > + ioc->flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC; > > +} > Any particular reason why it is not part of mmc_cmds.c ? No, I will move it. > > > diff --git a/mmc_cmds.c b/mmc_cmds.c > > index 44623fe..5544e4e 100644 > > --- a/mmc_cmds.c > > +++ b/mmc_cmds.c > > @@ -1825,6 +1825,8 @@ int do_sanitize(int nargs, char **argv) > > ret; > > \ > > }) > > > > +#define RMPB_MULTI_CMD_MAX_CMDS 3 > > + > > enum rpmb_op_type { > > MMC_RPMB_WRITE_KEY = 0x01, > > MMC_RPMB_READ_CNT = 0x02, > > @@ -1864,19 +1866,20 @@ static int do_rpmb_op(int fd, > > int err; > > u_int16_t rpmb_type; > > > > - struct mmc_ioc_cmd ioc = { > > - .arg = 0x0, > > - .blksz = 512, > > - .blocks = 1, > > - .write_flag = 1, > > - .opcode = MMC_WRITE_MULTIPLE_BLOCK, > > - .flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | > > MMC_CMD_ADTC, > > - .data_ptr = (uintptr_t)frame_in > > - }; > > + struct mmc_ioc_multi_cmd *mioc; > If <linux/mmc/ioctl.h> does not includes MMC_IOC_MULTI_CMD, > Which is the case sometimes - this will not compile. It's included since 4.4, what's your recommandation ? check with #ifdef MMC_IOC_MULTI_CMD and return a -ENOSUPP if the ioctl is not supported ? > > > + struct mmc_ioc_cmd *ioc; > > > > if (!frame_in || !frame_out || !out_cnt) > > return -EINVAL; > > > > + /* prepare arguments for MMC_IOC_MUTLI_CMD ioctl */ > > + mioc = (struct mmc_ioc_multi_cmd *) > > + malloc(sizeof (struct mmc_ioc_multi_cmd) + > > + RMPB_MULTI_CMD_MAX_CMDS * sizeof (struct > > mmc_ioc_cmd)); > > + if (!mioc) { > > + return -ENOMEM; > > + } > > + > > rpmb_type = be16toh(frame_in->req_resp); > > > > switch(rpmb_type) { > > @@ -1887,33 +1890,27 @@ static int do_rpmb_op(int fd, > > goto out; > > } > > > > + mioc->num_of_cmds = 3; > > + > > /* Write request */ > > - ioc.write_flag |= (1<<31); > > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > > - if (err < 0) { > > - err = -errno; > > - goto out; > > - } > > + ioc = &mioc->cmds[0]; > > + set_single_cmd(ioc, MMC_WRITE_MULTIPLE_BLOCK, (1 << 31) > > | 1); > > + ioc->blocks = 1; > Can this be part of set_single_cmd? Yes, will do > > > + mmc_ioc_cmd_set_data((*ioc), frame_in); > > > > /* Result request */ > > + ioc = &mioc->cmds[1]; > > memset(frame_out, 0, sizeof(*frame_out)); > > frame_out->req_resp = htobe16(MMC_RPMB_READ_RESP); > > - ioc.write_flag = 1; > > - ioc.data_ptr = (uintptr_t)frame_out; > > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > > - if (err < 0) { > > - err = -errno; > > - goto out; > > - } > > + set_single_cmd(ioc, MMC_WRITE_MULTIPLE_BLOCK, 1); > > + ioc->blocks = 1; > > + mmc_ioc_cmd_set_data((*ioc), frame_out); > > > > /* Get response */ > > - ioc.write_flag = 0; > > - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; > > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > > - if (err < 0) { > > - err = -errno; > > - goto out; > > - } > > + ioc = &mioc->cmds[2]; > > + set_single_cmd(ioc, MMC_READ_MULTIPLE_BLOCK, 0); > > + ioc->blocks = 1; > > + mmc_ioc_cmd_set_data((*ioc), frame_out); > You are using the same frame for both the result request and the response - > Expects troubles... Will fix, Thanks for the review, Clement > > > > > break; > > case MMC_RPMB_READ_CNT: > > @@ -1924,23 +1921,19 @@ static int do_rpmb_op(int fd, > > /* fall through */ > > > > case MMC_RPMB_READ: > > - /* Request */ > > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > > - if (err < 0) { > > - err = -errno; > > - goto out; > > - } > > + mioc->num_of_cmds = 2; > > + > > + /* Read request */ > > + ioc = &mioc->cmds[0]; > > + set_single_cmd(ioc, MMC_WRITE_MULTIPLE_BLOCK, 1); > > + ioc->blocks = 1; > > + mmc_ioc_cmd_set_data((*ioc), frame_in); > > > > /* Get response */ > > - ioc.write_flag = 0; > > - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; > > - ioc.blocks = out_cnt; > > - ioc.data_ptr = (uintptr_t)frame_out; > > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > > - if (err < 0) { > > - err = -errno; > > - goto out; > > - } > > + ioc = &mioc->cmds[1]; > > + set_single_cmd(ioc, MMC_READ_MULTIPLE_BLOCK, 0); > > + ioc->blocks = out_cnt; > > + mmc_ioc_cmd_set_data((*ioc), frame_out); > > > > break; > > default: > > @@ -1948,7 +1941,10 @@ static int do_rpmb_op(int fd, > > goto out; > > } > > > > + err = ioctl(fd, MMC_IOC_MULTI_CMD, mioc); > > + > > out: > > + free(mioc); > > return err; > > } > > Thanks, > Avri > > > > > > -- > > 2.17.1 >
> > > + struct mmc_ioc_multi_cmd *mioc; > > If <linux/mmc/ioctl.h> does not includes MMC_IOC_MULTI_CMD, > > Which is the case sometimes - this will not compile. > > It's included since 4.4, what's your recommandation ? > check with #ifdef MMC_IOC_MULTI_CMD and return a -ENOSUPP if the ioctl > is not supported ? Maybe use the same convention as in ffu? Thanks, Avri
On Thu, 18 Oct 2018 at 19:03, Avri Altman <Avri.Altman@wdc.com> wrote: > > > > > + struct mmc_ioc_multi_cmd *mioc; > > > If <linux/mmc/ioctl.h> does not includes MMC_IOC_MULTI_CMD, > > > Which is the case sometimes - this will not compile. > > > > It's included since 4.4, what's your recommandation ? > > check with #ifdef MMC_IOC_MULTI_CMD and return a -ENOSUPP if the ioctl > > is not supported ? > Maybe use the same convention as in ffu? OK will do, Thanks, Clement > > Thanks, > Avri
diff --git a/mmc.h b/mmc.h index 285c1f1..e5d92a0 100644 --- a/mmc.h +++ b/mmc.h @@ -194,3 +194,13 @@ #define MMC_RSP_R1 (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE) #define MMC_RSP_R1B (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE|MMC_RSP_BUSY) + +static inline void set_single_cmd(struct mmc_ioc_cmd *ioc, __u32 opcode, + int write_flag) +{ + ioc->opcode = opcode; + ioc->write_flag = write_flag; + ioc->arg = 0x0; + ioc->blksz = 512; + ioc->flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC; +} diff --git a/mmc_cmds.c b/mmc_cmds.c index 44623fe..5544e4e 100644 --- a/mmc_cmds.c +++ b/mmc_cmds.c @@ -1825,6 +1825,8 @@ int do_sanitize(int nargs, char **argv) ret; \ }) +#define RMPB_MULTI_CMD_MAX_CMDS 3 + enum rpmb_op_type { MMC_RPMB_WRITE_KEY = 0x01, MMC_RPMB_READ_CNT = 0x02, @@ -1864,19 +1866,20 @@ static int do_rpmb_op(int fd, int err; u_int16_t rpmb_type; - struct mmc_ioc_cmd ioc = { - .arg = 0x0, - .blksz = 512, - .blocks = 1, - .write_flag = 1, - .opcode = MMC_WRITE_MULTIPLE_BLOCK, - .flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC, - .data_ptr = (uintptr_t)frame_in - }; + struct mmc_ioc_multi_cmd *mioc; + struct mmc_ioc_cmd *ioc; if (!frame_in || !frame_out || !out_cnt) return -EINVAL; + /* prepare arguments for MMC_IOC_MUTLI_CMD ioctl */ + mioc = (struct mmc_ioc_multi_cmd *) + malloc(sizeof (struct mmc_ioc_multi_cmd) + + RMPB_MULTI_CMD_MAX_CMDS * sizeof (struct mmc_ioc_cmd)); + if (!mioc) { + return -ENOMEM; + } + rpmb_type = be16toh(frame_in->req_resp); switch(rpmb_type) { @@ -1887,33 +1890,27 @@ static int do_rpmb_op(int fd, goto out; } + mioc->num_of_cmds = 3; + /* Write request */ - ioc.write_flag |= (1<<31); - err = ioctl(fd, MMC_IOC_CMD, &ioc); - if (err < 0) { - err = -errno; - goto out; - } + ioc = &mioc->cmds[0]; + set_single_cmd(ioc, MMC_WRITE_MULTIPLE_BLOCK, (1 << 31) | 1); + ioc->blocks = 1; + mmc_ioc_cmd_set_data((*ioc), frame_in); /* Result request */ + ioc = &mioc->cmds[1]; memset(frame_out, 0, sizeof(*frame_out)); frame_out->req_resp = htobe16(MMC_RPMB_READ_RESP); - ioc.write_flag = 1; - ioc.data_ptr = (uintptr_t)frame_out; - err = ioctl(fd, MMC_IOC_CMD, &ioc); - if (err < 0) { - err = -errno; - goto out; - } + set_single_cmd(ioc, MMC_WRITE_MULTIPLE_BLOCK, 1); + ioc->blocks = 1; + mmc_ioc_cmd_set_data((*ioc), frame_out); /* Get response */ - ioc.write_flag = 0; - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; - err = ioctl(fd, MMC_IOC_CMD, &ioc); - if (err < 0) { - err = -errno; - goto out; - } + ioc = &mioc->cmds[2]; + set_single_cmd(ioc, MMC_READ_MULTIPLE_BLOCK, 0); + ioc->blocks = 1; + mmc_ioc_cmd_set_data((*ioc), frame_out); break; case MMC_RPMB_READ_CNT: @@ -1924,23 +1921,19 @@ static int do_rpmb_op(int fd, /* fall through */ case MMC_RPMB_READ: - /* Request */ - err = ioctl(fd, MMC_IOC_CMD, &ioc); - if (err < 0) { - err = -errno; - goto out; - } + mioc->num_of_cmds = 2; + + /* Read request */ + ioc = &mioc->cmds[0]; + set_single_cmd(ioc, MMC_WRITE_MULTIPLE_BLOCK, 1); + ioc->blocks = 1; + mmc_ioc_cmd_set_data((*ioc), frame_in); /* Get response */ - ioc.write_flag = 0; - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; - ioc.blocks = out_cnt; - ioc.data_ptr = (uintptr_t)frame_out; - err = ioctl(fd, MMC_IOC_CMD, &ioc); - if (err < 0) { - err = -errno; - goto out; - } + ioc = &mioc->cmds[1]; + set_single_cmd(ioc, MMC_READ_MULTIPLE_BLOCK, 0); + ioc->blocks = out_cnt; + mmc_ioc_cmd_set_data((*ioc), frame_out); break; default: @@ -1948,7 +1941,10 @@ static int do_rpmb_op(int fd, goto out; } + err = ioctl(fd, MMC_IOC_MULTI_CMD, mioc); + out: + free(mioc); return err; }