[PATCH 2/2] cmd: bcb: extend BCB C API to allow read/write the fields
Dmitrii Merkurev
dimorinny at google.com
Fri Nov 10 07:06:41 CET 2023
Uploaded v2 with fixes
On Thu, Nov 9, 2023 at 2:05 AM Mattijs Korpershoek <
mkorpershoek at baylibre.com> wrote:
> Hi Dmitrii,
>
> Thank you for your patch.
>
> On jeu., nov. 09, 2023 at 00:36, Dmitrii Merkurev <dimorinny at google.com>
> wrote:
>
> > Currently BCB C API only allows to modify 'command' BCB field.
> > Extend it so that we can also read and modify all the available
> > BCB fields (command, status, recovery, stage).
> >
> > Co-developed-by: Cody Schuffelen <schuffelen at google.com>
> > Signed-off-by: Cody Schuffelen <schuffelen at google.com>
> > Signed-off-by: Dmitrii Merkurev <dimorinny at google.com>
> > Cc: Eugeniu Rosca <erosca at de.adit-jv.com>
> > Cc: Ying-Chun Liu (PaulLiu) <paul.liu at linaro.org>
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Mattijs Korpershoek <mkorpershoek at baylibre.com>
> > Cc: Sean Anderson <sean.anderson at seco.com>
> > Cc: Cody Schuffelen <schuffelen at google.com>
>
> I could test this after applying the diffs from:
> https://lore.kernel.org/all/87a5rn9sdm.fsf@baylibre.com/
>
> I tested with:
> $ fastboot reboot bootloader
> => bcb load mmc 2 misc
> => bcb dump command
>
> I also tested
> => bcb set status hello
> => bcb dump status
>
> Tested-by: Mattijs Korpershoek <mkorpershoek at baylibre.com> # on vim3
>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
>
> Some small nitpicks below.
> The nitpick do not have to be adressed if you don't want to.
>
> > ---
> > cmd/bcb.c | 162 +++++++++++++++++++++++------------
> > drivers/fastboot/fb_common.c | 14 ++-
> > include/bcb.h | 60 ++++++++++++-
> > 3 files changed, 179 insertions(+), 57 deletions(-)
> >
> > diff --git a/cmd/bcb.c b/cmd/bcb.c
> > index 5d8c232663..7a77b7f7b0 100644
> > --- a/cmd/bcb.c
> > +++ b/cmd/bcb.c
> > @@ -25,10 +25,18 @@ enum bcb_cmd {
> > BCB_CMD_STORE,
> > };
> >
> > -static enum uclass_id bcb_uclass_id = UCLASS_INVALID;
> > -static int bcb_dev = -1;
> > -static int bcb_part = -1;
> > +static const char * const fields[] = {
> > + "command",
> > + "status",
> > + "recovery",
> > + "stage"
> > +};
> > +
> > static struct bootloader_message bcb __aligned(ARCH_DMA_MINALIGN) = { {
> 0 } };
> > +static struct disk_partition partition_data;
> > +
> > +static struct blk_desc *block;
> > +static struct disk_partition *partition = &partition_data;
> >
> > static int bcb_cmd_get(char *cmd)
> > {
> > @@ -82,7 +90,7 @@ static int bcb_is_misused(int argc, char *const argv[])
> > return -1;
> > }
> >
> > - if (cmd != BCB_CMD_LOAD && (bcb_dev < 0 || bcb_part < 0)) {
> > + if (cmd != BCB_CMD_LOAD && !block) {
> > printf("Error: Please, load BCB first!\n");
> > return -1;
> > }
> > @@ -94,7 +102,7 @@ err:
> > return -1;
> > }
> >
> > -static int bcb_field_get(char *name, char **fieldp, int *sizep)
> > +static int bcb_field_get(const char *name, char **fieldp, int *sizep)
> > {
> > if (!strcmp(name, "command")) {
> > *fieldp = bcb.command;
> > @@ -119,16 +127,21 @@ static int bcb_field_get(char *name, char
> **fieldp, int *sizep)
> > return 0;
> > }
> >
> > -static int __bcb_load(const char *iface, int devnum, const char *partp)
> > +static void __bcb_reset(void)
> > +{
> > + block = NULL;
> > + partition = &partition_data;
> > + memset(&partition_data, 0, sizeof(struct disk_partition));
> > + memset(&bcb, 0, sizeof(struct bootloader_message));
> > +}
> > +
> > +static int __bcb_initialize(const char *iface, int devnum, const char
> *partp)
> > {
> > - struct blk_desc *desc;
> > - struct disk_partition info;
> > - u64 cnt;
> > char *endp;
> > int part, ret;
> >
> > - desc = blk_get_dev(iface, devnum);
> > - if (!desc) {
> > + block = blk_get_dev(iface, devnum);
> > + if (!block) {
> > ret = -ENODEV;
> > goto err_read_fail;
> > }
> > @@ -137,7 +150,7 @@ static int __bcb_load(const char *iface, int devnum,
> const char *partp)
> > * always select the first hwpart in case another
> > * blk operation selected a different hwpart
> > */
> > - ret = blk_dselect_hwpart(desc, 0);
> > + ret = blk_dselect_hwpart(block, 0);
> > if (IS_ERR_VALUE(ret)) {
> > ret = -ENODEV;
> > goto err_read_fail;
> > @@ -145,49 +158,63 @@ static int __bcb_load(const char *iface, int
> devnum, const char *partp)
> >
> > part = simple_strtoul(partp, &endp, 0);
> > if (*endp == '\0') {
> > - ret = part_get_info(desc, part, &info);
> > + ret = part_get_info(block, part, partition);
> > if (ret)
> > goto err_read_fail;
> > } else {
> > - part = part_get_info_by_name(desc, partp, &info);
> > + part = part_get_info_by_name(block, partp, partition);
> > if (part < 0) {
> > ret = part;
> > goto err_read_fail;
> > }
> > }
> >
> > - cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz);
> > - if (cnt > info.size)
> > + return CMD_RET_SUCCESS;
> > +
> > +err_read_fail:
> > + printf("Error: %d %d:%s read failed (%d)\n", block->uclass_id,
> > + block->devnum, partition->name, ret);
> > + goto err;
>
> Nitpick: No need for this goto, we can just fall-through.
>
> > +err:
> > + __bcb_reset();
> > + return CMD_RET_FAILURE;
> > +}
> > +
> > +static int __bcb_load(void)
> > +{
> > + u64 cnt;
> > + int ret;
> > +
> > + cnt = DIV_ROUND_UP(sizeof(struct bootloader_message),
> partition->blksz);
> > + if (cnt > partition->size)
> > goto err_too_small;
> >
> > - if (blk_dread(desc, info.start, cnt, &bcb) != cnt) {
> > + if (blk_dread(block, partition->start, cnt, &bcb) != cnt) {
> > ret = -EIO;
> > goto err_read_fail;
> > }
> >
> > - bcb_uclass_id = desc->uclass_id;
> > - bcb_dev = desc->devnum;
> > - bcb_part = part;
> > - debug("%s: Loaded from %s %d:%d\n", __func__, iface, bcb_dev,
> bcb_part);
> > + debug("%s: Loaded from %d %d:%s\n", __func__, block->uclass_id,
> > + block->devnum, partition->name);
> >
> > return CMD_RET_SUCCESS;
> > err_read_fail:
> > - printf("Error: %s %d:%s read failed (%d)\n", iface, devnum, partp,
> ret);
> > + printf("Error: %d %d:%s read failed (%d)\n", block->uclass_id,
> > + block->devnum, partition->name, ret);
> > goto err;
> > err_too_small:
> > - printf("Error: %s %d:%s too small!", iface, devnum, partp);
> > + printf("Error: %d %d:%s too small!", block->uclass_id,
> > + block->devnum, partition->name);
> > goto err;
> > err:
> > - bcb_uclass_id = UCLASS_INVALID;
> > - bcb_dev = -1;
> > - bcb_part = -1;
> > -
> > + __bcb_reset();
>
> Nitpick: __bcb_reset() introduction could have been done in a separate
> patch
>
> > return CMD_RET_FAILURE;
> > }
> >
> > static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc,
> > char * const argv[])
> > {
> > + int ret;
> > int devnum;
> > char *endp;
> > char *iface = "mcc";
> > @@ -204,10 +231,14 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int
> flag, int argc,
> > return CMD_RET_FAILURE;
> > }
> >
> > - return __bcb_load(iface, devnum, argv[2]);
> > + ret = __bcb_initialize(iface, devnum, argv[2]);
> > + if (ret != CMD_RET_SUCCESS)
> > + return ret;
> > +
> > + return __bcb_load();
> > }
> >
> > -static int __bcb_set(char *fieldp, const char *valp)
> > +static int __bcb_set(const char *fieldp, const char *valp)
> > {
> > int size, len;
> > char *field, *str, *found, *tmp;
> > @@ -307,31 +338,20 @@ static int do_bcb_dump(struct cmd_tbl *cmdtp, int
> flag, int argc,
> >
> > static int __bcb_store(void)
> > {
> > - struct blk_desc *desc;
> > - struct disk_partition info;
> > u64 cnt;
> > int ret;
> >
> > - desc = blk_get_devnum_by_uclass_id(bcb_uclass_id, bcb_dev);
> > - if (!desc) {
> > - ret = -ENODEV;
> > - goto err;
> > - }
> > -
> > - ret = part_get_info(desc, bcb_part, &info);
> > - if (ret)
> > - goto err;
> > -
> > - cnt = DIV_ROUND_UP(sizeof(struct bootloader_message), info.blksz);
> > + cnt = DIV_ROUND_UP(sizeof(struct bootloader_message),
> partition->blksz);
> >
> > - if (blk_dwrite(desc, info.start, cnt, &bcb) != cnt) {
> > + if (blk_dwrite(block, partition->start, cnt, &bcb) != cnt) {
> > ret = -EIO;
> > goto err;
> > }
> >
> > return CMD_RET_SUCCESS;
> > err:
> > - printf("Error: %d %d:%d write failed (%d)\n", bcb_uclass_id,
> bcb_dev, bcb_part, ret);
> > + printf("Error: %d %d:%s write failed (%d)\n", block->uclass_id,
> > + block->devnum, partition->name, ret);
> >
> > return CMD_RET_FAILURE;
> > }
> > @@ -342,23 +362,59 @@ static int do_bcb_store(struct cmd_tbl *cmdtp, int
> flag, int argc,
> > return __bcb_store();
> > }
> >
> > -int bcb_write_reboot_reason(const char *iface, int devnum, char *partp,
> const char *reasonp)
> > +int bcb_find_partition_and_load(const char *iface, int devnum, char
> *partp)
> > {
> > int ret;
> >
> > - ret = __bcb_load(iface, devnum, partp);
> > - if (ret != CMD_RET_SUCCESS)
> > - return ret;
> > + __bcb_reset();
> >
> > - ret = __bcb_set("command", reasonp);
> > + ret = __bcb_initialize(iface, devnum, partp);
> > if (ret != CMD_RET_SUCCESS)
> > return ret;
> >
> > - ret = __bcb_store();
> > - if (ret != CMD_RET_SUCCESS)
> > - return ret;
> > + return __bcb_load();
> > +}
> >
> > - return 0;
> > +int bcb_load(struct blk_desc *block_description, struct disk_partition
> *disk_partition)
> > +{
> > + __bcb_reset();
> > +
> > + block = block_description;
> > + partition = disk_partition;
> > +
> > + return __bcb_load();
> > +}
> > +
> > +int bcb_set(enum bcb_field field, const char *value)
> > +{
> > + if (field > BCB_FIELD_STAGE)
> > + return CMD_RET_FAILURE;
> > + return __bcb_set(fields[field], value);
> > +}
> > +
> > +int bcb_get(enum bcb_field field, char *value_out, size_t value_size)
> > +{
> > + int size;
> > + char *field_value;
> > +
> > + if (field > BCB_FIELD_STAGE)
> > + return CMD_RET_FAILURE;
> > + if (bcb_field_get(fields[field], &field_value, &size))
> > + return CMD_RET_FAILURE;
> > +
> > + strlcpy(value_out, field_value, value_size);
> > +
> > + return CMD_RET_SUCCESS;
> > +}
> > +
> > +int bcb_store(void)
> > +{
> > + return __bcb_store();
> > +}
> > +
> > +void bcb_reset(void)
> > +{
> > + __bcb_reset();
> > }
> >
> > static struct cmd_tbl cmd_bcb_sub[] = {
> > diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
> > index 2a6608b28c..3576b06772 100644
> > --- a/drivers/fastboot/fb_common.c
> > +++ b/drivers/fastboot/fb_common.c
> > @@ -91,6 +91,7 @@ void fastboot_okay(const char *reason, char *response)
> > */
> > int __weak fastboot_set_reboot_flag(enum fastboot_reboot_reason reason)
> > {
> > + int ret;
> > static const char * const boot_cmds[] = {
> > [FASTBOOT_REBOOT_REASON_BOOTLOADER] =
> "bootonce-bootloader",
> > [FASTBOOT_REBOOT_REASON_FASTBOOTD] = "boot-fastboot",
> > @@ -105,7 +106,18 @@ int __weak fastboot_set_reboot_flag(enum
> fastboot_reboot_reason reason)
> > if (reason >= FASTBOOT_REBOOT_REASONS_COUNT)
> > return -EINVAL;
> >
> > - return bcb_write_reboot_reason("mmc", mmc_dev, "misc",
> boot_cmds[reason]);
> > + ret = bcb_find_partition_and_load("mmc", mmc_dev, "misc");
> > + if (ret)
> > + goto out;
> > +
> > + ret = bcb_set(BCB_FIELD_COMMAND, boot_cmds[reason]);
> > + if (ret)
> > + goto out;
> > +
> > + ret = bcb_store();
> > +out:
> > + bcb_reset();
> > + return ret;
> > }
> >
> > /**
> > diff --git a/include/bcb.h b/include/bcb.h
> > index a6326523c4..1941d8c28b 100644
> > --- a/include/bcb.h
> > +++ b/include/bcb.h
> > @@ -8,15 +8,69 @@
> > #ifndef __BCB_H__
> > #define __BCB_H__
> >
> > +#include <part.h>
> > +
> > +enum bcb_field {
> > + BCB_FIELD_COMMAND,
> > + BCB_FIELD_STATUS,
> > + BCB_FIELD_RECOVERY,
> > + BCB_FIELD_STAGE
> > +};
> > +
> > #if IS_ENABLED(CONFIG_CMD_BCB)
> > -int bcb_write_reboot_reason(const char *iface, int devnum, char *partp,
> const char *reasonp);
> > +
> > +int bcb_find_partition_and_load(const char *iface,
> > + int devnum, char *partp);
> > +int bcb_load(struct blk_desc *block_description,
> > + struct disk_partition *disk_partition);
> > +int bcb_set(enum bcb_field field, const char *value);
> > +
> > +/**
> > + * bcb_get() - get the field value.
> > + * @field: field to get
> > + * @value_out: buffer to copy bcb field value to
> > + * @value_size: buffer size to avoid overflow in case
> > + * value_out is smaller then the field value
> > + */
> > +int bcb_get(enum bcb_field field, char *value_out, size_t value_size);
> > +
> > +int bcb_store(void);
> > +void bcb_reset(void);
> > +
> > #else
> > +
> > #include <linux/errno.h>
> > -static inline int bcb_write_reboot_reason(const char *iface, int devnum,
> > - char *partp, const char *reasonp)
> > +
> > +static inline int bcb_load(struct blk_desc *block_description,
> > + struct disk_partition *disk_partition)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int bcb_find_partition_and_load(const char *iface,
> > + int devnum, char *partp)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int bcb_set(enum bcb_field field, const char *value)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int bcb_get(enum bcb_field field, char *value_out)
> > {
> > return -EOPNOTSUPP;
> > }
> > +
> > +static inline int bcb_store(void)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static inline void bcb_reset(void)
> > +{
> > +}
> > #endif
> >
> > #endif /* __BCB_H__ */
> > --
> > 2.42.0.869.gea05f2083d-goog
>
More information about the U-Boot
mailing list