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

[Simon Glass]

Hi Sean,


On Thu, 28 May 2020 at 13:29, Sean Anderson <seanga2 at gmail.com> wrote:
>
> On 5/28/20 10:32 AM, Simon Glass wrote:
> > 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
>
> It looks like I never actually got CC'd


Oops sorry.

>
>
> >
> > 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.
>
> Hopefully soon; I believe the last holdup was passing CI.
>
> >
> > 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:
>
> I think that's a good idea. However, from what I can see, that patch
> tests dev_read_addr_ptr, not devfdt_get_addr_ptr. You may also want to
> check the error case, to make sure FDT_ADDR_T_NONE gets returned.

Yes that's right...it is a test for dev_read_addr_ptr(), although in
one case it calls the latter.

>
>
> >
> > 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/
> >
>
> --Sean
>

Regards,
Simon