diff mbox series

[v6,01/17] dfu: rename dfu_tftp_write() to dfu_write_by_name()

Message ID 20200907053426.1675646-2-takahiro.akashi@linaro.org
State New
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: add capsule update support | expand

Commit Message

takahiro.akashi@linaro.org Sept. 7, 2020, 5:34 a.m. UTC
This function is essentially independent from tftp, 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>
---
 common/update.c                       |  5 +++--
 drivers/dfu/Kconfig                   |  5 +++++
 drivers/dfu/Makefile                  |  2 +-
 drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++--
 include/dfu.h                         | 32 +++++++++++++--------------
 5 files changed, 40 insertions(+), 21 deletions(-)
 rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)

Comments

Heinrich Schuchardt Sept. 9, 2020, 12:30 p.m. UTC | #1
On 07.09.20 07:34, AKASHI Takahiro wrote:
> This function is essentially independent from tftp, 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>
> ---
>  common/update.c                       |  5 +++--
>  drivers/dfu/Kconfig                   |  5 +++++
>  drivers/dfu/Makefile                  |  2 +-
>  drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++--
>  include/dfu.h                         | 32 +++++++++++++--------------
>  5 files changed, 40 insertions(+), 21 deletions(-)
>  rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)
>
> diff --git a/common/update.c b/common/update.c
> index 36b6b7523d50..39946776d74f 100644
> --- a/common/update.c
> +++ b/common/update.c
> @@ -324,8 +324,9 @@ got_update_file:
>  			}
>  		} else if (fit_image_check_type(fit, noffset,
>  						IH_TYPE_FIRMWARE)) {
> -			ret = dfu_tftp_write(fit_image_name, update_addr,
> -					     update_size, interface, devstring);
> +			ret = dfu_write_by_name(fit_image_name, update_addr,
> +						update_size, interface,
> +						devstring);
>  			if (ret)
>  				return ret;
>  		}
> diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
> index 0eec00ba734d..78f901ff348a 100644
> --- a/drivers/dfu/Kconfig
> +++ b/drivers/dfu/Kconfig
> @@ -14,8 +14,13 @@ config DFU_OVER_TFTP
>  	depends on NET
>
>  if DFU
> +config DFU_ALT
> +	bool

You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as
string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al.
It is used to initialize environment variable dfu_alt_info. Your patch
leads to an error on Travis:

       arm:  +   odroid

+In file included from <command-line>:

+include/linux/kconfig.h:21:49: error: pasting "__ARG_PLACEHOLDER_" and
""uImage fat 0 1;"" does not give a valid preprocessing token

+   21 | #define _config_enabled(value)
__config_enabled(__ARG_PLACEHOLDER_##value)

Best regards

Heinrich

> +	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 84abdc79acd1..cecfbd76597b 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
>
takahiro.akashi@linaro.org Sept. 10, 2020, 1:54 a.m. UTC | #2
On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote:
> On 07.09.20 07:34, AKASHI Takahiro wrote:
> > This function is essentially independent from tftp, 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>
> > ---
> >  common/update.c                       |  5 +++--
> >  drivers/dfu/Kconfig                   |  5 +++++
> >  drivers/dfu/Makefile                  |  2 +-
> >  drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++--
> >  include/dfu.h                         | 32 +++++++++++++--------------
> >  5 files changed, 40 insertions(+), 21 deletions(-)
> >  rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)
> >
> > diff --git a/common/update.c b/common/update.c
> > index 36b6b7523d50..39946776d74f 100644
> > --- a/common/update.c
> > +++ b/common/update.c
> > @@ -324,8 +324,9 @@ got_update_file:
> >  			}
> >  		} else if (fit_image_check_type(fit, noffset,
> >  						IH_TYPE_FIRMWARE)) {
> > -			ret = dfu_tftp_write(fit_image_name, update_addr,
> > -					     update_size, interface, devstring);
> > +			ret = dfu_write_by_name(fit_image_name, update_addr,
> > +						update_size, interface,
> > +						devstring);
> >  			if (ret)
> >  				return ret;
> >  		}
> > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
> > index 0eec00ba734d..78f901ff348a 100644
> > --- a/drivers/dfu/Kconfig
> > +++ b/drivers/dfu/Kconfig
> > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP
> >  	depends on NET
> >
> >  if DFU
> > +config DFU_ALT
> > +	bool
> 
> You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as
> string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al.
> It is used to initialize environment variable dfu_alt_info. Your patch
> leads to an error on Travis:
> 
>        arm:  +   odroid

