diff mbox series

[v2,27/32] test: cedit: use allocated address for reading file

Message ID 20240814110009.45310-28-sughosh.ganu@linaro.org
State New
Headers show
Series Make LMB memory map global and persistent | expand

Commit Message

Sughosh Ganu Aug. 14, 2024, 11 a.m. UTC
Instead of a randomly selected address, use an LMB allocated one for
reading the file into memory. With the LMB map now being persistent
and global, the address used for reading the file might be already
allocated as non-overwritable, resulting in a failure. Get a valid
address from LMB and then read the file to that address.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V1:
* Don't use the API version with flags parameter.

 test/boot/cedit.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Simon Glass Aug. 15, 2024, 8:32 p.m. UTC | #1
Hi Sughosh,

On Wed, 14 Aug 2024 at 05:02, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Instead of a randomly selected address, use an LMB allocated one for
> reading the file into memory. With the LMB map now being persistent
> and global, the address used for reading the file might be already
> allocated as non-overwritable, resulting in a failure. Get a valid
> address from LMB and then read the file to that address.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V1:
> * Don't use the API version with flags parameter.
>
>  test/boot/cedit.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>

No, this address needs to work fine without using lmb. Same with any
other tests. Tests make use of the sandbox memory space memory
addresses and it makes things easier to code and debug.

Regards,
SImon
Sughosh Ganu Aug. 16, 2024, 10:34 a.m. UTC | #2
On Fri, 16 Aug 2024 at 02:02, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Wed, 14 Aug 2024 at 05:02, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > Instead of a randomly selected address, use an LMB allocated one for
> > reading the file into memory. With the LMB map now being persistent
> > and global, the address used for reading the file might be already
> > allocated as non-overwritable, resulting in a failure. Get a valid
> > address from LMB and then read the file to that address.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> > Changes since V1:
> > * Don't use the API version with flags parameter.
> >
> >  test/boot/cedit.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
>
> No, this address needs to work fine without using lmb. Same with any
> other tests. Tests make use of the sandbox memory space memory
> addresses and it makes things easier to code and debug.

Like I had explained earlier [1], not using the LMB API for allocating
the address results in issues, since the load command internally
checks if the address can be used for reading the dtb. Without this
patch, the cmd_ut test fails. I am not sure why you do not like this
solution. But in any case, can you propose some other solution? I
believe I can tweak the address to some other value, but that would
not be a proper solution, but simply kicking the can down the road.
Thanks.

-sughosh

[1] - https://lists.denx.de/pipermail/u-boot/2024-July/560569.html

>
> Regards,
> SImon
Simon Glass Aug. 16, 2024, 2:47 p.m. UTC | #3
Hi Sughosh,

On Fri, 16 Aug 2024 at 04:34, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Fri, 16 Aug 2024 at 02:02, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Wed, 14 Aug 2024 at 05:02, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > Instead of a randomly selected address, use an LMB allocated one for
> > > reading the file into memory. With the LMB map now being persistent
> > > and global, the address used for reading the file might be already
> > > allocated as non-overwritable, resulting in a failure. Get a valid
> > > address from LMB and then read the file to that address.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > > Changes since V1:
> > > * Don't use the API version with flags parameter.
> > >
> > >  test/boot/cedit.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> >
> > No, this address needs to work fine without using lmb. Same with any
> > other tests. Tests make use of the sandbox memory space memory
> > addresses and it makes things easier to code and debug.
>
> Like I had explained earlier [1], not using the LMB API for allocating
> the address results in issues, since the load command internally
> checks if the address can be used for reading the dtb. Without this
> patch, the cmd_ut test fails. I am not sure why you do not like this
> solution. But in any case, can you propose some other solution? I
> believe I can tweak the address to some other value, but that would
> not be a proper solution, but simply kicking the can down the road.
> Thanks.

But this is why I suggested having lmb_push() and lmb_pop(), so you
should be able to use those in the few tests which have this problem.

