Nikita hi, Thank you for fixing this long aching issue. See my comments below. Cheers, Avri > -----Original Message----- > From: linux-mmc-owner@xxxxxxxxxxxxxxx <linux-mmc- > owner@xxxxxxxxxxxxxxx> On Behalf Of Ulf Hansson > Sent: Monday, August 27, 2018 1:10 PM > To: Nikita Maslov <wkernelteam@xxxxxxxxx>; Chris Ball <chris@xxxxxxxxxx> > Cc: linux-mmc@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] mmc-utils: RPMB fails with status 0x0001 on system eMMC > chips > > + Chris > > On 27 August 2018 at 11:27, Nikita Maslov <wkernelteam@xxxxxxxxx> wrote: > > There is a problem with RPMB access on systems which use eMMC as > > default storage. > > > > According to specs, RPMB requires multiple commands to be sent > > right one after another, in another case, if there are some other > > commands between them, RPMB will fail with status 0x0001. This case > > is common for systems which use eMMC as default system storage. > > > > In current version of mmc-utils RPMB operations are performed as > > a sequence of MMC_IOC_CMD ioctl calls. Between these calls eMMC is > > unlocked and may be used by mmc driver, so RPMB session will be broken. > > > > In Linux 4.4+, there is MMC_IOC_MULTI_CMD ioctl which allows to perform > > multiple IOC_CMDs in one request. This is a better way to work with > > RPMB, because eMMC is supposed to be locked during ioctl. > > > > I included the patch which tries to use MMC_IOC_MULTI_CMD for RPMB > > operations. If it fails with EINVAL, it tries to use MMC_IOC_CMD > > (for compatibility). No need, prior to removing the host lock, MMC_IOC_MULTI_CMD was supported as well, holding that lock. > > > > This patch was tested on ARM systems (armel) with eMMC chips from > > Micron and SanDisk. The rest of the commit message is not needed Also please add Signed-off-by > > > > All suggestions are welcomed. > > > > P.S. This patch was sent to Debian bug tracking system but was left > > without reply, so I decided to send it here. > > > > -- System Information: > > Debian Release: buster/sid > > APT prefers testing > > APT policy: (900, 'testing'), (500, 'testing-debug'), (10, 'unstable') > > Architecture: amd64 (x86_64) > > Foreign Architectures: i386 > > > > Kernel: Linux 4.14.0-3-amd64 (SMP w/4 CPU cores) > > Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), > > LANGUAGE=en_US:en (charmap=UTF-8) > > Shell: /bin/sh linked to /bin/dash > > Init: systemd (via /run/systemd/system) > > LSM: AppArmor: enabled > > > > Versions of packages mmc-utils depends on: > > ii libc6 2.26-6 > > > > mmc-utils recommends no packages. > > > > mmc-utils suggests no packages. > > > > -- no debconf information > > > > Author: Nikita Maslov <wkernelt...@xxxxxxxxx> > > Description: 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. > > . > > mmc-utils (0+git20170901.37c86e60-1) unstable; urgency=medium > > . > > * Snapshot, taken from the master (20170901). > > * Update supporting version of debhelper to 10. > > * Update Standards-Version to 4.1.0. > > > > Last-Update: 2018-03-07 > > > > --- > > > > --- mmc-utils-0+git20170901.37c86e60.orig/mmc_cmds.c > > +++ mmc-utils-0+git20170901.37c86e60/mmc_cmds.c > > @@ -1799,6 +1799,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, > > @@ -1837,8 +1839,9 @@ static int do_rpmb_op(int fd, > > { > > int err; > > u_int16_t rpmb_type; > > + unsigned i; > > - struct mmc_ioc_cmd ioc = { > > + struct mmc_ioc_cmd ioc_base = { > > .arg = 0x0, > > .blksz = 512, > > .blocks = 1, > > @@ -1848,9 +1851,24 @@ static int do_rpmb_op(int fd, > > .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; one line space please > > + /* 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)); 80 characters > > + if (!mioc) { > > + return -ENOMEM; > > + } > > + > > + for (i = 0; i < RMPB_MULTI_CMD_MAX_CMDS; i++) { > > + memcpy(&mioc->cmds[i], &ioc_base, sizeof (ioc_base)); I find this option of ioc_base less clear than a static inline set_single_cmd that sets opcode and flags. This way you are actively setting what you need for each command, Instead of fixing the default settings. > > + } > > + > > rpmb_type = be16toh(frame_in->req_resp); > > switch(rpmb_type) { You may consider to withdraw from the switch-case structure - you'll end up with less code, But keeping it is fine as well. > > @@ -1861,33 +1879,24 @@ static int do_rpmb_op(int fd, > > goto out; > > } > > + /* try to use multi_cmd if possible */ > > + 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]; > > + ioc->write_flag |= (1<<31); > > /* 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; > > - } > > + ioc->write_flag = 1; > > + ioc->data_ptr = (uintptr_t)frame_out; maybe use mmc_ioc_cmd_set_data? > > /* 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]; > > + ioc->write_flag = 0; > > + ioc->opcode = MMC_READ_MULTIPLE_BLOCK; > > break; > > case MMC_RPMB_READ_CNT: > > @@ -1898,23 +1907,17 @@ 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; > > + > > + /* Request is the same as ioc_base; don't change > > mioc->cmds[0] > > */ 80 characters > > /* 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]; > > + ioc->write_flag = 0; > > + ioc->opcode = MMC_READ_MULTIPLE_BLOCK; > > + ioc->blocks = out_cnt; > > + ioc->data_ptr = (uintptr_t)frame_out; mmc_ioc_cmd_set_data? > > break; > > default: > > @@ -1922,7 +1925,26 @@ static int do_rpmb_op(int fd, > > goto out; > > } One line space > > + /* Try to use MMC_IOC_MULTI_CMD */ No need for that comment > > + err = ioctl(fd, MMC_IOC_MULTI_CMD, mioc); > > + > > + /* If MMC_IOC_MULTI_CMD is not supported, run single commands */ This is actually not needed > > + if (err < 0 && errno == EINVAL) { > > + for (i = 0; i < mioc->num_of_cmds; i++) { > > + err = ioctl(fd, MMC_IOC_CMD, &mioc->cmds[i]); > > + if (err < 0) { > > + err = -errno; > > + goto out; > > + } > > + } > > + } else if (err < 0) { > > + err = -errno; > > + goto out; > > + } > > + > > + > > out: > > + free(mioc); > > return err; > > } > >