Okay, will fix it.
DFU_WRITE_ALT? Or, as Heinrich suggested, move them
to dfu.c if Lukasz agrees.

# I haven't seen any comments from him yet though.

-Takahiro Akashi


> +In file included from <command-line>:
> 
> +include/linux/kconfig.h:21:49: error: pasting "__ARG_PLACEHOLDER_" and
> ""uImage fat 0 1;"" does not give a valid preprocessing token
> 
> +   21 | #define _config_enabled(value)
> __config_enabled(__ARG_PLACEHOLDER_##value)
> 
> Best regards
> 
> Heinrich
> 
> > +	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 84abdc79acd1..cecfbd76597b 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
> >
>
Tom Rini Sept. 10, 2020, 2:26 a.m. UTC | #3
On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro wrote:
> On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote:
> > On 07.09.20 07:34, AKASHI Takahiro wrote:
> > > This function is essentially independent from tftp, 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>
> > > ---
> > >  common/update.c                       |  5 +++--
> > >  drivers/dfu/Kconfig                   |  5 +++++
> > >  drivers/dfu/Makefile                  |  2 +-
> > >  drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++--
> > >  include/dfu.h                         | 32 +++++++++++++--------------
> > >  5 files changed, 40 insertions(+), 21 deletions(-)
> > >  rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)
> > >
> > > diff --git a/common/update.c b/common/update.c
> > > index 36b6b7523d50..39946776d74f 100644
> > > --- a/common/update.c
> > > +++ b/common/update.c
> > > @@ -324,8 +324,9 @@ got_update_file:
> > >  			}
> > >  		} else if (fit_image_check_type(fit, noffset,
> > >  						IH_TYPE_FIRMWARE)) {
> > > -			ret = dfu_tftp_write(fit_image_name, update_addr,
> > > -					     update_size, interface, devstring);
> > > +			ret = dfu_write_by_name(fit_image_name, update_addr,
> > > +						update_size, interface,
> > > +						devstring);
> > >  			if (ret)
> > >  				return ret;
> > >  		}
> > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
> > > index 0eec00ba734d..78f901ff348a 100644
> > > --- a/drivers/dfu/Kconfig
> > > +++ b/drivers/dfu/Kconfig
> > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP
> > >  	depends on NET
> > >
> > >  if DFU
> > > +config DFU_ALT
> > > +	bool
> > 
> > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as
> > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al.
> > It is used to initialize environment variable dfu_alt_info. Your patch
> > leads to an error on Travis:
> > 
> >        arm:  +   odroid
> 
> Okay, will fix it.
> DFU_WRITE_ALT? Or, as Heinrich suggested, move them
> to dfu.c if Lukasz agrees.
> 
> # I haven't seen any comments from him yet though.

I think what you're introducing as "CONFIG_DFU_ALT" and renaming
"drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" is what's confusing.
That I think needs a different name than "alt" to convey that it's a
generic "from DRAM to flash/etc" hook.
takahiro.akashi@linaro.org Sept. 11, 2020, 12:46 a.m. UTC | #4
On Wed, Sep 09, 2020 at 10:26:34PM -0400, Tom Rini wrote:
> On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro wrote:
> > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote:
> > > On 07.09.20 07:34, AKASHI Takahiro wrote:
> > > > This function is essentially independent from tftp, 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>
> > > > ---
> > > >  common/update.c                       |  5 +++--
> > > >  drivers/dfu/Kconfig                   |  5 +++++
> > > >  drivers/dfu/Makefile                  |  2 +-
> > > >  drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++--
> > > >  include/dfu.h                         | 32 +++++++++++++--------------
> > > >  5 files changed, 40 insertions(+), 21 deletions(-)
> > > >  rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)
> > > >
> > > > diff --git a/common/update.c b/common/update.c
> > > > index 36b6b7523d50..39946776d74f 100644
> > > > --- a/common/update.c
> > > > +++ b/common/update.c
> > > > @@ -324,8 +324,9 @@ got_update_file:
> > > >  			}
> > > >  		} else if (fit_image_check_type(fit, noffset,
> > > >  						IH_TYPE_FIRMWARE)) {
> > > > -			ret = dfu_tftp_write(fit_image_name, update_addr,
> > > > -					     update_size, interface, devstring);
> > > > +			ret = dfu_write_by_name(fit_image_name, update_addr,
> > > > +						update_size, interface,
> > > > +						devstring);
> > > >  			if (ret)
> > > >  				return ret;
> > > >  		}
> > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
> > > > index 0eec00ba734d..78f901ff348a 100644
> > > > --- a/drivers/dfu/Kconfig
> > > > +++ b/drivers/dfu/Kconfig
> > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP
> > > >  	depends on NET
> > > >
> > > >  if DFU
> > > > +config DFU_ALT
> > > > +	bool
> > > 
> > > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as
> > > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al.
> > > It is used to initialize environment variable dfu_alt_info. Your patch
> > > leads to an error on Travis:
> > > 
> > >        arm:  +   odroid
> > 
> > Okay, will fix it.
> > DFU_WRITE_ALT? Or, as Heinrich suggested, move them
> > to dfu.c if Lukasz agrees.
> > 
> > # I haven't seen any comments from him yet though.
> 
> I think what you're introducing as "CONFIG_DFU_ALT" and renaming
> "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" is what's confusing.
> That I think needs a different name than "alt" to convey that it's a
> generic "from DRAM to flash/etc" hook.

