Hi Simon,
On Mon, 14 Oct 2019 at 14:05, Simon Goldschmidt simon.k.r.goldschmidt@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@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@gmail.com wrote:
Simon Glass sjg@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@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