Hi Avri,
On Thu, 18 Oct 2018 at 18:28, Avri Altman <Avri.Altman@xxxxxxx> wrote:
>
> Hi,
>
> > -----Original Message-----
> > From: Clément Péron <peron.clem@xxxxxxxxx>
> > Sent: Thursday, October 18, 2018 12:32 PM
> > To: Ulf Hansson <ulf.hansson@xxxxxxxxxx>; Avri Altman
> > <Avri.Altman@xxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx
> > Cc: Nikita Maslov <wkernelteam@xxxxxxxxx>; Clément Péron
> > <peron.clem@xxxxxxxxx>
> > Subject: [PATCH v2] mmc-utils: RPMB fails with status 0x0001 on system eMMC
> > chips
> >
> > From: Nikita Maslov <wkernelteam@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> >
> > 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
>