Whatever the name is, I need reviews on all of my DFU patches from Lukasz
in the first place.

-Takahiro Akashi


> -- 
> Tom
Tom Rini Sept. 11, 2020, 2:41 a.m. UTC | #5
On Fri, Sep 11, 2020 at 09:46:14AM +0900, AKASHI Takahiro wrote:
> On Wed, Sep 09, 2020 at 10:26:34PM -0400, Tom Rini wrote:
> > On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro wrote:
> > > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote:
> > > > On 07.09.20 07:34, AKASHI Takahiro wrote:
> > > > > This function is essentially independent from tftp, 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>
> > > > > ---
> > > > >  common/update.c                       |  5 +++--
> > > > >  drivers/dfu/Kconfig                   |  5 +++++
> > > > >  drivers/dfu/Makefile                  |  2 +-
> > > > >  drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++--
> > > > >  include/dfu.h                         | 32 +++++++++++++--------------
> > > > >  5 files changed, 40 insertions(+), 21 deletions(-)
> > > > >  rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)
> > > > >
> > > > > diff --git a/common/update.c b/common/update.c
> > > > > index 36b6b7523d50..39946776d74f 100644
> > > > > --- a/common/update.c
> > > > > +++ b/common/update.c
> > > > > @@ -324,8 +324,9 @@ got_update_file:
> > > > >  			}
> > > > >  		} else if (fit_image_check_type(fit, noffset,
> > > > >  						IH_TYPE_FIRMWARE)) {
> > > > > -			ret = dfu_tftp_write(fit_image_name, update_addr,
> > > > > -					     update_size, interface, devstring);
> > > > > +			ret = dfu_write_by_name(fit_image_name, update_addr,
> > > > > +						update_size, interface,
> > > > > +						devstring);
> > > > >  			if (ret)
> > > > >  				return ret;
> > > > >  		}
> > > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
> > > > > index 0eec00ba734d..78f901ff348a 100644
> > > > > --- a/drivers/dfu/Kconfig
> > > > > +++ b/drivers/dfu/Kconfig
> > > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP
> > > > >  	depends on NET
> > > > >
> > > > >  if DFU
> > > > > +config DFU_ALT
> > > > > +	bool
> > > > 
> > > > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as
> > > > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al.
> > > > It is used to initialize environment variable dfu_alt_info. Your patch
> > > > leads to an error on Travis:
> > > > 
> > > >        arm:  +   odroid
> > > 
> > > Okay, will fix it.
> > > DFU_WRITE_ALT? Or, as Heinrich suggested, move them
> > > to dfu.c if Lukasz agrees.
> > > 
> > > # I haven't seen any comments from him yet though.
> > 
> > I think what you're introducing as "CONFIG_DFU_ALT" and renaming
> > "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" is what's confusing.
> > That I think needs a different name than "alt" to convey that it's a
> > generic "from DRAM to flash/etc" hook.
> 
> Whatever the name is, I need reviews on all of my DFU patches from Lukasz
> in the first place.

