[U-Boot] IS_ERR_VALUE failing on socfpga gen5

[Simon Glass]

Hi Simon,

On Mon, 14 Oct 2019 at 14:05, Simon Goldschmidt
<simon.k.r.goldschmidt at gmail.com> wrote:
>
> Am 14.10.2019 um 22:02 schrieb Simon Glass:
> > Hi Simon,
> >
> > On Mon, 14 Oct 2019 at 13:13, Simon Goldschmidt
> > <simon.k.r.goldschmidt at gmail.com> wrote:
> >>
> >> Am 12.10.2019 um 00:11 schrieb Simon Glass:
> >>> Hi Simon,
> >>>
> >>> On Fri, 11 Oct 2019 at 12:31, Simon Goldschmidt
> >>> <simon.k.r.goldschmidt at gmail.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> Simon Glass <sjg at chromium.org> schrieb am Fr., 11. Okt. 2019, 20:27:
> >>>>>
> >>>>> Hi Simon,
> >>>>>
> >>>>> On Tue, 8 Oct 2019 at 14:34, Simon Goldschmidt
> >>>>> <simon.k.r.goldschmidt at gmail.com> wrote:
> >>>>>>
> >>>>>> In a series I'm currently preparing, I've stumbled accross the fact that
> >>>>>> IS_ERR_VALUE() doesn't reliably work on socfpga SPL as the onchip SRAM
> >>>>>> begins at 0xffff0000 and the heap is at the end of the 32 bit range.
> >>>>>>
> >>>>>
> >>>>> Which function is returning that address?
> >>>>
> >>>>
> >>>> Sorry, I don't know right now and it was kind of hard to debug. But it was somewhere in the DM initialization of new drivers I was adding. One of those functions returned a heap pointer that was interpreted as an error due to the address being in the range regarded as "error pointer"...
> >>>
> >>> I don't know a good solution to this. But we should consider changing
> >>> whatever API it was to use an int return value instead of an address.
> >>> The address can be passed back in another parameter. That is how most
> >>> uclass methods work.
> >>
> >> Digging into this again, it was 'syscon_node_to_regmap', which seems to
> >> be copied from Linux, so I don't think it's feasible to change this
> >> function. Also, IS_ERR() is used in ~120 files, I wouldn't want to
> >> change all these for socfpga.
> >>
> >> One idea that came to mind was to change the ERR_PTR/PTR_ERR macros in
> >> linux/err.h to convert errors to an architecture-specified pointer range
> >> instead of just converting the data type. However, that would mean
> >> another change to Linux imports, and Tom just seemed a little opposed to
> >> that...?
> >
> > I think we should keep it simple and change syscon_node_to_regmap() to
> > return an int. It's an easy change affecting 17 files and avoids the
> > problem.
>
> Well, it took me some hours to find the issue (without access to a
> debugger, sadly) and I'd like to save me and others from facing this
> again in the future. When I write the next driver, who tells me
> internals aren't using IS_ERR again?
>
> Plus, linux/err.h says:
>
> "This should be a per-architecture thing, to allow different error and
> pointer decisions."
>
> Let me prepare a patch to show what I mean and we can see if it's
> acceptable.

I don't think so. This is just making things confusing.

There is no need to use pointers to return errors. I know it is
slightly more efficient but the pain is much greater, as you have
found.

Also please check out log_msg_ret(). If you sprinkle that on all your
error returns and enable logging, then you get a nice detailed error
trace showing you where the problem happened. We should start trying
to use that everywhere IMO.

Regards,
Simon