> 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?
I see that you still think that the ffu_mode is better.
I don't want to block the progress of the series - as I am aware that there are customers that are waiting.
Please feel free to add my Acked-by tag to the whole series.
Thanks,
Avri
>
>
> Kind regards,
> Bean