The lower impact on the rest of the DFU subsytem and other automated
testing that can be done, the easier it can be for someone else to
review the DFU parts if Lukasz isn't able to find time to do it.
Thanks.
takahiro.akashi@linaro.org Sept. 18, 2020, 1:28 a.m. UTC | #6
Tom, Lukasz,

On Thu, Sep 10, 2020 at 10:41:39PM -0400, Tom Rini wrote:
> On Fri, Sep 11, 2020 at 09:46:14AM +0900, AKASHI Takahiro wrote:
> > On Wed, Sep 09, 2020 at 10:26:34PM -0400, Tom Rini wrote:
> > > On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro wrote:
> > > > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote:
> > > > > On 07.09.20 07:34, AKASHI Takahiro wrote:
> > > > > > This function is essentially independent from tftp, 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>
> > > > > > ---
> > > > > >  common/update.c                       |  5 +++--
> > > > > >  drivers/dfu/Kconfig                   |  5 +++++
> > > > > >  drivers/dfu/Makefile                  |  2 +-
> > > > > >  drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++--
> > > > > >  include/dfu.h                         | 32 +++++++++++++--------------
> > > > > >  5 files changed, 40 insertions(+), 21 deletions(-)
> > > > > >  rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)
> > > > > >
> > > > > > diff --git a/common/update.c b/common/update.c
> > > > > > index 36b6b7523d50..39946776d74f 100644
> > > > > > --- a/common/update.c
> > > > > > +++ b/common/update.c
> > > > > > @@ -324,8 +324,9 @@ got_update_file:
> > > > > >  			}
> > > > > >  		} else if (fit_image_check_type(fit, noffset,
> > > > > >  						IH_TYPE_FIRMWARE)) {
> > > > > > -			ret = dfu_tftp_write(fit_image_name, update_addr,
> > > > > > -					     update_size, interface, devstring);
> > > > > > +			ret = dfu_write_by_name(fit_image_name, update_addr,
> > > > > > +						update_size, interface,
> > > > > > +						devstring);
> > > > > >  			if (ret)
> > > > > >  				return ret;
> > > > > >  		}
> > > > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
> > > > > > index 0eec00ba734d..78f901ff348a 100644
> > > > > > --- a/drivers/dfu/Kconfig
> > > > > > +++ b/drivers/dfu/Kconfig
> > > > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP
> > > > > >  	depends on NET
> > > > > >
> > > > > >  if DFU
> > > > > > +config DFU_ALT
> > > > > > +	bool
> > > > > 
> > > > > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as
> > > > > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al.
> > > > > It is used to initialize environment variable dfu_alt_info. Your patch
> > > > > leads to an error on Travis:
> > > > > 
> > > > >        arm:  +   odroid
> > > > 
> > > > Okay, will fix it.
> > > > DFU_WRITE_ALT? Or, as Heinrich suggested, move them
> > > > to dfu.c if Lukasz agrees.
> > > > 
> > > > # I haven't seen any comments from him yet though.
> > > 
> > > I think what you're introducing as "CONFIG_DFU_ALT" and renaming
> > > "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" is what's confusing.
> > > That I think needs a different name than "alt" to convey that it's a
> > > generic "from DRAM to flash/etc" hook.
> > 
> > Whatever the name is, I need reviews on all of my DFU patches from Lukasz
> > in the first place.
> 
> The lower impact on the rest of the DFU subsytem and other automated
> testing that can be done, the easier it can be for someone else to
> review the DFU parts if Lukasz isn't able to find time to do it.

It sounds nice, but who will take that role?

Please name that person(s) if you're still busy on other stuffs.
        -> Lukasz

-Takahiro Akashi

> Thanks.
> 
> -- 
> Tom
Tom Rini Sept. 18, 2020, 1:32 a.m. UTC | #7
On Fri, Sep 18, 2020 at 10:28:57AM +0900, AKASHI Takahiro wrote:
> Tom, Lukasz,
> 
> On Thu, Sep 10, 2020 at 10:41:39PM -0400, Tom Rini wrote:
> > On Fri, Sep 11, 2020 at 09:46:14AM +0900, AKASHI Takahiro wrote:
> > > On Wed, Sep 09, 2020 at 10:26:34PM -0400, Tom Rini wrote:
> > > > On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro wrote:
> > > > > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote:
> > > > > > On 07.09.20 07:34, AKASHI Takahiro wrote:
> > > > > > > This function is essentially independent from tftp, 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>
> > > > > > > ---
> > > > > > >  common/update.c                       |  5 +++--
> > > > > > >  drivers/dfu/Kconfig                   |  5 +++++
> > > > > > >  drivers/dfu/Makefile                  |  2 +-
> > > > > > >  drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++--
> > > > > > >  include/dfu.h                         | 32 +++++++++++++--------------
> > > > > > >  5 files changed, 40 insertions(+), 21 deletions(-)
> > > > > > >  rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)
> > > > > > >
> > > > > > > diff --git a/common/update.c b/common/update.c
> > > > > > > index 36b6b7523d50..39946776d74f 100644
> > > > > > > --- a/common/update.c
> > > > > > > +++ b/common/update.c
> > > > > > > @@ -324,8 +324,9 @@ got_update_file:
> > > > > > >  			}
> > > > > > >  		} else if (fit_image_check_type(fit, noffset,
> > > > > > >  						IH_TYPE_FIRMWARE)) {
> > > > > > > -			ret = dfu_tftp_write(fit_image_name, update_addr,
> > > > > > > -					     update_size, interface, devstring);
> > > > > > > +			ret = dfu_write_by_name(fit_image_name, update_addr,
> > > > > > > +						update_size, interface,
> > > > > > > +						devstring);
> > > > > > >  			if (ret)
> > > > > > >  				return ret;
> > > > > > >  		}
> > > > > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
> > > > > > > index 0eec00ba734d..78f901ff348a 100644
> > > > > > > --- a/drivers/dfu/Kconfig
> > > > > > > +++ b/drivers/dfu/Kconfig
> > > > > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP
> > > > > > >  	depends on NET
> > > > > > >
> > > > > > >  if DFU
> > > > > > > +config DFU_ALT
> > > > > > > +	bool
> > > > > > 
> > > > > > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as
> > > > > > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al.
> > > > > > It is used to initialize environment variable dfu_alt_info. Your patch
> > > > > > leads to an error on Travis:
> > > > > > 
> > > > > >        arm:  +   odroid
> > > > > 
> > > > > Okay, will fix it.
> > > > > DFU_WRITE_ALT? Or, as Heinrich suggested, move them
> > > > > to dfu.c if Lukasz agrees.
> > > > > 
> > > > > # I haven't seen any comments from him yet though.
> > > > 
> > > > I think what you're introducing as "CONFIG_DFU_ALT" and renaming
> > > > "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" is what's confusing.
> > > > That I think needs a different name than "alt" to convey that it's a
> > > > generic "from DRAM to flash/etc" hook.
> > > 
> > > Whatever the name is, I need reviews on all of my DFU patches from Lukasz
> > > in the first place.
> > 
> > The lower impact on the rest of the DFU subsytem and other automated
> > testing that can be done, the easier it can be for someone else to
> > review the DFU parts if Lukasz isn't able to find time to do it.
> 
> It sounds nice, but who will take that role?

Me.
Lukasz Majewski Sept. 18, 2020, 8:08 a.m. UTC | #8
On Thu, 17 Sep 2020 21:32:25 -0400
Tom Rini <trini@konsulko.com> wrote:

