> Hi Avri,
>
> ext_csd is read in line 2789, close to the beginning of do_ffu.
Ahha - you are in the do_retry clause. Ok.
> The mode check and skip is done by the following snippet in the patch:
> /* check mode operation for ffu install*/
> if (!ext_csd[EXT_CSD_FFU_FEATURES]) {
> fprintf(stderr, "Please reboot to complete firmware installation on
> %s\n", device);
> ret = 0;
> goto out;
> }
> According to the spec when the FFU_FETURES field is 0 then
> MODE_OPERATION_CODES is not supported and sect_done checks should
> be skipped with no error.
> Further reads of ext_csd are done in case FFU_FETURES field is 1 - to check
> the NUM_OF_FW_SEC_PROG value.
Correct.
>
> I admit that the patch looks ugly but that is because the check was moved up
> in the code from an if-else block that had to be refactored.
Actually I find it quite elegant.
Please elaborate the comment above if (!ext_csd[EXT_CSD_FFU_FEATURES]) with your above explanation.
Also please add a fixes tag.
Thanks,
Avri
>
> Best regards,
> Bruno Matić
>
> -----Original Message-----
> From: Avri Altman <Avri.Altman@xxxxxxx>
> Sent: Monday, September 19, 2022 11:49 PM
> To: Avri Altman <Avri.Altman@xxxxxxx>; Matic, Bruno (Nokia - DE/Ulm)
> <bruno.matic@xxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx
> Cc: ulf.hansson@xxxxxxxxxx; Rossler, Jakob (Nokia - DE/Ulm)
> <jakob.rossler@xxxxxxxxx>; Heinonen, Aarne (Nokia - FI/Espoo)
> <aarne.heinonen@xxxxxxxxx>
> Subject: RE: [PATCH] mmc-utils: correct ffu in case of unsupported
> MODE_OPERATION_CODES
>
> >
> > > Move up the return in case MODE_OPERATION_CODES is not supported.
> > > According to the specification when MODE_OPERATION_CODES is not
> > > supported no checks should be done against NUM_OF_FW_SEC_PROG.
> And I wasn't able to find in your code, where you are skipping the check
> against sect_done, In case MODE_OPERATION_CODES field is not supported.
>
>
> Thanks,
> Avri
>
> > >
> > > Signed-off-by: Bruno Matic <bruno.matic@xxxxxxxxx>
> > > ---
> > > mmc_cmds.c | 82
> > > ++++++++++++++++++++++++++++--------------------------
> > > 1 file changed, 42 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/mmc_cmds.c b/mmc_cmds.c index ef1d8c6..81fdc7e 100644
> > > --- a/mmc_cmds.c
> > > +++ b/mmc_cmds.c
> > > @@ -2897,6 +2897,13 @@ do_retry:
> > > goto out;
> > > }
> > >
> > > + /* check mode operation for ffu install*/
> > > + if (!ext_csd[EXT_CSD_FFU_FEATURES]) {
> > You haven't read the ext_csd yet....
> >
> > Thanks,
> > Avri
> >
> > > + fprintf(stderr, "Please reboot to complete firmware
> > > + installation on
> > > %s\n", device);
> > > + ret = 0;
> > > + goto out;
> > > + }
> > > +
> > > ret = read_extcsd(dev_fd, ext_csd);
> > > if (ret) {
> > > fprintf(stderr, "Could not read EXT_CSD from %s\n",
> > > device); @@ -
> > > 2927,49 +2934,44 @@ do_retry:
> > > goto out;
> > > }
> > >
> > > - /* check mode operation for ffu install*/
> > > - if (!ext_csd[EXT_CSD_FFU_FEATURES]) {
> > > - fprintf(stderr, "Please reboot to complete firmware installation
> on
> > > %s\n", device);
> > > - } else {
> > > - fprintf(stderr, "Installing firmware on %s...\n", device);
> > > - /* Re-enter ffu mode and install the firmware */
> > > - multi_cmd->num_of_cmds = 2;
> > > -
> > > - /* set ext_csd to install mode */
> > > - multi_cmd->cmds[1].opcode = MMC_SWITCH;
> > > - multi_cmd->cmds[1].blksz = 0;
> > > - multi_cmd->cmds[1].blocks = 0;
> > > - multi_cmd->cmds[1].arg = (MMC_SWITCH_MODE_WRITE_BYTE
> <<
> > 24)
> > > |
> > > - (EXT_CSD_MODE_OPERATION_CODES << 16) |
> > > - (EXT_CSD_FFU_INSTALL << 8) |
> > > - EXT_CSD_CMD_SET_NORMAL;
> > > - multi_cmd->cmds[1].flags = MMC_RSP_SPI_R1B |
> MMC_RSP_R1B |
> > > MMC_CMD_AC;
> > > - multi_cmd->cmds[1].write_flag = 1;
> > > -
> > > - /* send ioctl with multi-cmd */
> > > - ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> > > + fprintf(stderr, "Installing firmware on %s...\n", device);
> > > + /* Re-enter ffu mode and install the firmware */
> > > + multi_cmd->num_of_cmds = 2;
> > >
> > > - if (ret) {
> > > - perror("Multi-cmd ioctl failed setting install mode");
> > > - /* In case multi-cmd ioctl failed before exiting from ffu mode
> */
> > > - ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> > > - goto out;
> > > - }
> > > + /* set ext_csd to install mode */
> > > + multi_cmd->cmds[1].opcode = MMC_SWITCH;
> > > + multi_cmd->cmds[1].blksz = 0;
> > > + multi_cmd->cmds[1].blocks = 0;
> > > + multi_cmd->cmds[1].arg = (MMC_SWITCH_MODE_WRITE_BYTE <<
> 24) |
> > > + (EXT_CSD_MODE_OPERATION_CODES << 16) |
> > > + (EXT_CSD_FFU_INSTALL << 8) |
> > > + EXT_CSD_CMD_SET_NORMAL;
> > > + multi_cmd->cmds[1].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B |
> > > MMC_CMD_AC;
> > > + multi_cmd->cmds[1].write_flag = 1;
> > >
> > > - ret = read_extcsd(dev_fd, ext_csd);
> > > - if (ret) {
> > > - fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> > > - goto out;
> > > - }
> > > + /* send ioctl with multi-cmd */
> > > + ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> > >
> > > - /* return status */
> > > - ret = ext_csd[EXT_CSD_FFU_STATUS];
> > > - if (ret) {
> > > - fprintf(stderr, "%s: error %d during FFU install:\n", device,
> ret);
> > > - goto out;
> > > - } else {
> > > - fprintf(stderr, "FFU finished successfully\n");
> > > - }
> > > + if (ret) {
> > > + perror("Multi-cmd ioctl failed setting install mode");
> > > + /* In case multi-cmd ioctl failed before exiting from ffu mode */
> > > + ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> > > + goto out;
> > > + }
> > > +
> > > + ret = read_extcsd(dev_fd, ext_csd);
> > > + if (ret) {
> > > + fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> > > + goto out;
> > > + }
> > > +
> > > + /* return status */
> > > + ret = ext_csd[EXT_CSD_FFU_STATUS];
> > > + if (ret) {
> > > + fprintf(stderr, "%s: error %d during FFU install:\n", device, ret);
> > > + goto out;
> > > + } else {
> > > + fprintf(stderr, "FFU finished successfully\n");
> > > }
> > >
> > > out:
> > > --
> > > 2.29.0
