Re: [PATCH v3 01/19] bootstd: Move bootflow-adding to bootstd

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 7415 bytes --]

On Tue, Nov 05, 2024 at 08:13:04AM -0700, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 4 Nov 2024 at 15:15, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 11/4/24 18:50, Simon Glass wrote:
> > > This relates to more than just the bootdev, since there is a global list
> > > of bootflows. Move the function to the bootstd file and rename it.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >   boot/bootdev-uclass.c | 25 -------------------------
> > >   boot/bootstd-uclass.c | 25 +++++++++++++++++++++++++
> > >   cmd/bootflow.c        |  2 +-
> > >   include/bootdev.h     | 15 ---------------
> > >   include/bootstd.h     | 17 +++++++++++++++++
> > >   5 files changed, 43 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c
> > > index 64ec4fde493..eddbf60600c 100644
> > > --- a/boot/bootdev-uclass.c
> > > +++ b/boot/bootdev-uclass.c
> > > @@ -32,31 +32,6 @@ enum {
> > >       BOOT_TARGETS_MAX_LEN    = 100,
> > >   };
> > >
> > > -int bootdev_add_bootflow(struct bootflow *bflow)
> > > -{
> > > -     struct bootstd_priv *std;
> > > -     struct bootflow *new;
> > > -     int ret;
> > > -
> > > -     ret = bootstd_get_priv(&std);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > > -     new = malloc(sizeof(*bflow));
> > > -     if (!new)
> > > -             return log_msg_ret("bflow", -ENOMEM);
> > > -     memcpy(new, bflow, sizeof(*bflow));
> > > -
> > > -     list_add_tail(&new->glob_node, &std->glob_head);
> > > -     if (bflow->dev) {
> > > -             struct bootdev_uc_plat *ucp = dev_get_uclass_plat(bflow->dev);
> > > -
> > > -             list_add_tail(&new->bm_node, &ucp->bootflow_head);
> > > -     }
> > > -
> > > -     return 0;
> > > -}
> > > -
> > >   int bootdev_first_bootflow(struct udevice *dev, struct bootflow **bflowp)
> > >   {
> > >       struct bootdev_uc_plat *ucp = dev_get_uclass_plat(dev);
> > > diff --git a/boot/bootstd-uclass.c b/boot/bootstd-uclass.c
> > > index fdb8d69e320..bf6e49ad97a 100644
> > > --- a/boot/bootstd-uclass.c
> > > +++ b/boot/bootstd-uclass.c
> > > @@ -61,6 +61,31 @@ void bootstd_clear_glob(void)
> > >       bootstd_clear_glob_(std);
> > >   }
> > >
> > > +int bootstd_add_bootflow(struct bootflow *bflow)
> > > +{
> > > +     struct bootstd_priv *std;
> > > +     struct bootflow *new;
> > > +     int ret;
> > > +
> > > +     ret = bootstd_get_priv(&std);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     new = malloc(sizeof(*bflow));
> > > +     if (!new)
> > > +             return log_msg_ret("bflow", -ENOMEM);
> > > +     memcpy(new, bflow, sizeof(*bflow));
> > > +
> > > +     list_add_tail(&new->glob_node, &std->glob_head);
> > > +     if (bflow->dev) {
> > > +             struct bootdev_uc_plat *ucp = dev_get_uclass_plat(bflow->dev);
> > > +
> > > +             list_add_tail(&new->bm_node, &ucp->bootflow_head);
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >   static int bootstd_remove(struct udevice *dev)
> > >   {
> > >       struct bootstd_priv *priv = dev_get_priv(dev);
> > > diff --git a/cmd/bootflow.c b/cmd/bootflow.c
> > > index f67948d7368..8962464bbf8 100644
> > > --- a/cmd/bootflow.c
> > > +++ b/cmd/bootflow.c
> > > @@ -207,7 +207,7 @@ static int do_bootflow_scan(struct cmd_tbl *cmdtp, int flag, int argc,
> > >               bflow.err = ret;
> > >               if (!ret)
> > >                       num_valid++;
> > > -             ret = bootdev_add_bootflow(&bflow);
> > > +             ret = bootstd_add_bootflow(&bflow);
> > >               if (ret) {
> > >                       printf("Out of memory\n");
> > >                       return CMD_RET_FAILURE;
> > > diff --git a/include/bootdev.h b/include/bootdev.h
> > > index ad4af0d1310..8db198dd56b 100644
> > > --- a/include/bootdev.h
> > > +++ b/include/bootdev.h
> > > @@ -195,21 +195,6 @@ void bootdev_list(bool probe);
> > >    */
> > >   void bootdev_clear_bootflows(struct udevice *dev);
> > >
> > > -/**
> > > - * bootdev_add_bootflow() - Add a bootflow to the bootdev's list
> > > - *
> > > - * All fields in @bflow must be set up. Note that @bflow->dev is used to add the
> > > - * bootflow to that device.
> > > - *
> > > - * @dev: Bootdev device to add to
> > > - * @bflow: Bootflow to add. Note that fields within bflow must be allocated
> > > - *   since this function takes over ownership of these. This functions makes
> > > - *   a copy of @bflow itself (without allocating its fields again), so the
> > > - *   caller must dispose of the memory used by the @bflow pointer itself
> > > - * Return: 0 if OK, -ENOMEM if out of memory
> > > - */
> > > -int bootdev_add_bootflow(struct bootflow *bflow);
> > > -
> > >   /**
> > >    * bootdev_first_bootflow() - Get the first bootflow from a bootdev
> > >    *
> > > diff --git a/include/bootstd.h b/include/bootstd.h
> > > index ac756e98d84..3fc93a4ec2e 100644
> > > --- a/include/bootstd.h
> > > +++ b/include/bootstd.h
> > > @@ -105,4 +105,21 @@ void bootstd_clear_glob(void);
> > >    */
> > >   int bootstd_prog_boot(void);
> > >
> > > +/**
> > > + * bootstd_add_bootflow() - Add a bootflow to the bootdev's and global list
> > > + *
> > > + * All fields in @bflow must be set up. Note that @bflow->dev is used to add the
> > > + * bootflow to that device.
> > > + *
> > > + * The bootflow is also added to the global list of all bootflows
> > > + *
> > > + * @dev: Bootdev device to add to
> >
> > This parameter does not exist.
> >
> > > + * @bflow: Bootflow to add. Note that fields within bflow must be allocated
> > > + *   since this function takes over ownership of these. This functions makes
> > > + *   a copy of @bflow itself (without allocating its fields again), so the
> > > + *   caller must dispose of the memory used by the @bflow pointer itself
> >
> > Please, add the headers to the API documentation and fix the numerous
> > documentation issues like:
> >
> > ./include/bootdev.h:57: warning: Enum value 'BOOTDEVP_0_NONE' not
> > described in enum 'bootdev_prio_t'
> > ./include/bootdev.h:57: warning: Enum value 'BOOTDEVP_1_PRE_SCAN' not
> > described in enum 'bootdev_prio_t'
> > ./include/bootdev.h:57: warning: Enum value 'BOOTDEVP_COUNT' not
> > described in enum 'bootdev_prio_t'
> > ./include/bootdev.h:57: warning: Excess enum value 'BOOTDEVP_6_PRE_SCAN'
> > description in 'bootdev_prio_t'
> > ./include/bootdev.h:118: warning: Function parameter or member
> > 'bootflow_head' not described in 'bootdev_uc_plat'
> > ./include/bootdev.h:118: warning: Function parameter or member 'prio'
> > not described in 'bootdev_uc_plat'
> > ./include/bootdev.h:412: warning: Function parameter or member 'blk' not
> > described in 'bootdev_setup_for_sibling_blk'
> > ./include/bootdev.h:412: warning: Excess function parameter 'parent'
> > description in 'bootdev_setup_for_sibling_blk'
> 
> I don't mind doing that in a follow-up once this series is in, but it
> is out-of-scope here.

I think that's flipping the priorities. Documenting what all of this is
supposed to be doing will help explain to the rest of us what you're
intending to do and why it's a good idea and so forth.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]