> On Fri, Sep 18, 2020 at 10:28:57AM +0900, AKASHI Takahiro wrote:
> > Tom, Lukasz,
> > 
> > On Thu, Sep 10, 2020 at 10:41:39PM -0400, Tom Rini wrote:  
> > > On Fri, Sep 11, 2020 at 09:46:14AM +0900, AKASHI Takahiro wrote:  
> > > > On Wed, Sep 09, 2020 at 10:26:34PM -0400, Tom Rini wrote:  
> > > > > On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro
> > > > > wrote:  
> > > > > > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich
> > > > > > Schuchardt wrote:  
> > > > > > > On 07.09.20 07:34, AKASHI Takahiro wrote:  
> > > > > > > > This function is essentially independent from tftp, 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> ---
> > > > > > > >  common/update.c                       |  5 +++--
> > > > > > > >  drivers/dfu/Kconfig                   |  5 +++++
> > > > > > > >  drivers/dfu/Makefile                  |  2 +-
> > > > > > > >  drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17
> > > > > > > > ++++++++++++-- include/dfu.h                         |
> > > > > > > > 32 +++++++++++++-------------- 5 files changed, 40
> > > > > > > > insertions(+), 21 deletions(-) rename
> > > > > > > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%)
> > > > > > > >
> > > > > > > > diff --git a/common/update.c b/common/update.c
> > > > > > > > index 36b6b7523d50..39946776d74f 100644
> > > > > > > > --- a/common/update.c
> > > > > > > > +++ b/common/update.c
> > > > > > > > @@ -324,8 +324,9 @@ got_update_file:
> > > > > > > >  			}
> > > > > > > >  		} else if (fit_image_check_type(fit,
> > > > > > > > noffset, IH_TYPE_FIRMWARE)) {
> > > > > > > > -			ret =
> > > > > > > > dfu_tftp_write(fit_image_name, update_addr,
> > > > > > > > -
> > > > > > > > update_size, interface, devstring);
> > > > > > > > +			ret =
> > > > > > > > dfu_write_by_name(fit_image_name, update_addr,
> > > > > > > > +
> > > > > > > > update_size, interface,
> > > > > > > > +
> > > > > > > > devstring); if (ret)
> > > > > > > >  				return ret;
> > > > > > > >  		}
> > > > > > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
> > > > > > > > index 0eec00ba734d..78f901ff348a 100644
> > > > > > > > --- a/drivers/dfu/Kconfig
> > > > > > > > +++ b/drivers/dfu/Kconfig
> > > > > > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP
> > > > > > > >  	depends on NET
> > > > > > > >
> > > > > > > >  if DFU
> > > > > > > > +config DFU_ALT
> > > > > > > > +	bool  
> > > > > > > 
> > > > > > > You cannot define CONFIG_DFU_ALT as bool. This symbol
> > > > > > > already exists as string in include/configs/odroid.h,
> > > > > > > include/configs/s5p_goni.h, et.al. It is used to
> > > > > > > initialize environment variable dfu_alt_info. Your patch
> > > > > > > leads to an error on Travis:
> > > > > > > 
> > > > > > >        arm:  +   odroid  
> > > > > > 
> > > > > > Okay, will fix it.
> > > > > > DFU_WRITE_ALT? Or, as Heinrich suggested, move them
> > > > > > to dfu.c if Lukasz agrees.
> > > > > > 
> > > > > > # I haven't seen any comments from him yet though.  
> > > > > 
> > > > > I think what you're introducing as "CONFIG_DFU_ALT" and
> > > > > renaming "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c"
> > > > > is what's confusing. That I think needs a different name than
> > > > > "alt" to convey that it's a generic "from DRAM to flash/etc"
> > > > > hook.  
> > > > 
> > > > Whatever the name is, I need reviews on all of my DFU patches
> > > > from Lukasz in the first place.  
> > > 
> > > The lower impact on the rest of the DFU subsytem and other
> > > automated testing that can be done, the easier it can be for
> > > someone else to review the DFU parts if Lukasz isn't able to find
> > > time to do it.  
> > 
> > It sounds nice, but who will take that role?  
> 
> Me.
> 

I thought that there is some ongoing discussion between Heinrich and
Takahiro regarding those patches ...

And yes, I'm busy with other work and my time budget is
unfortunately reduced since March 2020.

It would be good to have some help to not block ongoing DFU
development.


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 mbox series

Patch

diff --git a/common/update.c b/common/update.c
index 36b6b7523d50..39946776d74f 100644
--- a/common/update.c
+++ b/common/update.c
@@ -324,8 +324,9 @@  got_update_file:
 			}
 		} else if (fit_image_check_type(fit, noffset,
 						IH_TYPE_FIRMWARE)) {
-			ret = dfu_tftp_write(fit_image_name, update_addr,
-					     update_size, interface, devstring);
+			ret = dfu_write_by_name(fit_image_name, update_addr,
+						update_size, interface,
+						devstring);
 			if (ret)
 				return ret;
 		}
diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
index 0eec00ba734d..78f901ff348a 100644
--- a/drivers/dfu/Kconfig
+++ b/drivers/dfu/Kconfig
@@ -14,8 +14,13 @@  config DFU_OVER_TFTP
 	depends on NET
 
 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 84abdc79acd1..cecfbd76597b 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