If it becomes more widespread and affects a lot of tests, we could do
thing automatically in the test framework, but for now, that doesn't
seem necessary.

>
> -sughosh
>
> [1] - https://lists.denx.de/pipermail/u-boot/2024-July/560569.html

Regards,
Simon
Sughosh Ganu Aug. 16, 2024, 5:09 p.m. UTC | #4
On Fri, 16 Aug 2024 at 20:17, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Fri, 16 Aug 2024 at 04:34, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Fri, 16 Aug 2024 at 02:02, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Wed, 14 Aug 2024 at 05:02, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > Instead of a randomly selected address, use an LMB allocated one for
> > > > reading the file into memory. With the LMB map now being persistent
> > > > and global, the address used for reading the file might be already
> > > > allocated as non-overwritable, resulting in a failure. Get a valid
> > > > address from LMB and then read the file to that address.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > > Changes since V1:
> > > > * Don't use the API version with flags parameter.
> > > >
> > > >  test/boot/cedit.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > >
> > > No, this address needs to work fine without using lmb. Same with any
> > > other tests. Tests make use of the sandbox memory space memory
> > > addresses and it makes things easier to code and debug.
> >
> > Like I had explained earlier [1], not using the LMB API for allocating
> > the address results in issues, since the load command internally
> > checks if the address can be used for reading the dtb. Without this
> > patch, the cmd_ut test fails. I am not sure why you do not like this
> > solution. But in any case, can you propose some other solution? I
> > believe I can tweak the address to some other value, but that would
> > not be a proper solution, but simply kicking the can down the road.
> > Thanks.
>
> But this is why I suggested having lmb_push() and lmb_pop(), so you
> should be able to use those in the few tests which have this problem.

Ah yes, I forgot about that change. Let me try running the test again
and see what I get. Thanks.

-sughosh

>
> If it becomes more widespread and affects a lot of tests, we could do
> thing automatically in the test framework, but for now, that doesn't
> seem necessary.
>
> >
> > -sughosh
> >
> > [1] - https://lists.denx.de/pipermail/u-boot/2024-July/560569.html
>
> Regards,
> Simon
Sughosh Ganu Aug. 16, 2024, 5:53 p.m. UTC | #5
On Fri, 16 Aug 2024 at 22:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Fri, 16 Aug 2024 at 20:17, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Fri, 16 Aug 2024 at 04:34, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > On Fri, 16 Aug 2024 at 02:02, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Wed, 14 Aug 2024 at 05:02, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > Instead of a randomly selected address, use an LMB allocated one for
> > > > > reading the file into memory. With the LMB map now being persistent
> > > > > and global, the address used for reading the file might be already
> > > > > allocated as non-overwritable, resulting in a failure. Get a valid
> > > > > address from LMB and then read the file to that address.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > > Changes since V1:
> > > > > * Don't use the API version with flags parameter.
> > > > >
> > > > >  test/boot/cedit.c | 6 +++++-
> > > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > >
> > > >
> > > > No, this address needs to work fine without using lmb. Same with any
> > > > other tests. Tests make use of the sandbox memory space memory
> > > > addresses and it makes things easier to code and debug.
> > >
> > > Like I had explained earlier [1], not using the LMB API for allocating
> > > the address results in issues, since the load command internally
> > > checks if the address can be used for reading the dtb. Without this
> > > patch, the cmd_ut test fails. I am not sure why you do not like this
> > > solution. But in any case, can you propose some other solution? I
> > > believe I can tweak the address to some other value, but that would
> > > not be a proper solution, but simply kicking the can down the road.
> > > Thanks.
> >
> > But this is why I suggested having lmb_push() and lmb_pop(), so you
> > should be able to use those in the few tests which have this problem.
>
> Ah yes, I forgot about that change. Let me try running the test again
> and see what I get. Thanks.

