Hi Simon, On Mon, 14 Oct 2019 at 14:05, Simon Goldschmidt <simon.k.r.goldschm...@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.goldschm...@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.goldschm...@gmail.com> wrote: > >>>> > >>>> > >>>> > >>>> Simon Glass <s...@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.goldschm...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
