[U-Boot] IS_ERR_VALUE failing on socfpga gen5
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.
Being like that, the current test for error (value >= -4095) which equals 'value >= 0xfffff001' would require us to waste the last 4K of that memory, which is not an option when it comes to using DM in SPL...
Now on this platform, I'm sure I could find a range of 4096 bytes to use as invalid pointer range, but the question is: how do I integrate that into U-Boot in a clean manner? Would it be acceptable to change the error headers imported from Linux or would I have to through the hassle of changing Linux first?
Or does anyone have a better idea to allow using the last 4K for heap?
Regards, Simon
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?
Being like that, the current test for error (value >= -4095) which equals 'value >= 0xfffff001' would require us to waste the last 4K of that memory, which is not an option when it comes to using DM in SPL...
Now on this platform, I'm sure I could find a range of 4096 bytes to use as invalid pointer range, but the question is: how do I integrate that into U-Boot in a clean manner? Would it be acceptable to change the error headers imported from Linux or would I have to through the hassle of changing Linux first?
Or does anyone have a better idea to allow using the last 4K for heap?
Regards, Simon
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"...
Regards, Simon
Being like that, the current test for error (value >= -4095) which equals 'value >= 0xfffff001' would require us to waste the last 4K of that memory, which is not an option when it comes to using DM in SPL...
Now on this platform, I'm sure I could find a range of 4096 bytes to use as invalid pointer range, but the question is: how do I integrate that into U-Boot in a clean manner? Would it be acceptable to change the error headers imported from Linux or would I have to through the hassle of changing Linux first?
Or does anyone have a better idea to allow using the last 4K for heap?
Regards, Simon
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.
Regards, Simon
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...?
Tom?
Regards, Simon
Regards, Simon
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.
Tom?
Regards, Simon
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.
Regards, Simon
Tom?
Regards, Simon
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
participants (2)
-
Simon Glass -
Simon Goldschmidt