Sorry, I think I was hasty in my reply. This would not work. We have
the lmb_push()/lmb_get() functions only for the lmb specific tests. In
this scenario, we have the load command which checks the lmb address
against reservations, which are the "normal operation" memory map. So,
if we have to have this to work, this might need a way of using a new
lmb structure instance for every test. So that every test that gets
invoked, does so like the way the lmb tests are.

-sughosh

>
> -sughosh
>
> >
> > If it becomes more widespread and affects a lot of tests, we could do
> > thing automatically in the test framework, but for now, that doesn't
> > seem necessary.
> >
> > >
> > > -sughosh
> > >
> > > [1] - https://lists.denx.de/pipermail/u-boot/2024-July/560569.html
> >
> > Regards,
> > Simon
Tom Rini Aug. 16, 2024, 6:06 p.m. UTC | #6
On Thu, Aug 15, 2024 at 09:32:29PM +0100, Simon Glass wrote:
> Hi Sughosh,
> 
> On Wed, 14 Aug 2024 at 05:02, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > Instead of a randomly selected address, use an LMB allocated one for
> > reading the file into memory. With the LMB map now being persistent
> > and global, the address used for reading the file might be already
> > allocated as non-overwritable, resulting in a failure. Get a valid
> > address from LMB and then read the file to that address.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> > Changes since V1:
> > * Don't use the API version with flags parameter.
> >
> >  test/boot/cedit.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> 
> No, this address needs to work fine without using lmb. Same with any
> other tests. Tests make use of the sandbox memory space memory
> addresses and it makes things easier to code and debug.

Can't we just request/free that address from LMB then?
Sughosh Ganu Aug. 16, 2024, 6:28 p.m. UTC | #7
On Fri, 16 Aug 2024 at 11:36 PM, Tom Rini <trini@konsulko.com> wrote:

> On Thu, Aug 15, 2024 at 09:32:29PM +0100, Simon Glass wrote:
> > Hi Sughosh,
> >
> > On Wed, 14 Aug 2024 at 05:02, Sughosh Ganu <sughosh.ganu@linaro.org>
> wrote:
> > >
> > > Instead of a randomly selected address, use an LMB allocated one for
> > > reading the file into memory. With the LMB map now being persistent
> > > and global, the address used for reading the file might be already
> > > allocated as non-overwritable, resulting in a failure. Get a valid
> > > address from LMB and then read the file to that address.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > > Changes since V1:
> > > * Don't use the API version with flags parameter.
> > >
> > >  test/boot/cedit.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> >
> > No, this address needs to work fine without using lmb. Same with any
> > other tests. Tests make use of the sandbox memory space memory
> > addresses and it makes things easier to code and debug.
>
> Can't we just request/free that address from LMB then?


Very good point. Because that is precisely what this patch does :)

-sughosh


>
> --
> Tom
>
Tom Rini Aug. 16, 2024, 6:35 p.m. UTC | #8
On Fri, Aug 16, 2024 at 11:58:46PM +0530, Sughosh Ganu wrote:
> On Fri, 16 Aug 2024 at 11:36 PM, Tom Rini <trini@konsulko.com> wrote:
> 
> > On Thu, Aug 15, 2024 at 09:32:29PM +0100, Simon Glass wrote:
> > > Hi Sughosh,
> > >
> > > On Wed, 14 Aug 2024 at 05:02, Sughosh Ganu <sughosh.ganu@linaro.org>
> > wrote:
> > > >
> > > > Instead of a randomly selected address, use an LMB allocated one for
> > > > reading the file into memory. With the LMB map now being persistent
> > > > and global, the address used for reading the file might be already
> > > > allocated as non-overwritable, resulting in a failure. Get a valid
> > > > address from LMB and then read the file to that address.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > > Changes since V1:
> > > > * Don't use the API version with flags parameter.
> > > >
> > > >  test/boot/cedit.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > >
> > > No, this address needs to work fine without using lmb. Same with any
> > > other tests. Tests make use of the sandbox memory space memory
> > > addresses and it makes things easier to code and debug.
> >
> > Can't we just request/free that address from LMB then?
> 
> 
> Very good point. Because that is precisely what this patch does :)

