RE: [PATCH] mmc-utils: RPMB fails with status 0x0001 on system eMMC chips

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Memonry Technology]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux