| Message ID | 20190814102648.23082-1-j-keerthy@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [U-Boot] core: of_addr: Correct the size type of of_get_address to fdt_size_t | expand |
Hi Keerthy, On Wed, Aug 14, 2019 at 03:56:48PM +0530, Keerthy wrote: > Currently the size parameter is defined as u64 type. > Correct the size type of of_get_address to fdt_size_t > so that both 64 bit and 32 bit architectures are taken > care of. > > The initial bug report: > https://patchwork.ozlabs.org/patch/1090094/#2212555 > > Fixes: e679d03b08fb ("core: ofnode: Add ofnode_get_addr_size_index") > Reported-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> > Tested-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> > Signed-off-by: Keerthy <j-keerthy@ti.com> > --- > > Changes from RFT: > > * Fixed a typo in the commit log. > * Added Reported-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> > Tested-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> > > drivers/core/of_addr.c | 4 ++-- > drivers/core/ofnode.c | 2 +- > include/dm/of_addr.h | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/core/of_addr.c b/drivers/core/of_addr.c > index 4e256d9926..812a400b19 100644 > --- a/drivers/core/of_addr.c > +++ b/drivers/core/of_addr.c > @@ -122,7 +122,7 @@ static void dev_count_cells(const struct device_node *np, int *nap, int *nsp) > } > > const __be32 *of_get_address(const struct device_node *dev, int index, > - u64 *size, unsigned int *flags) > + fdt_size_t *size, unsigned int *flags) I took some time to also review the changes in addition to testing. I can see that, since its inception in Linux [1], of_get_address() used 'u64*' type for its 'size' argument. That's still valid in v5.3-rc5. So, it looks to me that with this patch we diverge from Linux. I would barely think that the ASAN issue being fixed in this patch exists in Linux, since the latter receives much more KASAN-enabled testing on regular basis. Do you foresee any alternative fix w/o diverging from Linux? [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=22ae782f86b7
On 19/08/19 3:54 PM, Eugeniu Rosca wrote: > Hi Keerthy, > > On Wed, Aug 14, 2019 at 03:56:48PM +0530, Keerthy wrote: >> Currently the size parameter is defined as u64 type. >> Correct the size type of of_get_address to fdt_size_t >> so that both 64 bit and 32 bit architectures are taken >> care of. >> >> The initial bug report: >> https://patchwork.ozlabs.org/patch/1090094/#2212555 >> >> Fixes: e679d03b08fb ("core: ofnode: Add ofnode_get_addr_size_index") >> Reported-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> >> Tested-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> >> Signed-off-by: Keerthy <j-keerthy@ti.com> >> --- >> >> Changes from RFT: >> >> * Fixed a typo in the commit log. >> * Added Reported-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> >> Tested-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> >> >> drivers/core/of_addr.c | 4 ++-- >> drivers/core/ofnode.c | 2 +- >> include/dm/of_addr.h | 2 +- >> 3 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/core/of_addr.c b/drivers/core/of_addr.c >> index 4e256d9926..812a400b19 100644 >> --- a/drivers/core/of_addr.c >> +++ b/drivers/core/of_addr.c >> @@ -122,7 +122,7 @@ static void dev_count_cells(const struct device_node *np, int *nap, int *nsp) >> } >> >> const __be32 *of_get_address(const struct device_node *dev, int index, >> - u64 *size, unsigned int *flags) >> + fdt_size_t *size, unsigned int *flags) > > I took some time to also review the changes in addition to testing. > > I can see that, since its inception in Linux [1], of_get_address() used > 'u64*' type for its 'size' argument. That's still valid in v5.3-rc5. > So, it looks to me that with this patch we diverge from Linux. > > I would barely think that the ASAN issue being fixed in this patch > exists in Linux, since the latter receives much more KASAN-enabled > testing on regular basis. > > Do you foresee any alternative fix w/o diverging from Linux? I am afraid No but isn't fdt_size_t also right type to represent size? > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=22ae782f86b7 >
Hi Keerthy, Hi Simon, cc: Tom On Tue, Aug 20, 2019 at 09:39:32AM +0530, Keerthy wrote: > On 19/08/19 3:54 PM, Eugeniu Rosca wrote: [..] > > I took some time to also review the changes in addition to testing. > > > > I can see that, since its inception in Linux [1], of_get_address() used > > 'u64*' type for its 'size' argument. That's still valid in v5.3-rc5. > > So, it looks to me that with this patch we diverge from Linux. > > > > I would barely think that the ASAN issue being fixed in this patch > > exists in Linux, since the latter receives much more KASAN-enabled > > testing on regular basis. > > > > Do you foresee any alternative fix w/o diverging from Linux? > > I am afraid No but isn't fdt_size_t also right type to represent size? 'fdt_size_t' is a U-Boot-ism, i.e. it exists nowhere else but in U-Boot. Just for the record, it appears to be added by Simon's v2013.04 commit 4397a2a80baef ("fdt: Add fdtdec_get_addr_size() to read reg properties") IMHO injecting a U-Boot-specific data type into the prototype of a Linux-backported function has below major drawbacks: - It is a non-upstream-able solution. Linux will unlikely accept 'fdt_size_t' as a new type simply b/c Linux OF code existed over a decade and it didn't need this type so far. - Mutilating Linux upstream code will make further U-Boot backports painful, time consuming and error-prone. Are we on the same page here? > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=22ae782f86b7 > >
Hi, On Mon, 26 Aug 2019 at 11:16, Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > Hi Keerthy, > Hi Simon, > cc: Tom > > On Tue, Aug 20, 2019 at 09:39:32AM +0530, Keerthy wrote: > > On 19/08/19 3:54 PM, Eugeniu Rosca wrote: > [..] > > > I took some time to also review the changes in addition to testing. > > > > > > I can see that, since its inception in Linux [1], of_get_address() used > > > 'u64*' type for its 'size' argument. That's still valid in v5.3-rc5. > > > So, it looks to me that with this patch we diverge from Linux. > > > > > > I would barely think that the ASAN issue being fixed in this patch > > > exists in Linux, since the latter receives much more KASAN-enabled > > > testing on regular basis. > > > > > > Do you foresee any alternative fix w/o diverging from Linux? > > > > I am afraid No but isn't fdt_size_t also right type to represent size? > > 'fdt_size_t' is a U-Boot-ism, i.e. it exists nowhere else but in U-Boot. > Just for the record, it appears to be added by Simon's v2013.04 commit > 4397a2a80baef ("fdt: Add fdtdec_get_addr_size() to read reg properties") > > IMHO injecting a U-Boot-specific data type into the prototype of a > Linux-backported function has below major drawbacks: > > - It is a non-upstream-able solution. Linux will unlikely accept > 'fdt_size_t' as a new type simply b/c Linux OF code existed over > a decade and it didn't need this type so far. > - Mutilating Linux upstream code will make further U-Boot backports > painful, time consuming and error-prone. > > Are we on the same page here? > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=22ae782f86b7 > > > > Yes I agree. What do people think about this approach? https://gitlab.denx.de/u-boot/custodians/u-boot-dm/commit/82c65a88284c27a02d0958d8a5354390893b172a Regards, SImon
diff --git a/drivers/core/of_addr.c b/drivers/core/of_addr.c index 4e256d9926..812a400b19 100644 --- a/drivers/core/of_addr.c +++ b/drivers/core/of_addr.c @@ -122,7 +122,7 @@ static void dev_count_cells(const struct device_node *np, int *nap, int *nsp) } const __be32 *of_get_address(const struct device_node *dev, int index, - u64 *size, unsigned int *flags) + fdt_size_t *size, unsigned int *flags) { const __be32 *prop; int psize; @@ -347,7 +347,7 @@ int of_address_to_resource(const struct device_node *dev, int index, struct resource *r) { const __be32 *addrp; - u64 size; + fdt_size_t size; unsigned int flags; const char *name = NULL; diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index 2ac73af934..b21b5183a3 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -260,7 +260,7 @@ fdt_addr_t ofnode_get_addr_size_index(ofnode node, int index, fdt_size_t *size) uint flags; prop_val = of_get_address(ofnode_to_np(node), index, - (u64 *)size, &flags); + size, &flags); if (!prop_val) return FDT_ADDR_T_NONE; diff --git a/include/dm/of_addr.h b/include/dm/of_addr.h index 3fa1ffce81..52443ef296 100644 --- a/include/dm/of_addr.h +++ b/include/dm/of_addr.h @@ -58,7 +58,7 @@ u64 of_translate_dma_address(const struct device_node *no, const __be32 *in_addr * @return pointer to address which can be read */ const __be32 *of_get_address(const struct device_node *no, int index, - u64 *size, unsigned int *flags); + fdt_size_t *size, unsigned int *flags); struct resource;
Currently the size parameter is defined as u64 type. Correct the size type of of_get_address to fdt_size_t so that both 64 bit and 32 bit architectures are taken care of. The initial bug report: https://patchwork.ozlabs.org/patch/1090094/#2212555 Fixes: e679d03b08fb ("core: ofnode: Add ofnode_get_addr_size_index") Reported-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> Tested-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> Signed-off-by: Keerthy <j-keerthy@ti.com> --- Changes from RFT: * Fixed a typo in the commit log. * Added Reported-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> Tested-by: Eugeniu Rosca <rosca.eugeniu@gmail.com> drivers/core/of_addr.c | 4 ++-- drivers/core/ofnode.c | 2 +- include/dm/of_addr.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)