Ah, I think the wording is unclear then in the commit message. It needs
to reflect that we're taking a previously intentional address and now
reserving/releasing it with LMB. And perhaps not change from hex to
decimal too.
Simon Glass Aug. 16, 2024, 11:53 p.m. UTC | #9
Hi Sughosh,

On Fri, 16 Aug 2024 at 11:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Fri, 16 Aug 2024 at 22:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Fri, 16 Aug 2024 at 20:17, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Fri, 16 Aug 2024 at 04:34, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > On Fri, 16 Aug 2024 at 02:02, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Wed, 14 Aug 2024 at 05:02, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > Instead of a randomly selected address, use an LMB allocated one for
> > > > > > reading the file into memory. With the LMB map now being persistent
> > > > > > and global, the address used for reading the file might be already
> > > > > > allocated as non-overwritable, resulting in a failure. Get a valid
> > > > > > address from LMB and then read the file to that address.
> > > > > >
> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > ---
> > > > > > Changes since V1:
> > > > > > * Don't use the API version with flags parameter.
> > > > > >
> > > > > >  test/boot/cedit.c | 6 +++++-
> > > > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > > >
> > > > >
> > > > > No, this address needs to work fine without using lmb. Same with any
> > > > > other tests. Tests make use of the sandbox memory space memory
> > > > > addresses and it makes things easier to code and debug.
> > > >
> > > > Like I had explained earlier [1], not using the LMB API for allocating
> > > > the address results in issues, since the load command internally
> > > > checks if the address can be used for reading the dtb. Without this
> > > > patch, the cmd_ut test fails. I am not sure why you do not like this
> > > > solution. But in any case, can you propose some other solution? I
> > > > believe I can tweak the address to some other value, but that would
> > > > not be a proper solution, but simply kicking the can down the road.
> > > > Thanks.
> > >
> > > But this is why I suggested having lmb_push() and lmb_pop(), so you
> > > should be able to use those in the few tests which have this problem.
> >
> > Ah yes, I forgot about that change. Let me try running the test again
> > and see what I get. Thanks.
>
> Sorry, I think I was hasty in my reply. This would not work. We have
> the lmb_push()/lmb_get() functions only for the lmb specific tests. In
> this scenario, we have the load command which checks the lmb address
> against reservations, which are the "normal operation" memory map. So,
> if we have to have this to work, this might need a way of using a new
> lmb structure instance for every test. So that every test that gets
> invoked, does so like the way the lmb tests are.

Why does the 'load' operation fail? When I revert this patch the tests
still seem to pass.

Regards,
Simon


