| Message ID | 20200722060539.15168-3-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: add capsule update support | expand |
On 22.07.20 08:05, AKASHI Takahiro wrote: > The range of an addressable pointer can go beyond 'integer'. > So change the argument type to a void pointer. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > common/update.c | 3 ++- > drivers/dfu/dfu_alt.c | 4 ++-- > include/dfu.h | 4 ++-- > 3 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/common/update.c b/common/update.c > index 7f73c6372da0..f82d77cc0be9 100644 > --- a/common/update.c > +++ b/common/update.c > @@ -181,7 +181,8 @@ got_update_file: > } > > if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) { > - ret = dfu_write_by_name(fit_image_name, update_addr, > + ret = dfu_write_by_name(fit_image_name, > + (void *)update_addr, > update_size, interface, > devstring); > if (ret) > diff --git a/drivers/dfu/dfu_alt.c b/drivers/dfu/dfu_alt.c > index 5b1b13d7170d..f6b87c51ed30 100644 > --- a/drivers/dfu/dfu_alt.c > +++ b/drivers/dfu/dfu_alt.c > @@ -23,14 +23,14 @@ > * > * Return: 0 - on success, error code - otherwise > */ > -int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > +int dfu_write_by_name(char *dfu_entity_name, void *addr, > unsigned int len, char *interface, char *devstring) > { > char *s, *sb; > int alt_setting_num, ret; > struct dfu_entity *dfu; > > - debug("%s: name: %s addr: 0x%x len: %d device: %s:%s\n", __func__, > + debug("%s: name: %s addr: 0x%p len: %d device: %s:%s\n", __func__, > dfu_entity_name, addr, len, interface, devstring); > > ret = dfu_init_env_entities(interface, devstring); > diff --git a/include/dfu.h b/include/dfu.h > index 94b0a9e68317..327fffc0dba6 100644 > --- a/include/dfu.h > +++ b/include/dfu.h > @@ -507,10 +507,10 @@ static inline int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, > * Return: 0 - on success, error code - otherwise > */ > #if CONFIG_IS_ENABLED(DFU_ALT) > -int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > +int dfu_write_by_name(char *dfu_entity_name, void *addr, > unsigned int len, char *interface, char *devstring); > #else > -static inline int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > +static inline int dfu_write_by_name(char *dfu_entity_name, void *addr, update_tftp() takes the value of this address from environment variable loadaddr. So this is not a pointer. It is an address in the virtual address space of the sandbox. You will have to call map_sysmem() to make it a pointer. To be strict the correct type for addr is phys_addr_t. But as we use simple_strtoul() to convert the loadaddr string using ulong as type is also fine. I suggest to use ulong as in update_tftp. We need to add a call to map_sysmem() to convert to the address pointer needed by dfu_write_from_mem_addr(). Best regards Heinrich > unsigned int len, char *interface, > char *devstring) > { >
On 22.07.20 14:43, Heinrich Schuchardt wrote: > On 22.07.20 08:05, AKASHI Takahiro wrote: >> The range of an addressable pointer can go beyond 'integer'. >> So change the argument type to a void pointer. >> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> --- >> common/update.c | 3 ++- >> drivers/dfu/dfu_alt.c | 4 ++-- >> include/dfu.h | 4 ++-- >> 3 files changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/common/update.c b/common/update.c >> index 7f73c6372da0..f82d77cc0be9 100644 >> --- a/common/update.c >> +++ b/common/update.c >> @@ -181,7 +181,8 @@ got_update_file: >> } >> >> if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) { >> - ret = dfu_write_by_name(fit_image_name, update_addr, >> + ret = dfu_write_by_name(fit_image_name, >> + (void *)update_addr, >> update_size, interface, >> devstring); >> if (ret) >> diff --git a/drivers/dfu/dfu_alt.c b/drivers/dfu/dfu_alt.c >> index 5b1b13d7170d..f6b87c51ed30 100644 >> --- a/drivers/dfu/dfu_alt.c >> +++ b/drivers/dfu/dfu_alt.c >> @@ -23,14 +23,14 @@ >> * >> * Return: 0 - on success, error code - otherwise >> */ >> -int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, >> +int dfu_write_by_name(char *dfu_entity_name, void *addr, >> unsigned int len, char *interface, char *devstring) >> { >> char *s, *sb; >> int alt_setting_num, ret; >> struct dfu_entity *dfu; >> >> - debug("%s: name: %s addr: 0x%x len: %d device: %s:%s\n", __func__, >> + debug("%s: name: %s addr: 0x%p len: %d device: %s:%s\n", __func__, >> dfu_entity_name, addr, len, interface, devstring); >> >> ret = dfu_init_env_entities(interface, devstring); >> diff --git a/include/dfu.h b/include/dfu.h >> index 94b0a9e68317..327fffc0dba6 100644 >> --- a/include/dfu.h >> +++ b/include/dfu.h >> @@ -507,10 +507,10 @@ static inline int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, >> * Return: 0 - on success, error code - otherwise >> */ >> #if CONFIG_IS_ENABLED(DFU_ALT) >> -int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, >> +int dfu_write_by_name(char *dfu_entity_name, void *addr, >> unsigned int len, char *interface, char *devstring); >> #else >> -static inline int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, >> +static inline int dfu_write_by_name(char *dfu_entity_name, void *addr, > > update_tftp() takes the value of this address from environment variable > loadaddr. So this is not a pointer. It is an address in the virtual > address space of the sandbox. You will have to call map_sysmem() to make > it a pointer. > > To be strict the correct type for addr is phys_addr_t. But as we use > simple_strtoul() to convert the loadaddr string using ulong as type is > also fine. I suggest to use ulong as in update_tftp. > > We need to add a call to map_sysmem() to convert to the address pointer > needed by dfu_write_from_mem_addr(). My first analysis was wrong. The missing address conversions for the sandbox are in common/update.c and driver/dfu/dfu_ram.c. I have created patch https://lists.denx.de/pipermail/u-boot/2020-July/421060.html [PATCH 1/1] dfu: fix dfu tftp on sandbox The only change needed for the current patch is to remove the now superfluous conversion when calling dfu_write_from_mem_addr(). Otherwise: Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
diff --git a/common/update.c b/common/update.c index 7f73c6372da0..f82d77cc0be9 100644 --- a/common/update.c +++ b/common/update.c @@ -181,7 +181,8 @@ got_update_file: } if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) { - ret = dfu_write_by_name(fit_image_name, update_addr, + ret = dfu_write_by_name(fit_image_name, + (void *)update_addr, update_size, interface, devstring); if (ret) diff --git a/drivers/dfu/dfu_alt.c b/drivers/dfu/dfu_alt.c index 5b1b13d7170d..f6b87c51ed30 100644 --- a/drivers/dfu/dfu_alt.c +++ b/drivers/dfu/dfu_alt.c @@ -23,14 +23,14 @@ * * Return: 0 - on success, error code - otherwise */ -int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, +int dfu_write_by_name(char *dfu_entity_name, void *addr, unsigned int len, char *interface, char *devstring) { char *s, *sb; int alt_setting_num, ret; struct dfu_entity *dfu; - debug("%s: name: %s addr: 0x%x len: %d device: %s:%s\n", __func__, + debug("%s: name: %s addr: 0x%p len: %d device: %s:%s\n", __func__, dfu_entity_name, addr, len, interface, devstring); ret = dfu_init_env_entities(interface, devstring); diff --git a/include/dfu.h b/include/dfu.h index 94b0a9e68317..327fffc0dba6 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -507,10 +507,10 @@ static inline int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, * Return: 0 - on success, error code - otherwise */ #if CONFIG_IS_ENABLED(DFU_ALT) -int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, +int dfu_write_by_name(char *dfu_entity_name, void *addr, unsigned int len, char *interface, char *devstring); #else -static inline int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, +static inline int dfu_write_by_name(char *dfu_entity_name, void *addr, unsigned int len, char *interface, char *devstring) {
The range of an addressable pointer can go beyond 'integer'. So change the argument type to a void pointer. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- common/update.c | 3 ++- drivers/dfu/dfu_alt.c | 4 ++-- include/dfu.h | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-)