> 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@xxxxxxxxx>
> >>
> >> ---
> >> 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
> >
