[PATCH 1/5] dm: core: Fix devfdt_get_addr_ptr return value

[Simon Glass]

On Wed, 27 May 2020 at 00:45, Ovidiu Panait <ovidiu.panait at windriver.com> wrote:
>
> According to the description of devfdt_get_addr_ptr, this function should
> return NULL on failure, but currently it returns (void *)FDT_ADDR_T_NONE.
>
> This is also a problem because there are two definitions for
> dev_read_addr_ptr, depending on CONFIG_DM_DEV_READ_INLINE:
>
> 1. one returning NULL on failure (drivers/core/read.c):
> void *dev_read_addr_ptr(const struct udevice *dev)
> {
>         fdt_addr_t addr = dev_read_addr(dev);
>
>         return (addr == FDT_ADDR_T_NONE) ? NULL : map_sysmem(addr, 0);
> }
>
> 2. another one, which is a wrapper over devfdt_get_addr_ptr, returning
> (void *)FDT_ADDR_T_NONE (include/dm/read.h)
>
> static inline void *dev_read_addr_ptr(const struct udevice *dev)
> {
>         return devfdt_get_addr_ptr(dev);
> }
>
> Currently, some drivers which make use of devfdt_get_addr_ptr check the
> return value for NULL:
> drivers/i2c/mvtwsi.c
> drivers/i2c/designware_i2c.c
> drivers/usb/host/ehci-zynq.c
>
> while others check the return value for (void *)FDT_ADDR_T_NONE:
> drivers/pinctrl/mvebu/pinctrl-mvebu.c
> drivers/timer/ast_timer.c
> drivers/watchdog/ast_wdt.c
>
> Fix this by making devfdt_get_addr_ptr return NULL on failure, as
> described in the function comments. Also, update the drivers currently
> checking (void *)FDT_ADDR_T_NONE to check for NULL.
>
> Signed-off-by: Ovidiu Panait <ovidiu.panait at windriver.com>
> ---
>
>  drivers/clk/aspeed/clk_ast2500.c      | 4 ++--
>  drivers/core/fdtaddr.c                | 5 ++++-
>  drivers/i2c/ast_i2c.c                 | 4 ++--
>  drivers/pinctrl/mvebu/pinctrl-mvebu.c | 2 +-
>  drivers/timer/ast_timer.c             | 4 ++--
>  drivers/watchdog/ast_wdt.c            | 4 ++--
>  6 files changed, 13 insertions(+), 10 deletions(-)
>

+Sean Anderson

I did already review another patch on this topic[1].  I'm not sure
when that is going to land though. Perhaps this one should be rebased
on that? Or Matthias can check with the other custodian to see what is
happening with that series.

I've sent a patch for a test which detects the inconsistency[2]. It is
very easy to write a new test so I encourage people to do that
whenever there is a fix like this. So with that:

Tested on sandbox
Tested-by: Simon Glass <sjg at chromium.org>

[1] http://patchwork.ozlabs.org/project/uboot/patch/20200521161503.384823-10-seanga2@gmail.com/
[2] http://patchwork.ozlabs.org/project/uboot/patch/20200528082045.1.Ibd999a0382164a37d1e59c00c8c7a9ff92b8f53a@changeid/