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;
> > }
> >
