| Message ID | AM5PR0701MB2964A49C4E5EEA8905926120EFB89@AM5PR0701MB2964.eurprd07.prod.outlook.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mmc-utils: correct and clean up the file handling | expand |
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) {
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) {
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) {
> 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) {
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
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) {
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
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 >
> 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 > >
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) {
> > 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
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
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 --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) {
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(-)