>
> -sughosh
>
> >
> > -sughosh
> >
> > >
> > > If it becomes more widespread and affects a lot of tests, we could do
> > > thing automatically in the test framework, but for now, that doesn't
> > > seem necessary.
> > >
> > > >
> > > > -sughosh
> > > >
> > > > [1] - https://lists.denx.de/pipermail/u-boot/2024-July/560569.html
> > >
> > > Regards,
> > > Simon
Sughosh Ganu Aug. 17, 2024, 10:05 a.m. UTC | #10
On Sat, 17 Aug 2024 at 05:24, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Fri, 16 Aug 2024 at 11:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Fri, 16 Aug 2024 at 22:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > On Fri, 16 Aug 2024 at 20:17, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Fri, 16 Aug 2024 at 04:34, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > On Fri, 16 Aug 2024 at 02:02, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Wed, 14 Aug 2024 at 05:02, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > Instead of a randomly selected address, use an LMB allocated one for
> > > > > > > reading the file into memory. With the LMB map now being persistent
> > > > > > > and global, the address used for reading the file might be already
> > > > > > > allocated as non-overwritable, resulting in a failure. Get a valid
> > > > > > > address from LMB and then read the file to that address.
> > > > > > >
> > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > ---
> > > > > > > Changes since V1:
> > > > > > > * Don't use the API version with flags parameter.
> > > > > > >
> > > > > > >  test/boot/cedit.c | 6 +++++-
> > > > > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > >
> > > > > > No, this address needs to work fine without using lmb. Same with any
> > > > > > other tests. Tests make use of the sandbox memory space memory
> > > > > > addresses and it makes things easier to code and debug.
> > > > >
> > > > > Like I had explained earlier [1], not using the LMB API for allocating
> > > > > the address results in issues, since the load command internally
> > > > > checks if the address can be used for reading the dtb. Without this
> > > > > patch, the cmd_ut test fails. I am not sure why you do not like this
> > > > > solution. But in any case, can you propose some other solution? I
> > > > > believe I can tweak the address to some other value, but that would
> > > > > not be a proper solution, but simply kicking the can down the road.
> > > > > Thanks.
> > > >
> > > > But this is why I suggested having lmb_push() and lmb_pop(), so you
> > > > should be able to use those in the few tests which have this problem.
> > >
> > > Ah yes, I forgot about that change. Let me try running the test again
> > > and see what I get. Thanks.
> >
> > Sorry, I think I was hasty in my reply. This would not work. We have
> > the lmb_push()/lmb_get() functions only for the lmb specific tests. In
> > this scenario, we have the load command which checks the lmb address
> > against reservations, which are the "normal operation" memory map. So,
> > if we have to have this to work, this might need a way of using a new
> > lmb structure instance for every test. So that every test that gets
> > invoked, does so like the way the lmb tests are.
>
> Why does the 'load' operation fail? When I revert this patch the tests
> still seem to pass.

I had not run the tests then. I thought that the address might
conflict with some existing reservation made with a different flag. I
have run the tests, and I too see that the CI goes through even with
this patch dropped. I still think that getting an address from LMB and
then using it is more robust, but then the same flow is being used in
many other tests as well. This should work as long as we don't get any
conflicting reservation with a different flag. I will drop this patch
in the next version. Thanks.

-sughosh

>
> Regards,
> Simon
>
>
> >
> > -sughosh
> >
> > >
> > > -sughosh
> > >
> > > >
> > > > If it becomes more widespread and affects a lot of tests, we could do
> > > > thing automatically in the test framework, but for now, that doesn't
> > > > seem necessary.
> > > >
> > > > >
> > > > > -sughosh
> > > > >
> > > > > [1] - https://lists.denx.de/pipermail/u-boot/2024-July/560569.html
> > > >
> > > > Regards,
> > > > Simon
Simon Glass Aug. 17, 2024, 3:57 p.m. UTC | #11
Hi Sughosh,

