diff mbox series

[v2,03/17] dfu: rename dfu_tftp_write() to dfu_write_by_name()

Message ID 20200617025515.23585-4-takahiro.akashi@linaro.org
State New
Headers show
Series efi_loader: add capsule update support | expand

Commit Message

AKASHI Takahiro June 17, 2020, 2:55 a.m. UTC
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 at 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%)

Comments

Sughosh Ganu June 20, 2020, 6:35 p.m. UTC | #1
On Wed, 17 Jun 2020 at 08:25, AKASHI Takahiro <takahiro.akashi at 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 at 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
Lukasz Majewski June 21, 2020, 7:38 a.m. UTC | #2
Hi Sughosh,

> On Wed, 17 Jun 2020 at 08:25, AKASHI Takahiro
> <takahiro.akashi at 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 at 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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200621/3c37ed3b/attachment.sig>
AKASHI Takahiro June 22, 2020, 12:41 a.m. UTC | #3
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 at 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 at 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 at denx.de
AKASHI Takahiro June 22, 2020, 12:58 a.m. UTC | #4
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 at 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 at 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
Lukasz Majewski June 22, 2020, 7:19 a.m. UTC | #5
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 at 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 at 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 at 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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200622/54350bff/attachment.sig>
diff mbox series

Patch

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