Re: [PATCH v1 2/2] mmc: core: Improve RPMB frame handling code

[Bean Huo <beanhuo@iokpp.de>]

On Wed, 2025-09-10 at 07:16 +0000, Avri Altman wrote:
> > From: Bean Huo <beanhuo@micron.com>
> > 
> > Simplify the switch statement by combining RPMB_PROGRAM_KEY and
> > RPMB_GET_WRITE_COUNTER
> > cases which have identical validation logic. Add RPMB_FRAME_SIZE macro
> > and replace all
> > sizeof(struct rpmb_frame) occurrences for better maintainability and
> > readability.
> > 
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > ---
> >   drivers/mmc/core/block.c | 35 +++++++++++++++--------------------
> >   1 file changed, 15 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > index dd6cffc0df72..7fe9e8cc1ce4 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -121,6 +121,8 @@ struct rpmb_frame {
> >   #define RPMB_READ_DATA         0x4    /* Read data from RPMB partition */
> >   #define RPMB_RESULT_READ       0x5    /* Read result request  (Internal) */
> > 
> > +#define RPMB_FRAME_SIZE        sizeof(struct rpmb_frame)
> Since you only check != and alignment, maybe you could further generalize it by using:
> #define CHECK_SIZE_NEQ(val) ((val) != sizeof(struct rpmb_frame))
> #define CHECK_SIZE_ALIGNED(val) IS_ALIGNED((val), sizeof(struct rpmb_frame))
> 
> >   static DEFINE_MUTEX(block_mutex);
> > 
> >   /*
> > @@ -2864,12 +2866,12 @@ static void set_idata(struct mmc_blk_ioc_data
> > *idata, u32 opcode,
> >           * The size of an RPMB frame must match what's expected by the
> >           * hardware.
> >           */
> > -       BUILD_BUG_ON(sizeof(struct rpmb_frame) != 512);
> > +       BUILD_BUG_ON(RPMB_FRAME_SIZE != 512);
> Maybe while at it change this to static_assert() ?
> 
> > 
> >          idata->ic.opcode = opcode;
> >          idata->ic.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
> >          idata->ic.write_flag = write_flag;
> > -       idata->ic.blksz = sizeof(struct rpmb_frame);
> > +       idata->ic.blksz = RPMB_FRAME_SIZE;
> >          idata->ic.blocks = buf_bytes /  idata->ic.blksz;
> >          idata->buf = buf;
> >          idata->buf_bytes = buf_bytes;
> > @@ -2893,32 +2895,27 @@ static int mmc_route_rpmb_frames(struct device
> > *dev, u8 *req,
> >          if (IS_ERR(md->queue.card))
> >                  return PTR_ERR(md->queue.card);
> > 
> > -       if (req_len < sizeof(*frm))
> > +       if (req_len < RPMB_FRAME_SIZE)
> >                  return -EINVAL;
> > 
> >          req_type = be16_to_cpu(frm->req_resp);
> >          switch (req_type) {
> >          case RPMB_PROGRAM_KEY:
> > -               if (req_len != sizeof(struct rpmb_frame) ||
> > -                   resp_len != sizeof(struct rpmb_frame))
> > -                       return -EINVAL;
> > -               write = true;
> > -               break;
> >          case RPMB_GET_WRITE_COUNTER:
> > -               if (req_len != sizeof(struct rpmb_frame) ||
> > -                   resp_len != sizeof(struct rpmb_frame))
> > +               if (req_len != RPMB_FRAME_SIZE ||
> > +                   resp_len != RPMB_FRAME_SIZE)
> >                          return -EINVAL;
> > -               write = false;
> > +               write = (req_type == RPMB_PROGRAM_KEY);
> A little bit awkward?  Maybe leave this for now?

Avri, 

thank you for reviewing. I’ll incorporate your suggestions in the next version.

Kind regards,
Bean