diff mbox series

mmc-utils: correct and clean up the file handling

Message ID AM5PR0701MB2964A49C4E5EEA8905926120EFB89@AM5PR0701MB2964.eurprd07.prod.outlook.com (mailing list archive)
State New, archived
Headers show
Series mmc-utils: correct and clean up the file handling | expand

Commit Message

Bruno Matic June 28, 2022, 2:40 p.m. UTC
Add the check if the whole firmware was loaded.
Cleaned up the leftovers of handling the file in chunks.

Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
---
 mmc_cmds.c | 69 +++++++++++++++++++++++++++---------------------------
 1 file changed, 34 insertions(+), 35 deletions(-)

Comments

Avri Altman June 28, 2022, 9:55 p.m. UTC | #1
Hi Bruno,
Thank you for your patch.

> Add the check if the whole firmware was loaded.
> Cleaned up the leftovers of handling the file in chunks.
> 
> Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
Christian proposed a fix to do_ffu about a week ago, 
see e.g. https://www.spinics.net/lists/linux-mmc/msg70961.html.

Would you mind waiting for few more days to allow it to finalize,
And then rebase your change and resend?

Thanks,
Avri

> ---
>  mmc_cmds.c | 69 +++++++++++++++++++++++++++---------------------------
>  1 file changed, 34 insertions(+), 35 deletions(-)
> 
> diff --git a/mmc_cmds.c b/mmc_cmds.c
> index 70480df..e64c747 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
>         __u8 *buf = NULL;
>         __u32 arg;
>         off_t fw_size;
> -       ssize_t chunk_size;
>         char *device;
>         struct mmc_ioc_multi_cmd *multi_cmd = NULL;
>         __u32 blocks = 1;
> @@ -2925,45 +2924,45 @@ int do_ffu(int nargs, char **argv)
>         multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B |
> MMC_CMD_AC;
>         multi_cmd->cmds[3].write_flag = 1;
> 
> -do_retry:
> -       /* read firmware chunk */
> +       /* read firmware */
>         lseek(img_fd, 0, SEEK_SET);
> -       chunk_size = read(img_fd, buf, fw_size);
> +       if (read(img_fd, buf, fw_size) != fw_size) {
> +               fprintf(stderr, "Could not read the whole firmware file\n");
> +               ret = -ENOSPC;
> +               goto out;
> +       }
> 
> -       if (chunk_size > 0) {
> -               /* send ioctl with multi-cmd */
> -               ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> +do_retry:
> +       /* send ioctl with multi-cmd */
> +       ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> 
> -               if (ret) {
> -                       perror("Multi-cmd ioctl");
> -                       /* In case multi-cmd ioctl failed before exiting from ffu mode */
> -                       ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> -                       goto out;
> -               }
> +       if (ret) {
> +               perror("Multi-cmd ioctl");
> +               /* 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;
> -               }
> +       ret = read_extcsd(dev_fd, ext_csd);
> +       if (ret) {
> +               fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> +               goto out;
> +       }
> 
> -               /* Test if we need to restart the download */
> -               sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> -               /* By spec, host should re-start download from the first sector if
> sect_done is 0 */
> -               if (sect_done == 0) {
> -                       if (retry > 0) {
> -                               retry--;
> -                               fprintf(stderr, "Programming failed. Retrying... (%d)\n",
> retry);
> -                               goto do_retry;
> -                       }
> -                       fprintf(stderr, "Programming failed! Aborting...\n");
> -                       goto out;
> -               } else {
> -                       fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done *
> sect_size, (intmax_t)fw_size);
> +       /* Test if we need to restart the download */
> +       sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> +       /* By spec, host should re-start download from the first sector if
> sect_done is 0 */
> +       if (sect_done == 0) {
> +               if (retry > 0) {
> +                       retry--;
> +                       fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
> +                       goto do_retry;
>                 }
> +               fprintf(stderr, "Programming failed! Aborting...\n");
> +               goto out;
>         }
> 
>         if ((sect_done * sect_size) == fw_size) {
Bruno Matic June 29, 2022, 7:33 a.m. UTC | #2
Hi Avri,

That is ok, I will wait.

Best regards,
Bruno Matić

-----Original Message-----
From: Avri Altman <Avri.Altman@wdc.com> 
Sent: Tuesday, June 28, 2022 11:56 PM
To: Matic, Bruno (Nokia - DE/Ulm) <bruno.matic@nokia.com>; linux-mmc@vger.kernel.org
Cc: Ulf Hansson <ulf.hansson@linaro.org>; Christian Löhle <CLoehle@hyperstone.com>
Subject: RE: [PATCH] mmc-utils: correct and clean up the file handling

Hi Bruno,
Thank you for your patch.

> Add the check if the whole firmware was loaded.
> Cleaned up the leftovers of handling the file in chunks.
> 
> Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
Christian proposed a fix to do_ffu about a week ago, see e.g. https://www.spinics.net/lists/linux-mmc/msg70961.html.

Would you mind waiting for few more days to allow it to finalize, And then rebase your change and resend?

Thanks,
Avri

> ---
>  mmc_cmds.c | 69 
> +++++++++++++++++++++++++++---------------------------
>  1 file changed, 34 insertions(+), 35 deletions(-)
> 
> diff --git a/mmc_cmds.c b/mmc_cmds.c
> index 70480df..e64c747 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
>         __u8 *buf = NULL;
>         __u32 arg;
>         off_t fw_size;
> -       ssize_t chunk_size;
>         char *device;
>         struct mmc_ioc_multi_cmd *multi_cmd = NULL;
>         __u32 blocks = 1;
> @@ -2925,45 +2924,45 @@ int do_ffu(int nargs, char **argv)
>         multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | 
> MMC_CMD_AC;
>         multi_cmd->cmds[3].write_flag = 1;
> 
> -do_retry:
> -       /* read firmware chunk */
> +       /* read firmware */
>         lseek(img_fd, 0, SEEK_SET);
> -       chunk_size = read(img_fd, buf, fw_size);
> +       if (read(img_fd, buf, fw_size) != fw_size) {
> +               fprintf(stderr, "Could not read the whole firmware file\n");
> +               ret = -ENOSPC;
> +               goto out;
> +       }
> 
> -       if (chunk_size > 0) {
> -               /* send ioctl with multi-cmd */
> -               ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> +do_retry:
> +       /* send ioctl with multi-cmd */
> +       ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> 
> -               if (ret) {
> -                       perror("Multi-cmd ioctl");
> -                       /* In case multi-cmd ioctl failed before exiting from ffu mode */
> -                       ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> -                       goto out;
> -               }
> +       if (ret) {
> +               perror("Multi-cmd ioctl");
> +               /* 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;
> -               }
> +       ret = read_extcsd(dev_fd, ext_csd);
> +       if (ret) {
> +               fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> +               goto out;
> +       }
> 
> -               /* Test if we need to restart the download */
> -               sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> -               /* By spec, host should re-start download from the first sector if
> sect_done is 0 */
> -               if (sect_done == 0) {
> -                       if (retry > 0) {
> -                               retry--;
> -                               fprintf(stderr, "Programming failed. Retrying... (%d)\n",
> retry);
> -                               goto do_retry;
> -                       }
> -                       fprintf(stderr, "Programming failed! Aborting...\n");
> -                       goto out;
> -               } else {
> -                       fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done *
> sect_size, (intmax_t)fw_size);
> +       /* Test if we need to restart the download */
> +       sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> +       /* By spec, host should re-start download from the first 
> + sector if
> sect_done is 0 */
> +       if (sect_done == 0) {
> +               if (retry > 0) {
> +                       retry--;
> +                       fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
> +                       goto do_retry;
>                 }
> +               fprintf(stderr, "Programming failed! Aborting...\n");
> +               goto out;
>         }
> 
>         if ((sect_done * sect_size) == fw_size) {
Bruno Matic July 7, 2022, 11:06 a.m. UTC | #3
Hi everyone,

If I may ask if there is any update on the state of the merge since I can't see anything in the git log.

Best regards,
Bruno Matić

-----Original Message-----
From: Matic, Bruno (Nokia - DE/Ulm) 
Sent: Wednesday, June 29, 2022 9:33 AM
To: Avri Altman <Avri.Altman@wdc.com>; linux-mmc@vger.kernel.org
Cc: Ulf Hansson <ulf.hansson@linaro.org>; Christian Löhle <CLoehle@hyperstone.com>
Subject: RE: [PATCH] mmc-utils: correct and clean up the file handling

Hi Avri,

That is ok, I will wait.

Best regards,
Bruno Matić

-----Original Message-----
From: Avri Altman <Avri.Altman@wdc.com>
Sent: Tuesday, June 28, 2022 11:56 PM
To: Matic, Bruno (Nokia - DE/Ulm) <bruno.matic@nokia.com>; linux-mmc@vger.kernel.org
Cc: Ulf Hansson <ulf.hansson@linaro.org>; Christian Löhle <CLoehle@hyperstone.com>
Subject: RE: [PATCH] mmc-utils: correct and clean up the file handling

Hi Bruno,
Thank you for your patch.

> Add the check if the whole firmware was loaded.
> Cleaned up the leftovers of handling the file in chunks.
> 
> Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
Christian proposed a fix to do_ffu about a week ago, see e.g. https://www.spinics.net/lists/linux-mmc/msg70961.html.

Would you mind waiting for few more days to allow it to finalize, And then rebase your change and resend?

Thanks,
Avri

> ---
>  mmc_cmds.c | 69
> +++++++++++++++++++++++++++---------------------------
>  1 file changed, 34 insertions(+), 35 deletions(-)
> 
> diff --git a/mmc_cmds.c b/mmc_cmds.c
> index 70480df..e64c747 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
>         __u8 *buf = NULL;
>         __u32 arg;
>         off_t fw_size;
> -       ssize_t chunk_size;
>         char *device;
>         struct mmc_ioc_multi_cmd *multi_cmd = NULL;
>         __u32 blocks = 1;
> @@ -2925,45 +2924,45 @@ int do_ffu(int nargs, char **argv)
>         multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | 
> MMC_CMD_AC;
>         multi_cmd->cmds[3].write_flag = 1;
> 
> -do_retry:
> -       /* read firmware chunk */
> +       /* read firmware */
>         lseek(img_fd, 0, SEEK_SET);
> -       chunk_size = read(img_fd, buf, fw_size);
> +       if (read(img_fd, buf, fw_size) != fw_size) {
> +               fprintf(stderr, "Could not read the whole firmware file\n");
> +               ret = -ENOSPC;
> +               goto out;
> +       }
> 
> -       if (chunk_size > 0) {
> -               /* send ioctl with multi-cmd */
> -               ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> +do_retry:
> +       /* send ioctl with multi-cmd */
> +       ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> 
> -               if (ret) {
> -                       perror("Multi-cmd ioctl");
> -                       /* In case multi-cmd ioctl failed before exiting from ffu mode */
> -                       ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> -                       goto out;
> -               }
> +       if (ret) {
> +               perror("Multi-cmd ioctl");
> +               /* 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;
> -               }
> +       ret = read_extcsd(dev_fd, ext_csd);
> +       if (ret) {
> +               fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> +               goto out;
> +       }
> 
> -               /* Test if we need to restart the download */
> -               sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> -               /* By spec, host should re-start download from the first sector if
> sect_done is 0 */
> -               if (sect_done == 0) {
> -                       if (retry > 0) {
> -                               retry--;
> -                               fprintf(stderr, "Programming failed. Retrying... (%d)\n",
> retry);
> -                               goto do_retry;
> -                       }
> -                       fprintf(stderr, "Programming failed! Aborting...\n");
> -                       goto out;
> -               } else {
> -                       fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done *
> sect_size, (intmax_t)fw_size);
> +       /* Test if we need to restart the download */
> +       sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> +       /* By spec, host should re-start download from the first 
> + sector if
> sect_done is 0 */
> +       if (sect_done == 0) {
> +               if (retry > 0) {
> +                       retry--;
> +                       fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
> +                       goto do_retry;
>                 }
> +               fprintf(stderr, "Programming failed! Aborting...\n");
> +               goto out;
>         }
> 
>         if ((sect_done * sect_size) == fw_size) {
Avri Altman July 7, 2022, 11:59 a.m. UTC | #4
> Hi everyone,
> 
> If I may ask if there is any update on the state of the merge since I can't see
> anything in the git log.
I am following this - Ulf didn't pick it up yet.
Will ping you once he will.

Thanks,
Avri

> 
> Best regards,
> Bruno Matić
> 
> -----Original Message-----
> From: Matic, Bruno (Nokia - DE/Ulm)
> Sent: Wednesday, June 29, 2022 9:33 AM
> To: Avri Altman <Avri.Altman@wdc.com>; linux-mmc@vger.kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>; Christian Löhle
> <CLoehle@hyperstone.com>
> Subject: RE: [PATCH] mmc-utils: correct and clean up the file handling
> 
> Hi Avri,
> 
> That is ok, I will wait.
> 
> Best regards,
> Bruno Matić
> 
> -----Original Message-----
> From: Avri Altman <Avri.Altman@wdc.com>
> Sent: Tuesday, June 28, 2022 11:56 PM
> To: Matic, Bruno (Nokia - DE/Ulm) <bruno.matic@nokia.com>; linux-
> mmc@vger.kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>; Christian Löhle
> <CLoehle@hyperstone.com>
> Subject: RE: [PATCH] mmc-utils: correct and clean up the file handling
> 
> Hi Bruno,
> Thank you for your patch.
> 
> > Add the check if the whole firmware was loaded.
> > Cleaned up the leftovers of handling the file in chunks.
> >
> > Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
> Christian proposed a fix to do_ffu about a week ago, see e.g.
> https://www.spinics.net/lists/linux-mmc/msg70961.html.
> 
> Would you mind waiting for few more days to allow it to finalize, And then
> rebase your change and resend?
> 
> Thanks,
> Avri
> 
> > ---
> >  mmc_cmds.c | 69
> > +++++++++++++++++++++++++++---------------------------
> >  1 file changed, 34 insertions(+), 35 deletions(-)
> >
> > diff --git a/mmc_cmds.c b/mmc_cmds.c
> > index 70480df..e64c747 100644
> > --- a/mmc_cmds.c
> > +++ b/mmc_cmds.c
> > @@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
> >         __u8 *buf = NULL;
> >         __u32 arg;
> >         off_t fw_size;
> > -       ssize_t chunk_size;
> >         char *device;
> >         struct mmc_ioc_multi_cmd *multi_cmd = NULL;
> >         __u32 blocks = 1;
> > @@ -2925,45 +2924,45 @@ int do_ffu(int nargs, char **argv)
> >         multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B |
> > MMC_CMD_AC;
> >         multi_cmd->cmds[3].write_flag = 1;
> >
> > -do_retry:
> > -       /* read firmware chunk */
> > +       /* read firmware */
> >         lseek(img_fd, 0, SEEK_SET);
> > -       chunk_size = read(img_fd, buf, fw_size);
> > +       if (read(img_fd, buf, fw_size) != fw_size) {
> > +               fprintf(stderr, "Could not read the whole firmware file\n");
> > +               ret = -ENOSPC;
> > +               goto out;
> > +       }
> >
> > -       if (chunk_size > 0) {
> > -               /* send ioctl with multi-cmd */
> > -               ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> > +do_retry:
> > +       /* send ioctl with multi-cmd */
> > +       ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> >
> > -               if (ret) {
> > -                       perror("Multi-cmd ioctl");
> > -                       /* In case multi-cmd ioctl failed before exiting from ffu mode
> */
> > -                       ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> > -                       goto out;
> > -               }
> > +       if (ret) {
> > +               perror("Multi-cmd ioctl");
> > +               /* 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;
> > -               }
> > +       ret = read_extcsd(dev_fd, ext_csd);
> > +       if (ret) {
> > +               fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> > +               goto out;
> > +       }
> >
> > -               /* Test if we need to restart the download */
> > -               sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> > -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> > -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> > -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> > -               /* By spec, host should re-start download from the first sector if
> > sect_done is 0 */
> > -               if (sect_done == 0) {
> > -                       if (retry > 0) {
> > -                               retry--;
> > -                               fprintf(stderr, "Programming failed. Retrying... (%d)\n",
> > retry);
> > -                               goto do_retry;
> > -                       }
> > -                       fprintf(stderr, "Programming failed! Aborting...\n");
> > -                       goto out;
> > -               } else {
> > -                       fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done *
> > sect_size, (intmax_t)fw_size);
> > +       /* Test if we need to restart the download */
> > +       sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> > +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> > +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> > +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> > +       /* By spec, host should re-start download from the first
> > + sector if
> > sect_done is 0 */
> > +       if (sect_done == 0) {
> > +               if (retry > 0) {
> > +                       retry--;
> > +                       fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
> > +                       goto do_retry;
> >                 }
> > +               fprintf(stderr, "Programming failed! Aborting...\n");
> > +               goto out;
> >         }
> >
> >         if ((sect_done * sect_size) == fw_size) {
Ulf Hansson July 13, 2022, 11:03 a.m. UTC | #5
On Tue, 28 Jun 2022 at 23:55, Avri Altman <Avri.Altman@wdc.com> wrote:
>
> Hi Bruno,
> Thank you for your patch.
>
> > Add the check if the whole firmware was loaded.
> > Cleaned up the leftovers of handling the file in chunks.
> >
> > Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
> Christian proposed a fix to do_ffu about a week ago,
> see e.g. https://www.spinics.net/lists/linux-mmc/msg70961.html.
>
> Would you mind waiting for few more days to allow it to finalize,
> And then rebase your change and resend?

FYI, the patch from Christian has been applied now. Apologize for the delay.

[...]

Kind regards
Uffe
Bruno Matic July 19, 2022, 8:03 a.m. UTC | #6
Hi everyone,

Here is the rebased patch.

Add the check if the whole firmware was loaded.
Cleaned up the leftovers of handling the file in chunks.

Signed-off-by: Bruno Matic <bruno.matic@nokia.com>

---
 mmc_cmds.c | 67 +++++++++++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/mmc_cmds.c b/mmc_cmds.c
index 8d7910e..d017b64 100644
--- a/mmc_cmds.c
+++ b/mmc_cmds.c
@@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
 	__u8 *buf = NULL;
 	__u32 arg;
 	off_t fw_size;
-	ssize_t chunk_size;
 	char *device;
 	struct mmc_ioc_multi_cmd *multi_cmd = NULL;
 
@@ -2926,45 +2925,45 @@ int do_ffu(int nargs, char **argv)
 	multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
 	multi_cmd->cmds[3].write_flag = 1;
 
-do_retry:
-	/* read firmware chunk */
+	/* read firmware */
 	lseek(img_fd, 0, SEEK_SET);
-	chunk_size = read(img_fd, buf, fw_size);
+	if (read(img_fd, buf, fw_size) != fw_size) {
+		fprintf(stderr, "Could not read the whole firmware file\n");
+		ret = -ENOSPC;
+		goto out;
+	}
 
-	if (chunk_size > 0) {
-		/* send ioctl with multi-cmd */
-		ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
+do_retry:
+	/* send ioctl with multi-cmd */
+	ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
 
-		if (ret) {
-			perror("Multi-cmd ioctl");
-			/* In case multi-cmd ioctl failed before exiting from ffu mode */
-			ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
-			goto out;
-		}
+	if (ret) {
+		perror("Multi-cmd ioctl");
+		/* 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;
-		}
+	ret = read_extcsd(dev_fd, ext_csd);
+	if (ret) {
+		fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
+		goto out;
+	}
 
-		/* Test if we need to restart the download */
-		sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
-				ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
-				ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
-				ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
-		/* By spec, host should re-start download from the first sector if sect_done is 0 */
-		if (sect_done == 0) {
-			if (retry > 0) {
-				retry--;
-				fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
-				goto do_retry;
-			}
-			fprintf(stderr, "Programming failed! Aborting...\n");
-			goto out;
-		} else {
-			fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done * sect_size, (intmax_t)fw_size);
+	/* Test if we need to restart the download */
+	sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
+			ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
+			ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
+			ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
+	/* By spec, host should re-start download from the first sector if sect_done is 0 */
+	if (sect_done == 0) {
+		if (retry > 0) {
+			retry--;
+			fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
+			goto do_retry;
 		}
+		fprintf(stderr, "Programming failed! Aborting...\n");
+		goto out;
 	}
 
 	if ((sect_done * sect_size) == fw_size) {

Best regards,
Bruno Matić

-----Original Message-----
From: Avri Altman <Avri.Altman@wdc.com> 
Sent: Thursday, July 7, 2022 2:00 PM
To: Matic, Bruno (Nokia - DE/Ulm) <bruno.matic@nokia.com>; linux-mmc@vger.kernel.org
Cc: Ulf Hansson <ulf.hansson@linaro.org>; Christian Löhle <CLoehle@hyperstone.com>
Subject: RE: [PATCH] mmc-utils: correct and clean up the file handling

> Hi everyone,
> 
> If I may ask if there is any update on the state of the merge since I 
> can't see anything in the git log.
I am following this - Ulf didn't pick it up yet.
Will ping you once he will.

Thanks,
Avri

> 
> Best regards,
> Bruno Matić
> 
> -----Original Message-----
> From: Matic, Bruno (Nokia - DE/Ulm)
> Sent: Wednesday, June 29, 2022 9:33 AM
> To: Avri Altman <Avri.Altman@wdc.com>; linux-mmc@vger.kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>; Christian Löhle 
> <CLoehle@hyperstone.com>
> Subject: RE: [PATCH] mmc-utils: correct and clean up the file handling
> 
> Hi Avri,
> 
> That is ok, I will wait.
> 
> Best regards,
> Bruno Matić
> 
> -----Original Message-----
> From: Avri Altman <Avri.Altman@wdc.com>
> Sent: Tuesday, June 28, 2022 11:56 PM
> To: Matic, Bruno (Nokia - DE/Ulm) <bruno.matic@nokia.com>; linux- 
> mmc@vger.kernel.org
> Cc: Ulf Hansson <ulf.hansson@linaro.org>; Christian Löhle 
> <CLoehle@hyperstone.com>
> Subject: RE: [PATCH] mmc-utils: correct and clean up the file handling
> 
> Hi Bruno,
> Thank you for your patch.
> 
> > Add the check if the whole firmware was loaded.
> > Cleaned up the leftovers of handling the file in chunks.
> >
> > Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
> Christian proposed a fix to do_ffu about a week ago, see e.g.
> https://www.spinics.net/lists/linux-mmc/msg70961.html.
> 
> Would you mind waiting for few more days to allow it to finalize, And 
> then rebase your change and resend?
> 
> Thanks,
> Avri
> 
> > ---
> >  mmc_cmds.c | 69
> > +++++++++++++++++++++++++++---------------------------
> >  1 file changed, 34 insertions(+), 35 deletions(-)
> >
> > diff --git a/mmc_cmds.c b/mmc_cmds.c index 70480df..e64c747 100644
> > --- a/mmc_cmds.c
> > +++ b/mmc_cmds.c
> > @@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
> >         __u8 *buf = NULL;
> >         __u32 arg;
> >         off_t fw_size;
> > -       ssize_t chunk_size;
> >         char *device;
> >         struct mmc_ioc_multi_cmd *multi_cmd = NULL;
> >         __u32 blocks = 1;
> > @@ -2925,45 +2924,45 @@ int do_ffu(int nargs, char **argv)
> >         multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | 
> > MMC_CMD_AC;
> >         multi_cmd->cmds[3].write_flag = 1;
> >
> > -do_retry:
> > -       /* read firmware chunk */
> > +       /* read firmware */
> >         lseek(img_fd, 0, SEEK_SET);
> > -       chunk_size = read(img_fd, buf, fw_size);
> > +       if (read(img_fd, buf, fw_size) != fw_size) {
> > +               fprintf(stderr, "Could not read the whole firmware file\n");
> > +               ret = -ENOSPC;
> > +               goto out;
> > +       }
> >
> > -       if (chunk_size > 0) {
> > -               /* send ioctl with multi-cmd */
> > -               ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> > +do_retry:
> > +       /* send ioctl with multi-cmd */
> > +       ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> >
> > -               if (ret) {
> > -                       perror("Multi-cmd ioctl");
> > -                       /* In case multi-cmd ioctl failed before exiting from ffu mode
> */
> > -                       ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> > -                       goto out;
> > -               }
> > +       if (ret) {
> > +               perror("Multi-cmd ioctl");
> > +               /* 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;
> > -               }
> > +       ret = read_extcsd(dev_fd, ext_csd);
> > +       if (ret) {
> > +               fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> > +               goto out;
> > +       }
> >
> > -               /* Test if we need to restart the download */
> > -               sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> > -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> > -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> > -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> > -               /* By spec, host should re-start download from the first sector if
> > sect_done is 0 */
> > -               if (sect_done == 0) {
> > -                       if (retry > 0) {
> > -                               retry--;
> > -                               fprintf(stderr, "Programming failed. Retrying... (%d)\n",
> > retry);
> > -                               goto do_retry;
> > -                       }
> > -                       fprintf(stderr, "Programming failed! Aborting...\n");
> > -                       goto out;
> > -               } else {
> > -                       fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done *
> > sect_size, (intmax_t)fw_size);
> > +       /* Test if we need to restart the download */
> > +       sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> > +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> > +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> > +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> > +       /* By spec, host should re-start download from the first 
> > + sector if
> > sect_done is 0 */
> > +       if (sect_done == 0) {
> > +               if (retry > 0) {
> > +                       retry--;
> > +                       fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
> > +                       goto do_retry;
> >                 }
> > +               fprintf(stderr, "Programming failed! Aborting...\n");
> > +               goto out;
> >         }
> >
> >         if ((sect_done * sect_size) == fw_size) {
Avri Altman July 20, 2022, 8:04 a.m. UTC | #7
Bruni hi,
Thank you for your patch.

> Hi everyone,
> 
> Here is the rebased patch.
> 
> Add the check if the whole firmware was loaded.
> Cleaned up the leftovers of handling the file in chunks.
> 
> Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
> 
> ---
>  mmc_cmds.c | 67 +++++++++++++++++++++++++++---------------------------
>  1 file changed, 33 insertions(+), 34 deletions(-)
> 
> diff --git a/mmc_cmds.c b/mmc_cmds.c
> index 8d7910e..d017b64 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
>         __u8 *buf = NULL;
>         __u32 arg;
>         off_t fw_size;
> -       ssize_t chunk_size;
>         char *device;
>         struct mmc_ioc_multi_cmd *multi_cmd = NULL;
> 
> @@ -2926,45 +2925,45 @@ int do_ffu(int nargs, char **argv)
>         multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B |
> MMC_CMD_AC;
>         multi_cmd->cmds[3].write_flag = 1;
> 
> -do_retry:
> -       /* read firmware chunk */
> +       /* read firmware */
>         lseek(img_fd, 0, SEEK_SET);
> -       chunk_size = read(img_fd, buf, fw_size);
> +       if (read(img_fd, buf, fw_size) != fw_size) {
> +               fprintf(stderr, "Could not read the whole firmware file\n");
> +               ret = -ENOSPC;
No space left?

> +               goto out;
> +       }
> 
> -       if (chunk_size > 0) {
> -               /* send ioctl with multi-cmd */
> -               ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> +do_retry:
> +       /* send ioctl with multi-cmd */
> +       ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> 
> -               if (ret) {
> -                       perror("Multi-cmd ioctl");
> -                       /* In case multi-cmd ioctl failed before exiting from ffu mode */
> -                       ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> -                       goto out;
> -               }
> +       if (ret) {
> +               perror("Multi-cmd ioctl");
> +               /* In case multi-cmd ioctl failed before exiting from ffu mode */
> +               ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> +               goto out;
> +       }
Why do we need this hack?
Why would the last command is prone to fail?
If there is no good reason - Lets remove this hack - as a 2nd patch in this series.

> 
> -               ret = read_extcsd(dev_fd, ext_csd);
> -               if (ret) {
> -                       fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> -                       goto out;
> -               }
> +       ret = read_extcsd(dev_fd, ext_csd);
> +       if (ret) {
> +               fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> +               goto out;
> +       }
> 
> -               /* Test if we need to restart the download */
> -               sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> -               /* By spec, host should re-start download from the first sector if
> sect_done is 0 */
> -               if (sect_done == 0) {
> -                       if (retry > 0) {
> -                               retry--;
> -                               fprintf(stderr, "Programming failed. Retrying... (%d)\n",
> retry);
> -                               goto do_retry;
> -                       }
> -                       fprintf(stderr, "Programming failed! Aborting...\n");
> -                       goto out;
> -               } else {
> -                       fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done *
> sect_size, (intmax_t)fw_size);
> +       /* Test if we need to restart the download */
> +       sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> +       /* By spec, host should re-start download from the first sector if
> sect_done is 0 */
Can we use here be32toh or get_unaligned_be32 or something instead?

> +       if (sect_done == 0) {
> +               if (retry > 0) {
If (retry--)

> +                       retry--;
> +                       fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
> +                       goto do_retry;
>                 }
> +               fprintf(stderr, "Programming failed! Aborting...\n");
> +               goto out;
>         }
> 
>         if ((sect_done * sect_size) == fw_size) {
> 
> Best regards,
> Bruno Matić

Thanks,
Avri
Bruno Matic Aug. 15, 2022, 9:51 a.m. UTC | #8
Hi everyone,

Comments are in-line bellow.

>Bruni hi,
>Thank you for your patch.
>
>> Hi everyone,
>> 
>> Here is the rebased patch.
>> 
>> Add the check if the whole firmware was loaded.
>> Cleaned up the leftovers of handling the file in chunks.
>> 
>> Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
>> 
>> ---
>>  mmc_cmds.c | 67 
>> +++++++++++++++++++++++++++---------------------------
>>  1 file changed, 33 insertions(+), 34 deletions(-)
>> 
>> diff --git a/mmc_cmds.c b/mmc_cmds.c
>> index 8d7910e..d017b64 100644
>> --- a/mmc_cmds.c
>> +++ b/mmc_cmds.c
>> @@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
>>         __u8 *buf = NULL;
>>         __u32 arg;
>>         off_t fw_size;
>> -       ssize_t chunk_size;
>>         char *device;
>>         struct mmc_ioc_multi_cmd *multi_cmd = NULL;
>> 
>> @@ -2926,45 +2925,45 @@ int do_ffu(int nargs, char **argv)
>>         multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | 
>> MMC_CMD_AC;
>>         multi_cmd->cmds[3].write_flag = 1;
>> 
>> -do_retry:
>> -       /* read firmware chunk */
>> +       /* read firmware */
>>         lseek(img_fd, 0, SEEK_SET);
>> -       chunk_size = read(img_fd, buf, fw_size);
>> +       if (read(img_fd, buf, fw_size) != fw_size) {
>> +               fprintf(stderr, "Could not read the whole firmware file\n");
>> +               ret = -ENOSPC;
>No space left?
>
Here I would propose to use perror instead of fprintf - something like:
	perror("Could not read the firmware file: ");
This will also propagate the errno from read.

>> +               goto out;
>> +       }
>> 
>> -       if (chunk_size > 0) {
>> -               /* send ioctl with multi-cmd */
>> -               ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
>> +do_retry:
>> +       /* send ioctl with multi-cmd */
>> +       ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
>> 
>> -               if (ret) {
>> -                       perror("Multi-cmd ioctl");
>> -                       /* In case multi-cmd ioctl failed before exiting from ffu mode */
>> -                       ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
>> -                       goto out;
>> -               }
>> +       if (ret) {
>> +               perror("Multi-cmd ioctl");
>> +               /* In case multi-cmd ioctl failed before exiting from ffu mode */
>> +               ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
>> +               goto out;
>> +       }
>Why do we need this hack?
>Why would the last command is prone to fail?
>If there is no good reason - Lets remove this hack - as a 2nd patch in this series.
If I assume correctly you refer to repetition of third command after a failure.
This was left as-is since I understood that first and second command can fail, but the device
should not remain in upgrade mode in case of failure.

>
>> 
>> -               ret = read_extcsd(dev_fd, ext_csd);
>> -               if (ret) {
>> -                       fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
>> -                       goto out;
>> -               }
>> +       ret = read_extcsd(dev_fd, ext_csd);
>> +       if (ret) {
>> +               fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
>> +               goto out;
>> +       }
>> 
>> -               /* Test if we need to restart the download */
>> -               sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
>> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
>> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
>> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
>> -               /* By spec, host should re-start download from the first sector if
>> sect_done is 0 */
>> -               if (sect_done == 0) {
>> -                       if (retry > 0) {
>> -                               retry--;
>> -                               fprintf(stderr, "Programming failed. Retrying... (%d)\n",
>> retry);
>> -                               goto do_retry;
>> -                       }
>> -                       fprintf(stderr, "Programming failed! Aborting...\n");
>> -                       goto out;
>> -               } else {
>> -                       fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done *
>> sect_size, (intmax_t)fw_size);
>> +       /* Test if we need to restart the download */
>> +       sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
>> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
>> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
>> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
>> +       /* By spec, host should re-start download from the first 
>> + sector if
>> sect_done is 0 */
>Can we use here be32toh or get_unaligned_be32 or something instead?
Tried to look into it - none of the functions fit the need, input must be an int.
Best I can offer is to write a macro - something along the lines:
	#define le32toh_array(p) (p | *(&p+1) << 8 | *(&p+2) << 16 | *(&p+3) << 24 )
	sect_done = le32toh_array(ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0]);

>
>> +       if (sect_done == 0) {
>> +               if (retry > 0) {
>If (retry--)
Agreed - will be done in v2.

>
>> +                       retry--;
>> +                       fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
>> +                       goto do_retry;
>>                 }
>> +               fprintf(stderr, "Programming failed! Aborting...\n");
>> +               goto out;
>>         }
>> 
>>         if ((sect_done * sect_size) == fw_size) {
>> 
>> Best regards,
>> Bruno Matić
>
>Thanks,
>Avri
>
Avri Altman Aug. 15, 2022, 12:11 p.m. UTC | #9
> Hi everyone,
> 
> Comments are in-line bellow.
> 
> >Bruni hi,
> >Thank you for your patch.
> >
> >> Hi everyone,
> >>
> >> Here is the rebased patch.
> >>
> >> Add the check if the whole firmware was loaded.
> >> Cleaned up the leftovers of handling the file in chunks.
> >>
> >> Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
> >>
> >> ---
> >>  mmc_cmds.c | 67
> >> +++++++++++++++++++++++++++---------------------------
> >>  1 file changed, 33 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/mmc_cmds.c b/mmc_cmds.c
> >> index 8d7910e..d017b64 100644
> >> --- a/mmc_cmds.c
> >> +++ b/mmc_cmds.c
> >> @@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
> >>         __u8 *buf = NULL;
> >>         __u32 arg;
> >>         off_t fw_size;
> >> -       ssize_t chunk_size;
> >>         char *device;
> >>         struct mmc_ioc_multi_cmd *multi_cmd = NULL;
> >>
> >> @@ -2926,45 +2925,45 @@ int do_ffu(int nargs, char **argv)
> >>         multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B |
> >> MMC_CMD_AC;
> >>         multi_cmd->cmds[3].write_flag = 1;
> >>
> >> -do_retry:
> >> -       /* read firmware chunk */
> >> +       /* read firmware */
> >>         lseek(img_fd, 0, SEEK_SET);
> >> -       chunk_size = read(img_fd, buf, fw_size);
> >> +       if (read(img_fd, buf, fw_size) != fw_size) {
> >> +               fprintf(stderr, "Could not read the whole firmware file\n");
> >> +               ret = -ENOSPC;
> >No space left?
> >
> Here I would propose to use perror instead of fprintf - something like:
>         perror("Could not read the firmware file: ");
> This will also propagate the errno from read.
Agreed.

> 
> >> +               goto out;
> >> +       }
> >>
> >> -       if (chunk_size > 0) {
> >> -               /* send ioctl with multi-cmd */
> >> -               ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> >> +do_retry:
> >> +       /* send ioctl with multi-cmd */
> >> +       ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> >>
> >> -               if (ret) {
> >> -                       perror("Multi-cmd ioctl");
> >> -                       /* In case multi-cmd ioctl failed before exiting from ffu
> mode */
> >> -                       ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> >> -                       goto out;
> >> -               }
> >> +       if (ret) {
> >> +               perror("Multi-cmd ioctl");
> >> +               /* In case multi-cmd ioctl failed before exiting from ffu mode */
> >> +               ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> >> +               goto out;
> >> +       }
> >Why do we need this hack?
> >Why would the last command is prone to fail?
> >If there is no good reason - Lets remove this hack - as a 2nd patch in this
> series.
> If I assume correctly you refer to repetition of third command after a failure.
> This was left as-is since I understood that first and second command can fail,
> but the device
> should not remain in upgrade mode in case of failure.
OK.

> 
> >
> >>
> >> -               ret = read_extcsd(dev_fd, ext_csd);
> >> -               if (ret) {
> >> -                       fprintf(stderr, "Could not read EXT_CSD from %s\n",
> device);
> >> -                       goto out;
> >> -               }
> >> +       ret = read_extcsd(dev_fd, ext_csd);
> >> +       if (ret) {
> >> +               fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> >> +               goto out;
> >> +       }
> >>
> >> -               /* Test if we need to restart the download */
> >> -               sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> >> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> >> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> >> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> >> -               /* By spec, host should re-start download from the first sector if
> >> sect_done is 0 */
> >> -               if (sect_done == 0) {
> >> -                       if (retry > 0) {
> >> -                               retry--;
> >> -                               fprintf(stderr, "Programming failed. Retrying... (%d)\n",
> >> retry);
> >> -                               goto do_retry;
> >> -                       }
> >> -                       fprintf(stderr, "Programming failed! Aborting...\n");
> >> -                       goto out;
> >> -               } else {
> >> -                       fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done *
> >> sect_size, (intmax_t)fw_size);
> >> +       /* Test if we need to restart the download */
> >> +       sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> >> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> >> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> >> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> >> +       /* By spec, host should re-start download from the first
> >> + sector if
> >> sect_done is 0 */
> >Can we use here be32toh or get_unaligned_be32 or something instead?
> Tried to look into it - none of the functions fit the need, input must be an
> int.
> Best I can offer is to write a macro - something along the lines:
>         #define le32toh_array(p) (p | *(&p+1) << 8 | *(&p+2) << 16 | *(&p+3) <<
> 24 )
>         sect_done =
> le32toh_array(ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0]);
Then better leave it as it is

Thanks,
Avri

> 
> >
> >> +       if (sect_done == 0) {
> >> +               if (retry > 0) {
> >If (retry--)
> Agreed - will be done in v2.
> 
> >
> >> +                       retry--;
> >> +                       fprintf(stderr, "Programming failed. Retrying... (%d)\n",
> retry);
> >> +                       goto do_retry;
> >>                 }
> >> +               fprintf(stderr, "Programming failed! Aborting...\n");
> >> +               goto out;
> >>         }
> >>
> >>         if ((sect_done * sect_size) == fw_size) {
> >>
> >> Best regards,
> >> Bruno Matić
> >
> >Thanks,
> >Avri
> >
Bruno Matic Aug. 15, 2022, 1:11 p.m. UTC | #10
Hi everyone,
As said, here is the reworked patch.


Add the check if the whole firmware was loaded.
Cleaned up the leftovers of handling the file in chunks.

Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
---
 mmc_cmds.c | 66 ++++++++++++++++++++++++++----------------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

diff --git a/mmc_cmds.c b/mmc_cmds.c
index 70480df..7d37e93 100644
--- a/mmc_cmds.c
+++ b/mmc_cmds.c
@@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
 	__u8 *buf = NULL;
 	__u32 arg;
 	off_t fw_size;
-	ssize_t chunk_size;
 	char *device;
 	struct mmc_ioc_multi_cmd *multi_cmd = NULL;
 	__u32 blocks = 1;
@@ -2925,45 +2924,44 @@ int do_ffu(int nargs, char **argv)
 	multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
 	multi_cmd->cmds[3].write_flag = 1;
 
-do_retry:
-	/* read firmware chunk */
+	/* read firmware */
 	lseek(img_fd, 0, SEEK_SET);
-	chunk_size = read(img_fd, buf, fw_size);
+	if (read(img_fd, buf, fw_size) != fw_size) {
+		perror("Could not read the firmware file: ");
+		ret = -ENOSPC;
+		goto out;
+	}
 
-	if (chunk_size > 0) {
-		/* send ioctl with multi-cmd */
-		ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
+do_retry:
+	/* send ioctl with multi-cmd */
+	ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
 
-		if (ret) {
-			perror("Multi-cmd ioctl");
-			/* In case multi-cmd ioctl failed before exiting from ffu mode */
-			ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
-			goto out;
-		}
+	if (ret) {
+		perror("Multi-cmd ioctl");
+		/* 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;
-		}
+	ret = read_extcsd(dev_fd, ext_csd);
+	if (ret) {
+		fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
+		goto out;
+	}
 
-		/* Test if we need to restart the download */
-		sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
-				ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
-				ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
-				ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
-		/* By spec, host should re-start download from the first sector if sect_done is 0 */
-		if (sect_done == 0) {
-			if (retry > 0) {
-				retry--;
-				fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
-				goto do_retry;
-			}
-			fprintf(stderr, "Programming failed! Aborting...\n");
-			goto out;
-		} else {
-			fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done * sect_size, (intmax_t)fw_size);
+	/* Test if we need to restart the download */
+	sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
+			ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
+			ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
+			ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
+	/* By spec, host should re-start download from the first sector if sect_done is 0 */
+	if (sect_done == 0) {
+		if (retry--) {
+			fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
+			goto do_retry;
 		}
+		fprintf(stderr, "Programming failed! Aborting...\n");
+		goto out;
 	}
 
 	if ((sect_done * sect_size) == fw_size) {
Avri Altman Aug. 15, 2022, 3:51 p.m. UTC | #11
> 
> Hi everyone,
> As said, here is the reworked patch.
> 
> 
> Add the check if the whole firmware was loaded.
> Cleaned up the leftovers of handling the file in chunks.
> 
> Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Please try in the future to properly mark your spins ( -v option of format-patch) - would be easier to follow.

Thanks,
Avri 

> ---
>  mmc_cmds.c | 66 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/mmc_cmds.c b/mmc_cmds.c
> index 70480df..7d37e93 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
>         __u8 *buf = NULL;
>         __u32 arg;
>         off_t fw_size;
> -       ssize_t chunk_size;
>         char *device;
>         struct mmc_ioc_multi_cmd *multi_cmd = NULL;
>         __u32 blocks = 1;
> @@ -2925,45 +2924,44 @@ int do_ffu(int nargs, char **argv)
>         multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B |
> MMC_CMD_AC;
>         multi_cmd->cmds[3].write_flag = 1;
> 
> -do_retry:
> -       /* read firmware chunk */
> +       /* read firmware */
>         lseek(img_fd, 0, SEEK_SET);
> -       chunk_size = read(img_fd, buf, fw_size);
> +       if (read(img_fd, buf, fw_size) != fw_size) {
> +               perror("Could not read the firmware file: ");
> +               ret = -ENOSPC;
> +               goto out;
> +       }
> 
> -       if (chunk_size > 0) {
> -               /* send ioctl with multi-cmd */
> -               ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> +do_retry:
> +       /* send ioctl with multi-cmd */
> +       ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> 
> -               if (ret) {
> -                       perror("Multi-cmd ioctl");
> -                       /* In case multi-cmd ioctl failed before exiting from ffu mode
> */
> -                       ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> -                       goto out;
> -               }
> +       if (ret) {
> +               perror("Multi-cmd ioctl");
> +               /* 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;
> -               }
> +       ret = read_extcsd(dev_fd, ext_csd);
> +       if (ret) {
> +               fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> +               goto out;
> +       }
> 
> -               /* Test if we need to restart the download */
> -               sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> -               /* By spec, host should re-start download from the first sector if
> sect_done is 0 */
> -               if (sect_done == 0) {
> -                       if (retry > 0) {
> -                               retry--;
> -                               fprintf(stderr, "Programming failed. Retrying... (%d)\n",
> retry);
> -                               goto do_retry;
> -                       }
> -                       fprintf(stderr, "Programming failed! Aborting...\n");
> -                       goto out;
> -               } else {
> -                       fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done *
> sect_size, (intmax_t)fw_size);
> +       /* Test if we need to restart the download */
> +       sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> +       /* By spec, host should re-start download from the first sector if
> sect_done is 0 */
> +       if (sect_done == 0) {
> +               if (retry--) {
> +                       fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
> +                       goto do_retry;
>                 }
> +               fprintf(stderr, "Programming failed! Aborting...\n");
> +               goto out;
>         }
> 
>         if ((sect_done * sect_size) == fw_size) {
> --
> 2.29.0
Bruno Matic Sept. 6, 2022, 7:57 a.m. UTC | #12
Will keep it in mind.

Best regards,
Bruno Matić

-----Original Message-----
From: Avri Altman <Avri.Altman@wdc.com> 
Sent: Monday, August 15, 2022 5:52 PM
To: Matic, Bruno (Nokia - DE/Ulm) <bruno.matic@nokia.com>; linux-mmc@vger.kernel.org
Cc: Ulf Hansson <ulf.hansson@linaro.org>; Christian Löhle <CLoehle@hyperstone.com>; Rossler, Jakob (Nokia - DE/Ulm) <jakob.rossler@nokia.com>; Heinonen, Aarne (Nokia - FI/Espoo) <aarne.heinonen@nokia.com>
Subject: RE: [PATCH] mmc-utils: correct and clean up the file handling

> 
> Hi everyone,
> As said, here is the reworked patch.
> 
> 
> Add the check if the whole firmware was loaded.
> Cleaned up the leftovers of handling the file in chunks.
> 
> Signed-off-by: Bruno Matic <bruno.matic@nokia.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Please try in the future to properly mark your spins ( -v option of format-patch) - would be easier to follow.

Thanks,
Avri 

> ---
>  mmc_cmds.c | 66 
> ++++++++++++++++++++++++++----------------------------
>  1 file changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/mmc_cmds.c b/mmc_cmds.c
> index 70480df..7d37e93 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
>         __u8 *buf = NULL;
>         __u32 arg;
>         off_t fw_size;
> -       ssize_t chunk_size;
>         char *device;
>         struct mmc_ioc_multi_cmd *multi_cmd = NULL;
>         __u32 blocks = 1;
> @@ -2925,45 +2924,44 @@ int do_ffu(int nargs, char **argv)
>         multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | 
> MMC_CMD_AC;
>         multi_cmd->cmds[3].write_flag = 1;
> 
> -do_retry:
> -       /* read firmware chunk */
> +       /* read firmware */
>         lseek(img_fd, 0, SEEK_SET);
> -       chunk_size = read(img_fd, buf, fw_size);
> +       if (read(img_fd, buf, fw_size) != fw_size) {
> +               perror("Could not read the firmware file: ");
> +               ret = -ENOSPC;
> +               goto out;
> +       }
> 
> -       if (chunk_size > 0) {
> -               /* send ioctl with multi-cmd */
> -               ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> +do_retry:
> +       /* send ioctl with multi-cmd */
> +       ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> 
> -               if (ret) {
> -                       perror("Multi-cmd ioctl");
> -                       /* In case multi-cmd ioctl failed before exiting from ffu mode
> */
> -                       ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> -                       goto out;
> -               }
> +       if (ret) {
> +               perror("Multi-cmd ioctl");
> +               /* 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;
> -               }
> +       ret = read_extcsd(dev_fd, ext_csd);
> +       if (ret) {
> +               fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> +               goto out;
> +       }
> 
> -               /* Test if we need to restart the download */
> -               sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> -               /* By spec, host should re-start download from the first sector if
> sect_done is 0 */
> -               if (sect_done == 0) {
> -                       if (retry > 0) {
> -                               retry--;
> -                               fprintf(stderr, "Programming failed. Retrying... (%d)\n",
> retry);
> -                               goto do_retry;
> -                       }
> -                       fprintf(stderr, "Programming failed! Aborting...\n");
> -                       goto out;
> -               } else {
> -                       fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done *
> sect_size, (intmax_t)fw_size);
> +       /* Test if we need to restart the download */
> +       sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> +       /* By spec, host should re-start download from the first 
> + sector if
> sect_done is 0 */
> +       if (sect_done == 0) {
> +               if (retry--) {
> +                       fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
> +                       goto do_retry;
>                 }
> +               fprintf(stderr, "Programming failed! Aborting...\n");
> +               goto out;
>         }
> 
>         if ((sect_done * sect_size) == fw_size) {
> --
> 2.29.0
Ulf Hansson Sept. 6, 2022, 8:29 a.m. UTC | #13
On Mon, 15 Aug 2022 at 15:11, Matic, Bruno (Nokia - DE/Ulm)
<bruno.matic@nokia.com> wrote:
>
> Hi everyone,
> As said, here is the reworked patch.
>
>
> Add the check if the whole firmware was loaded.
> Cleaned up the leftovers of handling the file in chunks.
>
> Signed-off-by: Bruno Matic <bruno.matic@nokia.com>

Hi Bruno,

Unfortunately, I was not able to apply this patch.

$subject patch was not accepted by the mmc patchwork [1], which I am
using to manage the patches. Please make sure to conform to the
process of submitting patches [2] and run scripts/checkpatch.pl before
re-submitting.

Kind regards
Uffe

[1]
https://patchwork.kernel.org/project/linux-mmc/list/
[2]
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

> ---
>  mmc_cmds.c | 66 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 32 insertions(+), 34 deletions(-)
>
> diff --git a/mmc_cmds.c b/mmc_cmds.c
> index 70480df..7d37e93 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -2812,7 +2812,6 @@ int do_ffu(int nargs, char **argv)
>         __u8 *buf = NULL;
>         __u32 arg;
>         off_t fw_size;
> -       ssize_t chunk_size;
>         char *device;
>         struct mmc_ioc_multi_cmd *multi_cmd = NULL;
>         __u32 blocks = 1;
> @@ -2925,45 +2924,44 @@ int do_ffu(int nargs, char **argv)
>         multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>         multi_cmd->cmds[3].write_flag = 1;
>
> -do_retry:
> -       /* read firmware chunk */
> +       /* read firmware */
>         lseek(img_fd, 0, SEEK_SET);
> -       chunk_size = read(img_fd, buf, fw_size);
> +       if (read(img_fd, buf, fw_size) != fw_size) {
> +               perror("Could not read the firmware file: ");
> +               ret = -ENOSPC;
> +               goto out;
> +       }
>
> -       if (chunk_size > 0) {
> -               /* send ioctl with multi-cmd */
> -               ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
> +do_retry:
> +       /* send ioctl with multi-cmd */
> +       ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
>
> -               if (ret) {
> -                       perror("Multi-cmd ioctl");
> -                       /* In case multi-cmd ioctl failed before exiting from ffu mode */
> -                       ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
> -                       goto out;
> -               }
> +       if (ret) {
> +               perror("Multi-cmd ioctl");
> +               /* 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;
> -               }
> +       ret = read_extcsd(dev_fd, ext_csd);
> +       if (ret) {
> +               fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
> +               goto out;
> +       }
>
> -               /* Test if we need to restart the download */
> -               sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> -                               ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> -               /* By spec, host should re-start download from the first sector if sect_done is 0 */
> -               if (sect_done == 0) {
> -                       if (retry > 0) {
> -                               retry--;
> -                               fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
> -                               goto do_retry;
> -                       }
> -                       fprintf(stderr, "Programming failed! Aborting...\n");
> -                       goto out;
> -               } else {
> -                       fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done * sect_size, (intmax_t)fw_size);
> +       /* Test if we need to restart the download */
> +       sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
> +                       ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
> +       /* By spec, host should re-start download from the first sector if sect_done is 0 */
> +       if (sect_done == 0) {
> +               if (retry--) {
> +                       fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
> +                       goto do_retry;
>                 }
> +               fprintf(stderr, "Programming failed! Aborting...\n");
> +               goto out;
>         }
>
>         if ((sect_done * sect_size) == fw_size) {
> --
> 2.29.0
>
diff mbox series

Patch

diff --git a/mmc_cmds.c b/mmc_cmds.c
index 70480df..e64c747 100644
--- a/mmc_cmds.c
+++ b/mmc_cmds.c
@@ -2812,7 +2812,6 @@  int do_ffu(int nargs, char **argv)
 	__u8 *buf = NULL;
 	__u32 arg;
 	off_t fw_size;
-	ssize_t chunk_size;
 	char *device;
 	struct mmc_ioc_multi_cmd *multi_cmd = NULL;
 	__u32 blocks = 1;
@@ -2925,45 +2924,45 @@  int do_ffu(int nargs, char **argv)
 	multi_cmd->cmds[3].flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
 	multi_cmd->cmds[3].write_flag = 1;
 
-do_retry:
-	/* read firmware chunk */
+	/* read firmware */
 	lseek(img_fd, 0, SEEK_SET);
-	chunk_size = read(img_fd, buf, fw_size);
+	if (read(img_fd, buf, fw_size) != fw_size) {
+		fprintf(stderr, "Could not read the whole firmware file\n");
+		ret = -ENOSPC;
+		goto out;
+	}
 
-	if (chunk_size > 0) {
-		/* send ioctl with multi-cmd */
-		ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
+do_retry:
+	/* send ioctl with multi-cmd */
+	ret = ioctl(dev_fd, MMC_IOC_MULTI_CMD, multi_cmd);
 
-		if (ret) {
-			perror("Multi-cmd ioctl");
-			/* In case multi-cmd ioctl failed before exiting from ffu mode */
-			ioctl(dev_fd, MMC_IOC_CMD, &multi_cmd->cmds[3]);
-			goto out;
-		}
+	if (ret) {
+		perror("Multi-cmd ioctl");
+		/* 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;
-		}
+	ret = read_extcsd(dev_fd, ext_csd);
+	if (ret) {
+		fprintf(stderr, "Could not read EXT_CSD from %s\n", device);
+		goto out;
+	}
 
-		/* Test if we need to restart the download */
-		sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
-				ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
-				ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
-				ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
-		/* By spec, host should re-start download from the first sector if sect_done is 0 */
-		if (sect_done == 0) {
-			if (retry > 0) {
-				retry--;
-				fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
-				goto do_retry;
-			}
-			fprintf(stderr, "Programming failed! Aborting...\n");
-			goto out;
-		} else {
-			fprintf(stderr, "Programmed %d/%jd bytes\r", sect_done * sect_size, (intmax_t)fw_size);
+	/* Test if we need to restart the download */
+	sect_done = ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0] |
+			ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_1] << 8 |
+			ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_2] << 16 |
+			ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_3] << 24;
+	/* By spec, host should re-start download from the first sector if sect_done is 0 */
+	if (sect_done == 0) {
+		if (retry > 0) {
+			retry--;
+			fprintf(stderr, "Programming failed. Retrying... (%d)\n", retry);
+			goto do_retry;
 		}
+		fprintf(stderr, "Programming failed! Aborting...\n");
+		goto out;
 	}
 
 	if ((sect_done * sect_size) == fw_size) {