On Mon, 2024-10-21 at 07:39 +0000, Avri Altman wrote:
> > > > + return -ENOMEM;
> > > > + }
> > > I was expecting that do_ffu_download will be called with struct
> > > mmc_ioc_multi_cmd *multi_cmd argument as well.
> > > That each ffu<x> mode fills it according to its own logic.
> > > This you won't be needing that ffu_mode additional parameter.
> >
> > I wanted to clarify why the ffu_mode parameter is necessary during
> > the
> > download phase. By extracting the logic into a common approach and
> > using
> > ffu_mode to dynamically update the multi_cmd structure across
> > iterations
> > per ffu-mode, I can handle the variations between different FFU
> > modes more
> > effectively. This allows me to extract more common code and avoid
> > duplication, as the ffu_mode helps determine which specific updates
> > or
> > adjustments need to be made to the multi_cmd at each stage.
> >
> > Without this, it would be difficult to manage multiple loops or
> > iterations of
> > the download process, especially when the command structure needs
> > to be
> > modified in different ways depending on the mode.
> > The use of ffu_mode centralizes this control, making the code
> > cleaner and
> > more maintainable.
> I see your point.
> Each mode is packing differently the multi-ioctl, and you need to
> update different offset of the cmds part.
> So how about instead of ffu mode, adjust set_ffu_single_cmd to do
> just that.
>
> And maybe in few preparation phases - to make the review process more
> concise:
> First let's make it get the cmds part of the multi-ioctl instead of
> the multi-ioctl:
> +static void set_ffu_write_cmd(struct mmc_ioc_cmd *cmds, __u8
> *ext_csd, unsigned int bytes,
> + __u8 *buf, off_t offset)
> {
> __u32 arg = per_byte_htole32(&ext_csd[EXT_CSD_FFU_ARG_0]);
>
> /* send block count */
> - set_single_cmd(&multi_cmd->cmds[1], MMC_SET_BLOCK_COUNT, 0,
> 0,
> - bytes / 512);
> - multi_cmd->cmds[1].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
> MMC_CMD_AC;
> + set_single_cmd(&cmds[0], MMC_SET_BLOCK_COUNT, 0, 0, bytes /
> 512);
> + cmds[0].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
>
> /*
> * send image chunk: blksz and blocks essentially do not
> matter, as
> * long as the product is fw_size, but some hosts don't
> handle larger
> * blksz well.
> */
> - set_single_cmd(&multi_cmd->cmds[2], MMC_WRITE_MULTIPLE_BLOCK,
> 1,
> - bytes / 512, arg);
> - mmc_ioc_cmd_set_data(multi_cmd->cmds[2], buf + offset);
> + set_single_cmd(&cmds[1], MMC_WRITE_MULTIPLE_BLOCK, 1, bytes /
> 512, arg);
> + mmc_ioc_cmd_set_data(cmds[1], buf + offset);
> +}
>
> And then, allow it to carry sbc or not:
> +static void set_ffu_sbc_cmd(struct mmc_ioc_cmd *cmds, int bytes)
> {
> - __u32 arg = per_byte_htole32(&ext_csd[EXT_CSD_FFU_ARG_0]);
> -
> /* send block count */
> - set_single_cmd(&multi_cmd->cmds[1], MMC_SET_BLOCK_COUNT, 0,
> 0,
> - bytes / 512);
> - multi_cmd->cmds[1].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
> MMC_CMD_AC;
> + set_single_cmd(&cmds[0], MMC_SET_BLOCK_COUNT, 0, 0, bytes /
> 512);
> + cmds[0].flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
> +}
>
> +static void set_ffu_write_cmd(struct mmc_ioc_cmd *cmds, __u8
> *ext_csd, unsigned int bytes,
> + __u8 *buf, off_t offset, bool sbc)
> +{
> + __u32 arg = per_byte_htole32(&ext_csd[EXT_CSD_FFU_ARG_0]);
> +
> + if (sbc)
> + set_ffu_sbc_cmd(cmds, bytes);
> /*
> * send image chunk: blksz and blocks essentially do not
> matter, as
> * long as the product is fw_size, but some hosts don't
> handle larger
> * blksz well.
> */
> - set_single_cmd(&multi_cmd->cmds[2], MMC_WRITE_MULTIPLE_BLOCK,
> 1,
> - bytes / 512, arg);
> - mmc_ioc_cmd_set_data(multi_cmd->cmds[2], buf + offset);
> + set_single_cmd(&cmds[1], MMC_WRITE_MULTIPLE_BLOCK, 1, bytes /
> 512, arg);
> + mmc_ioc_cmd_set_data(cmds[1], buf + offset);
> +}
> +
>
> And only then, as a 3rd preparation phase move the do_retry out.
> And you need to tell it what offset the write commands are. I guess
> this is the equivalent of your ffu_mode.
> But the difference is that when I look into do_ffu2, do_ffu3 etc. -
> The logic of pointing to the write command is there.
>
> +static int __do_ffu(int dev_fd, char *device, struct
> mmc_ioc_multi_cmd *multi_cmd, int cmd_off,
> + off_t fw_size, unsigned int sect_size, unsigned
> int default_chunk,
> + __u8 *ext_csd, __u8 *buf)
> Maybe we can pack all the ffu arguments into a single control struct
> to get passed to __do_ffu as struct ffu_params *.
>
> +{
> + off_t bytes_left, off;
> + __u32 sect_done = 0;
> + __u8 ffu_feature = ext_csd[EXT_CSD_FFU_FEATURES];
> + int write_command_offset = cmd_off;
> + int retry = 3;
> + int ret;
> +
> +do_retry:
> + bytes_left = fw_size;
> + off = 0;
> + while (bytes_left) {
> + unsigned int chunk_size = bytes_left < default_chunk
> ?
> + bytes_left : default_chunk;
> +
> + /* prepare multi_cmd for FFU based on cmd to be used
> */
> + set_ffu_write_cmd(&multi_cmd-
> >cmds[write_command_offset],
> + ext_csd, chunk_size, buf,
> off, 1);
> +
> + /* 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]);
> + return ret;
> + }
> +
> + bytes_left -= chunk_size;
> + off += chunk_size;
> + }
> +
> + /*
> + * By spec - check if mode operation codes are supported in
> ffu features,
> + * if not then skip checking number of sectors programmed
> after install
> + */
> + if (!ffu_feature) {
> + fprintf(stderr, "Please reboot to complete firmware
> installation on %s\n", device);
> + return 0;
> + }
> +
> + ret = read_extcsd(dev_fd, ext_csd);
> + if (ret) {
> + fprintf(stderr, "Could not read EXT_CSD from %s\n",
> device);
> + return ret;
> + }
> +
> + /* Test if we need to restart the download */
> + sect_done =
> per_byte_htole32(&ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG_0]);
> + /* 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");
> + return -EINVAL;
> + }
> +
> + fprintf(stderr, "Programmed %jd/%jd bytes\n",
> (intmax_t)sect_done * sect_size, (intmax_t)fw_size);
> +
> + if ((sect_done * sect_size) == fw_size) {
> + fprintf(stderr, "Programming finished
> successfully\n");
> + return 0;
> + }
> +
> + fprintf(stderr, "FW size and number of sectors written
> mismatch\n");
> + return -EINVAL;
> }
>
> Each mode can have its own arguments - it’s a different de-facto util
> so you are not bounded by do_ffu arguments.
> Thus the argument parsing is done for each mode differently.
> Also allocating the multi-ioctl is privet to each mode. Some have 4,
> some have 2, and some 1.
>
> Thanks,
> Avri
I can change your suggestions, before changing, I would like to have a
solid agreement,
The approach in my patch is to concentrate differences in
do_ffu_download() while keeping common functions outside it, the reason
for this becuase the main difference btw ffu_modes are FW budnle
download.
void _do_ffu(int ffu_mode) {
// Declare parameters specific to the FFU mode
do_ffu_prepare();
ffu_is_supported();
do_ffu_download(ffu_mode);
do_ffu_installation();
}
void do_ffu1() {
_do_ffu(FFU_MODE_1);
}
void do_ffu2() {
_do_ffu(FFU_MODE_2);
}
Advantages:
Reduced Duplication: Each do_ffu<x>() avoids repeating the same logic.
Improved Maintainability: Changes only need to be made in one location.
Clear Separation: Common operations remain consistent across all modes.
The approach you suggested is to sperate them at the begigning of
do_ffu<x>, then pass the different parameters to each different
function:
It risks unnecessary duplication and increased complexity, as seen in
this structure:
void do_ffu1() {
// Duplicate logic here
do_ffu_prepare(ffu1);
ffu_is_supported();
do_ffu_download(ffu_structure);
do_ffu_installation();
}
void do_ffu2() {
// Duplicate logic here
do_ffu_prepare(ffu2);
ffu_is_supported();
do_ffu_download(ffu_structure);
do_ffu_installation();
}
Please tell me was the second one what you expected?
Kind regards,
Bean
