* [PATCH v1 0/2] mmc: core: RPMB code improvements @ 2025-09-09 21:13 Bean Huo 2025-09-09 21:13 ` [PATCH v1 1/2] mmc: core: Fix variable shadowing in mmc_route_rpmb_frames() Bean Huo 2025-09-09 21:13 ` [PATCH v1 2/2] mmc: core: Improve RPMB frame handling code Bean Huo 0 siblings, 2 replies; 7+ messages in thread From: Bean Huo @ 2025-09-09 21:13 UTC (permalink / raw) To: ulf.hansson, linux-mmc, jens.wiklander; +Cc: Bean Huo This patch series improves the RPMB frame handling code in the MMC block driver with several code quality enhancements. Bean Huo (2): mmc: core: Fix variable shadowing in mmc_route_rpmb_frames() mmc: core: Improve RPMB frame handling code drivers/mmc/core/block.c | 39 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 22 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v1 1/2] mmc: core: Fix variable shadowing in mmc_route_rpmb_frames() 2025-09-09 21:13 [PATCH v1 0/2] mmc: core: RPMB code improvements Bean Huo @ 2025-09-09 21:13 ` Bean Huo 2025-09-10 5:42 ` Avri Altman 2025-09-09 21:13 ` [PATCH v1 2/2] mmc: core: Improve RPMB frame handling code Bean Huo 1 sibling, 1 reply; 7+ messages in thread From: Bean Huo @ 2025-09-09 21:13 UTC (permalink / raw) To: ulf.hansson, linux-mmc, jens.wiklander; +Cc: Bean Huo From: Bean Huo <beanhuo@micron.com> Rename the inner 'frm' variable to 'resp_frm' in the write path of mmc_route_rpmb_frames() to avoid shadowing the outer 'frm' variable. The function declares 'frm' at function scope pointing to the request frame, but then redeclares another 'frm' variable inside the write block pointing to the response frame. This shadowing makes the code confusing and error-prone. Using 'resp_frm' for the response frame makes the distinction clear and improves code readability. Signed-off-by: Bean Huo <beanhuo@micron.com> --- drivers/mmc/core/block.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 9cc47bf94804..dd6cffc0df72 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -2936,15 +2936,15 @@ static int mmc_route_rpmb_frames(struct device *dev, u8 *req, return -ENOMEM; if (write) { - struct rpmb_frame *frm = (struct rpmb_frame *)resp; + struct rpmb_frame *resp_frm = (struct rpmb_frame *)resp; /* Send write request frame(s) */ set_idata(idata[0], MMC_WRITE_MULTIPLE_BLOCK, 1 | MMC_CMD23_ARG_REL_WR, req, req_len); /* Send result request frame */ - memset(frm, 0, sizeof(*frm)); - frm->req_resp = cpu_to_be16(RPMB_RESULT_READ); + memset(resp_frm, 0, sizeof(*resp_frm)); + resp_frm->req_resp = cpu_to_be16(RPMB_RESULT_READ); set_idata(idata[1], MMC_WRITE_MULTIPLE_BLOCK, 1, resp, resp_len); -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH v1 1/2] mmc: core: Fix variable shadowing in mmc_route_rpmb_frames() 2025-09-09 21:13 ` [PATCH v1 1/2] mmc: core: Fix variable shadowing in mmc_route_rpmb_frames() Bean Huo @ 2025-09-10 5:42 ` Avri Altman 2025-09-10 6:17 ` Jens Wiklander 0 siblings, 1 reply; 7+ messages in thread From: Avri Altman @ 2025-09-10 5:42 UTC (permalink / raw) To: Bean Huo, ulf.hansson@linaro.org, linux-mmc@vger.kernel.org, jens.wiklander@linaro.org Cc: Bean Huo > > From: Bean Huo <beanhuo@micron.com> > > Rename the inner 'frm' variable to 'resp_frm' in the write path of > mmc_route_rpmb_frames() to avoid shadowing the outer 'frm' variable. > > The function declares 'frm' at function scope pointing to the request frame, > but then redeclares another 'frm' variable inside the write block pointing to > the response frame. This shadowing makes the code confusing and error- > prone. > > Using 'resp_frm' for the response frame makes the distinction clear and > improves code readability. Fixes: 7852028a35f0 ("mmc: block: register RPMB partition with the RPMB subsystem") > Signed-off-by: Bean Huo <beanhuo@micron.com> Reviewed-by: Avri Altman <avri.altman@sandisk.com> > --- > drivers/mmc/core/block.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index > 9cc47bf94804..dd6cffc0df72 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -2936,15 +2936,15 @@ static int mmc_route_rpmb_frames(struct device > *dev, u8 *req, > return -ENOMEM; > > if (write) { > - struct rpmb_frame *frm = (struct rpmb_frame *)resp; > + struct rpmb_frame *resp_frm = (struct rpmb_frame *)resp; > > /* Send write request frame(s) */ > set_idata(idata[0], MMC_WRITE_MULTIPLE_BLOCK, > 1 | MMC_CMD23_ARG_REL_WR, req, req_len); > > /* Send result request frame */ > - memset(frm, 0, sizeof(*frm)); > - frm->req_resp = cpu_to_be16(RPMB_RESULT_READ); > + memset(resp_frm, 0, sizeof(*resp_frm)); > + resp_frm->req_resp = cpu_to_be16(RPMB_RESULT_READ); > set_idata(idata[1], MMC_WRITE_MULTIPLE_BLOCK, 1, resp, > resp_len); > > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/2] mmc: core: Fix variable shadowing in mmc_route_rpmb_frames() 2025-09-10 5:42 ` Avri Altman @ 2025-09-10 6:17 ` Jens Wiklander 0 siblings, 0 replies; 7+ messages in thread From: Jens Wiklander @ 2025-09-10 6:17 UTC (permalink / raw) To: Avri Altman Cc: Bean Huo, ulf.hansson@linaro.org, linux-mmc@vger.kernel.org, Bean Huo On Wed, Sep 10, 2025 at 7:42 AM Avri Altman <Avri.Altman@sandisk.com> wrote: > > > > > From: Bean Huo <beanhuo@micron.com> > > > > Rename the inner 'frm' variable to 'resp_frm' in the write path of > > mmc_route_rpmb_frames() to avoid shadowing the outer 'frm' variable. > > > > The function declares 'frm' at function scope pointing to the request frame, > > but then redeclares another 'frm' variable inside the write block pointing to > > the response frame. This shadowing makes the code confusing and error- > > prone. > > > > Using 'resp_frm' for the response frame makes the distinction clear and > > improves code readability. > > Fixes: 7852028a35f0 ("mmc: block: register RPMB partition with the RPMB subsystem") > > Signed-off-by: Bean Huo <beanhuo@micron.com> > Reviewed-by: Avri Altman <avri.altman@sandisk.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Thanks, Jens > > > > --- > > drivers/mmc/core/block.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index > > 9cc47bf94804..dd6cffc0df72 100644 > > --- a/drivers/mmc/core/block.c > > +++ b/drivers/mmc/core/block.c > > @@ -2936,15 +2936,15 @@ static int mmc_route_rpmb_frames(struct device > > *dev, u8 *req, > > return -ENOMEM; > > > > if (write) { > > - struct rpmb_frame *frm = (struct rpmb_frame *)resp; > > + struct rpmb_frame *resp_frm = (struct rpmb_frame *)resp; > > > > /* Send write request frame(s) */ > > set_idata(idata[0], MMC_WRITE_MULTIPLE_BLOCK, > > 1 | MMC_CMD23_ARG_REL_WR, req, req_len); > > > > /* Send result request frame */ > > - memset(frm, 0, sizeof(*frm)); > > - frm->req_resp = cpu_to_be16(RPMB_RESULT_READ); > > + memset(resp_frm, 0, sizeof(*resp_frm)); > > + resp_frm->req_resp = cpu_to_be16(RPMB_RESULT_READ); > > set_idata(idata[1], MMC_WRITE_MULTIPLE_BLOCK, 1, resp, > > resp_len); > > > > -- > > 2.34.1 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v1 2/2] mmc: core: Improve RPMB frame handling code 2025-09-09 21:13 [PATCH v1 0/2] mmc: core: RPMB code improvements Bean Huo 2025-09-09 21:13 ` [PATCH v1 1/2] mmc: core: Fix variable shadowing in mmc_route_rpmb_frames() Bean Huo @ 2025-09-09 21:13 ` Bean Huo 2025-09-10 7:16 ` Avri Altman 1 sibling, 1 reply; 7+ messages in thread From: Bean Huo @ 2025-09-09 21:13 UTC (permalink / raw) To: ulf.hansson, linux-mmc, jens.wiklander; +Cc: Bean Huo 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) + 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); 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); break; case RPMB_WRITE_DATA: - 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 = true; break; case RPMB_READ_DATA: - 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; break; @@ -2926,10 +2923,8 @@ static int mmc_route_rpmb_frames(struct device *dev, u8 *req, return -EINVAL; } - if (write) - cmd_count = 3; - else - cmd_count = 2; + /* Write operations require 3 commands, read operations require 2 */ + cmd_count = write ? 3 : 2; idata = alloc_idata(rpmb, cmd_count); if (!idata) @@ -2943,7 +2938,7 @@ static int mmc_route_rpmb_frames(struct device *dev, u8 *req, 1 | MMC_CMD23_ARG_REL_WR, req, req_len); /* Send result request frame */ - memset(resp_frm, 0, sizeof(*resp_frm)); + memset(resp_frm, 0, RPMB_FRAME_SIZE); resp_frm->req_resp = cpu_to_be16(RPMB_RESULT_READ); set_idata(idata[1], MMC_WRITE_MULTIPLE_BLOCK, 1, resp, resp_len); -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH v1 2/2] mmc: core: Improve RPMB frame handling code 2025-09-09 21:13 ` [PATCH v1 2/2] mmc: core: Improve RPMB frame handling code Bean Huo @ 2025-09-10 7:16 ` Avri Altman 2025-09-10 19:23 ` Bean Huo 0 siblings, 1 reply; 7+ messages in thread From: Avri Altman @ 2025-09-10 7:16 UTC (permalink / raw) To: Bean Huo, ulf.hansson@linaro.org, linux-mmc@vger.kernel.org, jens.wiklander@linaro.org Cc: Bean Huo > 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? Thanks, Avri > break; > case RPMB_WRITE_DATA: > - 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 = true; > break; > case RPMB_READ_DATA: > - 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; > break; > @@ -2926,10 +2923,8 @@ static int mmc_route_rpmb_frames(struct device > *dev, u8 *req, > return -EINVAL; > } > > - if (write) > - cmd_count = 3; > - else > - cmd_count = 2; > + /* Write operations require 3 commands, read operations require 2 */ > + cmd_count = write ? 3 : 2; > > idata = alloc_idata(rpmb, cmd_count); > if (!idata) > @@ -2943,7 +2938,7 @@ static int mmc_route_rpmb_frames(struct device > *dev, u8 *req, > 1 | MMC_CMD23_ARG_REL_WR, req, req_len); > > /* Send result request frame */ > - memset(resp_frm, 0, sizeof(*resp_frm)); > + memset(resp_frm, 0, RPMB_FRAME_SIZE); > resp_frm->req_resp = cpu_to_be16(RPMB_RESULT_READ); > set_idata(idata[1], MMC_WRITE_MULTIPLE_BLOCK, 1, resp, > resp_len); > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 2/2] mmc: core: Improve RPMB frame handling code 2025-09-10 7:16 ` Avri Altman @ 2025-09-10 19:23 ` Bean Huo 0 siblings, 0 replies; 7+ messages in thread From: Bean Huo @ 2025-09-10 19:23 UTC (permalink / raw) To: Avri Altman, ulf.hansson@linaro.org, linux-mmc@vger.kernel.org, jens.wiklander@linaro.org Cc: Bean Huo 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-09-10 19:24 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-09-09 21:13 [PATCH v1 0/2] mmc: core: RPMB code improvements Bean Huo 2025-09-09 21:13 ` [PATCH v1 1/2] mmc: core: Fix variable shadowing in mmc_route_rpmb_frames() Bean Huo 2025-09-10 5:42 ` Avri Altman 2025-09-10 6:17 ` Jens Wiklander 2025-09-09 21:13 ` [PATCH v1 2/2] mmc: core: Improve RPMB frame handling code Bean Huo 2025-09-10 7:16 ` Avri Altman 2025-09-10 19:23 ` Bean Huo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).