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.
>> + 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
>