On Sat, 17 Aug 2024 at 04:06, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Sat, 17 Aug 2024 at 05:24, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Fri, 16 Aug 2024 at 11:53, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > On Fri, 16 Aug 2024 at 22:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > On Fri, 16 Aug 2024 at 20:17, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Fri, 16 Aug 2024 at 04:34, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > On Fri, 16 Aug 2024 at 02:02, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Sughosh,
> > > > > > >
> > > > > > > On Wed, 14 Aug 2024 at 05:02, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Instead of a randomly selected address, use an LMB allocated one for
> > > > > > > > reading the file into memory. With the LMB map now being persistent
> > > > > > > > and global, the address used for reading the file might be already
> > > > > > > > allocated as non-overwritable, resulting in a failure. Get a valid
> > > > > > > > address from LMB and then read the file to that address.
> > > > > > > >
> > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > > ---
> > > > > > > > Changes since V1:
> > > > > > > > * Don't use the API version with flags parameter.
> > > > > > > >
> > > > > > > >  test/boot/cedit.c | 6 +++++-
> > > > > > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > >
> > > > > > > No, this address needs to work fine without using lmb. Same with any
> > > > > > > other tests. Tests make use of the sandbox memory space memory
> > > > > > > addresses and it makes things easier to code and debug.
> > > > > >
> > > > > > Like I had explained earlier [1], not using the LMB API for allocating
> > > > > > the address results in issues, since the load command internally
> > > > > > checks if the address can be used for reading the dtb. Without this
> > > > > > patch, the cmd_ut test fails. I am not sure why you do not like this
> > > > > > solution. But in any case, can you propose some other solution? I
> > > > > > believe I can tweak the address to some other value, but that would
> > > > > > not be a proper solution, but simply kicking the can down the road.
> > > > > > Thanks.
> > > > >
> > > > > But this is why I suggested having lmb_push() and lmb_pop(), so you
> > > > > should be able to use those in the few tests which have this problem.
> > > >
> > > > Ah yes, I forgot about that change. Let me try running the test again
> > > > and see what I get. Thanks.
> > >
> > > Sorry, I think I was hasty in my reply. This would not work. We have
> > > the lmb_push()/lmb_get() functions only for the lmb specific tests. In
> > > this scenario, we have the load command which checks the lmb address
> > > against reservations, which are the "normal operation" memory map. So,
> > > if we have to have this to work, this might need a way of using a new
> > > lmb structure instance for every test. So that every test that gets
> > > invoked, does so like the way the lmb tests are.
> >
> > Why does the 'load' operation fail? When I revert this patch the tests
> > still seem to pass.
>
> I had not run the tests then. I thought that the address might
> conflict with some existing reservation made with a different flag. I
> have run the tests, and I too see that the CI goes through even with
> this patch dropped. I still think that getting an address from LMB and
> then using it is more robust, but then the same flow is being used in
> many other tests as well. This should work as long as we don't get any
> conflicting reservation with a different flag. I will drop this patch
> in the next version. Thanks.

OK, well let's see if it does cause problems later. We can use the
push/pop feature if needed down the track.

> > > > > If it becomes more widespread and affects a lot of tests, we could do
> > > > > thing automatically in the test framework, but for now, that doesn't
> > > > > seem necessary.
> > > > >
> > > > > > [1] - https://lists.denx.de/pipermail/u-boot/2024-July/560569.html

Regards,
Simon
diff mbox series

Patch

diff --git a/test/boot/cedit.c b/test/boot/cedit.c
index fd19da0a0c..923ddd1481 100644
--- a/test/boot/cedit.c
+++ b/test/boot/cedit.c
@@ -7,6 +7,7 @@ 
 #include <cedit.h>
 #include <env.h>
 #include <expo.h>
+#include <lmb.h>
 #include <mapmem.h>
 #include <dm/ofnode.h>
 #include <test/ut.h>
@@ -61,7 +62,7 @@  static int cedit_fdt(struct unit_test_state *uts)
 	struct video_priv *vid_priv;
 	extern struct expo *cur_exp;
 	struct scene_obj_menu *menu;
-	ulong addr = 0x1000;
+	ulong addr;
 	struct ofprop prop;
 	struct scene *scn;
 	oftree tree;
@@ -86,6 +87,8 @@  static int cedit_fdt(struct unit_test_state *uts)
 	str = abuf_data(&tline->buf);
 	strcpy(str, "my-machine");
 
+	addr = lmb_alloc(1024, 1024);
+	ut_asserteq(!!addr, !0);
 	ut_assertok(run_command("cedit write_fdt hostfs - settings.dtb", 0));
 	ut_assertok(run_commandf("load hostfs - %lx settings.dtb", addr));
 	ut_assert_nextlinen("1024 bytes read");
@@ -94,6 +97,7 @@  static int cedit_fdt(struct unit_test_state *uts)
 	tree = oftree_from_fdt(fdt);
 	node = ofnode_find_subnode(oftree_root(tree), CEDIT_NODE_NAME);
 	ut_assert(ofnode_valid(node));
+	lmb_free(addr, 1024);
 
 	ut_asserteq(ID_CPU_SPEED_2,
 		    ofnode_read_u32_default(node, "cpu-speed", 0));