| Message ID | 20200617025515.23585-4-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: add capsule update support | expand |
On Wed, 17 Jun 2020 at 08:25, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > This function is essentially independent from tffp, and will also be > utilised in implementing UEFI capsule update in a later commit. > So just give it a more generic name. > In addition, a new configuration option, CONFIG_DFU_ALT, was introduced > so that the file will be compiled with different options, particularly > one added in a later commit. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > drivers/dfu/Kconfig | 5 +++++ > drivers/dfu/Makefile | 2 +- > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > include/dfu.h | 32 +++++++++++++-------------- > 4 files changed, 37 insertions(+), 19 deletions(-) > rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > You are renaming dfu_tftp.c to dfu_alt.c. Can the two api's not be added to dfu.c directly. Is there a need for a separate dfu_alt.c. It finally depends on what Lukasz thinks. <snip> > index ffae4bb54f80..5b1b13d7170d 100644 > --- a/drivers/dfu/dfu_tftp.c > +++ b/drivers/dfu/dfu_alt.c > @@ -10,8 +10,21 @@ > #include <errno.h> > #include <dfu.h> > > -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int > len, > - char *interface, char *devstring) > +/** > + * dfu_write_by_name() - write data to DFU medium > + * @dfu_entity_name: Name of DFU entity to write > + * @addr: Address of data buffer to write > + * @len: Number of bytes > + * @interface: Destination DFU medium (e.g. "mmc") > + * @devstring: Instance number of destination DFU medium (e.g. > "1") > + * > + * This function is storing data received on DFU supported medium which > + * is specified by @dfu_entity_name. > + * > + * Return: 0 - on success, error code - otherwise > + */ > +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > + unsigned int len, char *interface, char *devstring) > { > I know that this has been taken from the original dfu_tftp.c, but the addr parameter needs to be a void pointer. The unsigned int does not work when addr is greater than 4GB. This needs to be changed for both the api's that are being added. -sughosh
Hi Sughosh, > On Wed, 17 Jun 2020 at 08:25, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > This function is essentially independent from tffp, and will also be > > utilised in implementing UEFI capsule update in a later commit. > > So just give it a more generic name. > > In addition, a new configuration option, CONFIG_DFU_ALT, was > > introduced so that the file will be compiled with different > > options, particularly one added in a later commit. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > drivers/dfu/Kconfig | 5 +++++ > > drivers/dfu/Makefile | 2 +- > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > > include/dfu.h | 32 > > +++++++++++++-------------- 4 files changed, 37 insertions(+), 19 > > deletions(-) rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > > > > You are renaming dfu_tftp.c to dfu_alt.c. Can the two api's not be > added to dfu.c directly. Is there a need for a separate dfu_alt.c. It > finally depends on what Lukasz thinks. If this rename is going to provide a common code to be used in both use cases - then I'm fine with it. UEFI seems to be different from tftp. And dfu_tftp.c name indicates that it shall be used with tftp protocol. However, I don't know what rationale is behind dfu_alt.c name? > > <snip> > > > > index ffae4bb54f80..5b1b13d7170d 100644 > > --- a/drivers/dfu/dfu_tftp.c > > +++ b/drivers/dfu/dfu_alt.c > > @@ -10,8 +10,21 @@ > > #include <errno.h> > > #include <dfu.h> > > > > -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, > > unsigned int len, > > - char *interface, char *devstring) > > +/** > > + * dfu_write_by_name() - write data to DFU medium > > + * @dfu_entity_name: Name of DFU entity to write > > + * @addr: Address of data buffer to write > > + * @len: Number of bytes > > + * @interface: Destination DFU medium (e.g. "mmc") > > + * @devstring: Instance number of destination DFU medium > > (e.g. "1") > > + * > > + * This function is storing data received on DFU supported medium > > which > > + * is specified by @dfu_entity_name. > > + * > > + * Return: 0 - on success, error code - otherwise > > + */ > > +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > > + unsigned int len, char *interface, char > > *devstring) { > > > > I know that this has been taken from the original dfu_tftp.c, but the > addr parameter needs to be a void pointer. The unsigned int does not > work when addr is greater than 4GB. This needs to be changed for both > the api's that are being added. > > -sughosh Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Lukasz, On Sun, Jun 21, 2020 at 09:38:32AM +0200, Lukasz Majewski wrote: > Hi Sughosh, > > > On Wed, 17 Jun 2020 at 08:25, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > This function is essentially independent from tffp, and will also be > > > utilised in implementing UEFI capsule update in a later commit. > > > So just give it a more generic name. > > > In addition, a new configuration option, CONFIG_DFU_ALT, was > > > introduced so that the file will be compiled with different > > > options, particularly one added in a later commit. > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > drivers/dfu/Kconfig | 5 +++++ > > > drivers/dfu/Makefile | 2 +- > > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > > > include/dfu.h | 32 > > > +++++++++++++-------------- 4 files changed, 37 insertions(+), 19 > > > deletions(-) rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > > > > > > > You are renaming dfu_tftp.c to dfu_alt.c. Can the two api's not be > > added to dfu.c directly. Is there a need for a separate dfu_alt.c. It > > finally depends on what Lukasz thinks. > > If this rename is going to provide a common code to be used in both > use cases - then I'm fine with it. > > UEFI seems to be different from tftp. UEFI subsystem doesn't utilize tftp. > And dfu_tftp.c name indicates > that it shall be used with tftp protocol. Well, the name does, but in fact, as my commit message indicated, it has no dependency on tftp and UEFI can and will utilize this function. > However, I don't know what rationale is behind dfu_alt.c name? See my patch in my series: https://lists.denx.de/pipermail/u-boot/2020-June/416550.html We will need one more variant, especially, almost the same logic without FIT handling. Thanks, -Takahiro Akashi > > > > > <snip> > > > > > > > index ffae4bb54f80..5b1b13d7170d 100644 > > > --- a/drivers/dfu/dfu_tftp.c > > > +++ b/drivers/dfu/dfu_alt.c > > > @@ -10,8 +10,21 @@ > > > #include <errno.h> > > > #include <dfu.h> > > > > > > -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, > > > unsigned int len, > > > - char *interface, char *devstring) > > > +/** > > > + * dfu_write_by_name() - write data to DFU medium > > > + * @dfu_entity_name: Name of DFU entity to write > > > + * @addr: Address of data buffer to write > > > + * @len: Number of bytes > > > + * @interface: Destination DFU medium (e.g. "mmc") > > > + * @devstring: Instance number of destination DFU medium > > > (e.g. "1") > > > + * > > > + * This function is storing data received on DFU supported medium > > > which > > > + * is specified by @dfu_entity_name. > > > + * > > > + * Return: 0 - on success, error code - otherwise > > > + */ > > > +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > > > + unsigned int len, char *interface, char > > > *devstring) { > > > > > > > I know that this has been taken from the original dfu_tftp.c, but the > > addr parameter needs to be a void pointer. The unsigned int does not > > work when addr is greater than 4GB. This needs to be changed for both > > the api's that are being added. > > > > -sughosh > > > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Sun, Jun 21, 2020 at 12:05:29AM +0530, Sughosh Ganu wrote: > On Wed, 17 Jun 2020 at 08:25, AKASHI Takahiro <takahiro.akashi@linaro.org> > wrote: > > > This function is essentially independent from tffp, and will also be > > utilised in implementing UEFI capsule update in a later commit. > > So just give it a more generic name. > > In addition, a new configuration option, CONFIG_DFU_ALT, was introduced > > so that the file will be compiled with different options, particularly > > one added in a later commit. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > drivers/dfu/Kconfig | 5 +++++ > > drivers/dfu/Makefile | 2 +- > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > > include/dfu.h | 32 +++++++++++++-------------- > > 4 files changed, 37 insertions(+), 19 deletions(-) > > rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > > > > You are renaming dfu_tftp.c to dfu_alt.c. Can the two api's not be added to > dfu.c directly. Is there a need for a separate dfu_alt.c. It finally > depends on what Lukasz thinks. > > <snip> > > > > index ffae4bb54f80..5b1b13d7170d 100644 > > --- a/drivers/dfu/dfu_tftp.c > > +++ b/drivers/dfu/dfu_alt.c > > @@ -10,8 +10,21 @@ > > #include <errno.h> > > #include <dfu.h> > > > > -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int > > len, > > - char *interface, char *devstring) > > +/** > > + * dfu_write_by_name() - write data to DFU medium > > + * @dfu_entity_name: Name of DFU entity to write > > + * @addr: Address of data buffer to write > > + * @len: Number of bytes > > + * @interface: Destination DFU medium (e.g. "mmc") > > + * @devstring: Instance number of destination DFU medium (e.g. > > "1") > > + * > > + * This function is storing data received on DFU supported medium which > > + * is specified by @dfu_entity_name. > > + * > > + * Return: 0 - on success, error code - otherwise > > + */ > > +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > > + unsigned int len, char *interface, char *devstring) > > { > > > > I know that this has been taken from the original dfu_tftp.c, but the addr > parameter needs to be a void pointer. The unsigned int does not work when > addr is greater than 4GB. This needs to be changed for both the api's that > are being added. I can agree, but wonder why this has not caused any issue so far. I know that the caller, update_tftp(), defines and uses a "ulong" variable for this parameter. A casting like "(void *)(uintptr_t)addr" can be misleading. -Takahiro Akashi > -sughosh
Hi AKASHI, > Lukasz, > > On Sun, Jun 21, 2020 at 09:38:32AM +0200, Lukasz Majewski wrote: > > Hi Sughosh, > > > > > On Wed, 17 Jun 2020 at 08:25, AKASHI Takahiro > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > This function is essentially independent from tffp, and will > > > > also be utilised in implementing UEFI capsule update in a later > > > > commit. So just give it a more generic name. > > > > In addition, a new configuration option, CONFIG_DFU_ALT, was > > > > introduced so that the file will be compiled with different > > > > options, particularly one added in a later commit. > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > --- > > > > drivers/dfu/Kconfig | 5 +++++ > > > > drivers/dfu/Makefile | 2 +- > > > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > > > > include/dfu.h | 32 > > > > +++++++++++++-------------- 4 files changed, 37 insertions(+), > > > > 19 deletions(-) rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} > > > > (67%) > > > > > > You are renaming dfu_tftp.c to dfu_alt.c. Can the two api's not be > > > added to dfu.c directly. Is there a need for a separate > > > dfu_alt.c. It finally depends on what Lukasz thinks. > > > > If this rename is going to provide a common code to be used in both > > use cases - then I'm fine with it. > > > > UEFI seems to be different from tftp. > > UEFI subsystem doesn't utilize tftp. > > > And dfu_tftp.c name indicates > > that it shall be used with tftp protocol. > > Well, the name does, but in fact, as my commit message indicated, > it has no dependency on tftp and UEFI can and will utilize this > function. > > > However, I don't know what rationale is behind dfu_alt.c name? > > See my patch in my series: > https://lists.denx.de/pipermail/u-boot/2020-June/416550.html > > We will need one more variant, especially, almost the same logic > without FIT handling. Ok. Thanks for the explanation. > > Thanks, > -Takahiro Akashi > > > > > > > > > <snip> > > > > > > > > > > index ffae4bb54f80..5b1b13d7170d 100644 > > > > --- a/drivers/dfu/dfu_tftp.c > > > > +++ b/drivers/dfu/dfu_alt.c > > > > @@ -10,8 +10,21 @@ > > > > #include <errno.h> > > > > #include <dfu.h> > > > > > > > > -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, > > > > unsigned int len, > > > > - char *interface, char *devstring) > > > > +/** > > > > + * dfu_write_by_name() - write data to DFU medium > > > > + * @dfu_entity_name: Name of DFU entity to write > > > > + * @addr: Address of data buffer to write > > > > + * @len: Number of bytes > > > > + * @interface: Destination DFU medium (e.g. "mmc") > > > > + * @devstring: Instance number of destination DFU > > > > medium (e.g. "1") > > > > + * > > > > + * This function is storing data received on DFU supported > > > > medium which > > > > + * is specified by @dfu_entity_name. > > > > + * > > > > + * Return: 0 - on success, error code - otherwise > > > > + */ > > > > +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > > > > + unsigned int len, char *interface, char > > > > *devstring) { > > > > > > > > > > I know that this has been taken from the original dfu_tftp.c, but > > > the addr parameter needs to be a void pointer. The unsigned int > > > does not work when addr is greater than 4GB. This needs to be > > > changed for both the api's that are being added. > > > > > > -sughosh > > > > > > > > > > Best regards, > > > > Lukasz Majewski > > > > -- > > > > DENX Software Engineering GmbH, Managing Director: Wolfgang > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, > > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > > lukma@denx.de > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig index cafb6a34090e..1bddaef45532 100644 --- a/drivers/dfu/Kconfig +++ b/drivers/dfu/Kconfig @@ -15,8 +15,13 @@ config DFU_OVER_TFTP select UPDATE_TFTP if DFU +config DFU_ALT + bool + default n + config DFU_TFTP bool "DFU via TFTP" + select DFU_ALT select DFU_OVER_TFTP help This option allows performing update of DFU-managed medium with data diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile index 0d7925c083ef..cc7de1d3ed9b 100644 --- a/drivers/dfu/Makefile +++ b/drivers/dfu/Makefile @@ -9,5 +9,5 @@ obj-$(CONFIG_$(SPL_)DFU_MTD) += dfu_mtd.o obj-$(CONFIG_$(SPL_)DFU_NAND) += dfu_nand.o obj-$(CONFIG_$(SPL_)DFU_RAM) += dfu_ram.o obj-$(CONFIG_$(SPL_)DFU_SF) += dfu_sf.o -obj-$(CONFIG_$(SPL_)DFU_TFTP) += dfu_tftp.o +obj-$(CONFIG_$(SPL_)DFU_ALT) += dfu_alt.o obj-$(CONFIG_$(SPL_)DFU_VIRT) += dfu_virt.o diff --git a/drivers/dfu/dfu_tftp.c b/drivers/dfu/dfu_alt.c similarity index 67% rename from drivers/dfu/dfu_tftp.c rename to drivers/dfu/dfu_alt.c index ffae4bb54f80..5b1b13d7170d 100644 --- a/drivers/dfu/dfu_tftp.c +++ b/drivers/dfu/dfu_alt.c @@ -10,8 +10,21 @@ #include <errno.h> #include <dfu.h> -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int len, - char *interface, char *devstring) +/** + * dfu_write_by_name() - write data to DFU medium + * @dfu_entity_name: Name of DFU entity to write + * @addr: Address of data buffer to write + * @len: Number of bytes + * @interface: Destination DFU medium (e.g. "mmc") + * @devstring: Instance number of destination DFU medium (e.g. "1") + * + * This function is storing data received on DFU supported medium which + * is specified by @dfu_entity_name. + * + * Return: 0 - on success, error code - otherwise + */ +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, + unsigned int len, char *interface, char *devstring) { char *s, *sb; int alt_setting_num, ret; diff --git a/include/dfu.h b/include/dfu.h index 6fa450593605..94b0a9e68317 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -494,27 +494,27 @@ static inline int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, #endif /** - * dfu_tftp_write() - write TFTP data to DFU medium + * dfu_write_by_name() - write data to DFU medium + * @dfu_entity_name: Name of DFU entity to write + * @addr: Address of data buffer to write + * @len: Number of bytes + * @interface: Destination DFU medium (e.g. "mmc") + * @devstring: Instance number of destination DFU medium (e.g. "1") * - * This function is storing data received via TFTP on DFU supported medium. + * This function is storing data received on DFU supported medium which + * is specified by @dfu_entity_name. * - * @dfu_entity_name: name of DFU entity to write - * @addr: address of data buffer to write - * @len: number of bytes - * @interface: destination DFU medium (e.g. "mmc") - * @devstring: instance number of destination DFU medium (e.g. "1") - * - * Return: 0 on success, otherwise error code + * Return: 0 - on success, error code - otherwise */ -#if CONFIG_IS_ENABLED(DFU_TFTP) -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int len, - char *interface, char *devstring); +#if CONFIG_IS_ENABLED(DFU_ALT) +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, + unsigned int len, char *interface, char *devstring); #else -static inline int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, - unsigned int len, char *interface, - char *devstring) +static inline int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, + unsigned int len, char *interface, + char *devstring) { - puts("TFTP write support for DFU not available!\n"); + puts("write support for DFU not available!\n"); return -ENOSYS; } #endif
This function is essentially independent from tffp, and will also be utilised in implementing UEFI capsule update in a later commit. So just give it a more generic name. In addition, a new configuration option, CONFIG_DFU_ALT, was introduced so that the file will be compiled with different options, particularly one added in a later commit. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- drivers/dfu/Kconfig | 5 +++++ drivers/dfu/Makefile | 2 +- drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- include/dfu.h | 32 +++++++++++++-------------- 4 files changed, 37 insertions(+), 19 deletions(-) rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)