| Message ID | 2e8206d8bbd7d3140cf2734ca9b3fcd12ced4f14.1727968902.git.jerome.forissier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce the lwIP network stack | expand |
Hi Jerome, On Thu, 3 Oct 2024 at 18:23, Jerome Forissier <jerome.forissier@linaro.org> wrote: > > When DSA_SANDBOX is not set, the sandbox tests fail as follows: > > $ ./test/py/test.py --build-dir=$(pwd) -k bootdev_test_any > [...] > Scanning for bootflows with label '9' > [...] > Cannot find '9' (err=-19) > > This is due to the device list containing two less entries than > expected. Therefore, look for label '7' when DSA_SANDBOX is disabled. > > The actual use case is NET_LWIP=y (to be introduced in later patches) > which implies DSA_SANDBOX=n for the time being. > > Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> > --- > test/boot/bootflow.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c > index 6ad63afe90a..c440b8eb778 100644 > --- a/test/boot/bootflow.c > +++ b/test/boot/bootflow.c > @@ -109,9 +109,12 @@ static int bootflow_cmd_label(struct unit_test_state *uts) > * 8 [ ] OK mmc mmc2.bootdev > * 9 [ + ] OK mmc mmc1.bootdev > * a [ ] OK mmc mmc0.bootdev > + * > + * However with CONFIG_DSA_SANDBOX=n we have two less (dsa-test@0 and > + * dsa-test@1). > */ > - ut_assertok(run_command("bootflow scan -lH 9", 0)); > - ut_assert_nextline("Scanning for bootflows with label '9'"); Shouldn't this under and #ifdef, IS_ENABLED etc? > + ut_assertok(run_command("bootflow scan -lH 7", 0)); > + ut_assert_nextline("Scanning for bootflows with label '7'"); > ut_assert_skip_to_line("(1 bootflow, 1 valid)"); > > ut_assertok(run_command("bootflow scan -lH 0", 0)); > -- > 2.40.1 >
On 10/4/24 08:55, Ilias Apalodimas wrote: > Hi Jerome, > > On Thu, 3 Oct 2024 at 18:23, Jerome Forissier > <jerome.forissier@linaro.org> wrote: >> >> When DSA_SANDBOX is not set, the sandbox tests fail as follows: >> >> $ ./test/py/test.py --build-dir=$(pwd) -k bootdev_test_any >> [...] >> Scanning for bootflows with label '9' >> [...] >> Cannot find '9' (err=-19) >> >> This is due to the device list containing two less entries than >> expected. Therefore, look for label '7' when DSA_SANDBOX is disabled. >> >> The actual use case is NET_LWIP=y (to be introduced in later patches) >> which implies DSA_SANDBOX=n for the time being. >> >> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> >> --- >> test/boot/bootflow.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c >> index 6ad63afe90a..c440b8eb778 100644 >> --- a/test/boot/bootflow.c >> +++ b/test/boot/bootflow.c >> @@ -109,9 +109,12 @@ static int bootflow_cmd_label(struct unit_test_state *uts) >> * 8 [ ] OK mmc mmc2.bootdev >> * 9 [ + ] OK mmc mmc1.bootdev >> * a [ ] OK mmc mmc0.bootdev >> + * >> + * However with CONFIG_DSA_SANDBOX=n we have two less (dsa-test@0 and >> + * dsa-test@1). >> */ >> - ut_assertok(run_command("bootflow scan -lH 9", 0)); >> - ut_assert_nextline("Scanning for bootflows with label '9'"); > > Shouldn't this under and #ifdef, IS_ENABLED etc? In theory yes, but we can avoid the conditional by using index 7 which is always valid, i.e., in all configurations we have at least 7 devices (even 8 actually). > >> + ut_assertok(run_command("bootflow scan -lH 7", 0)); >> + ut_assert_nextline("Scanning for bootflows with label '7'"); >> ut_assert_skip_to_line("(1 bootflow, 1 valid)"); >> >> ut_assertok(run_command("bootflow scan -lH 0", 0)); >> -- >> 2.40.1 >>
On Fri, 4 Oct 2024 at 11:46, Jerome Forissier <jerome.forissier@linaro.org> wrote: > > > > On 10/4/24 08:55, Ilias Apalodimas wrote: > > Hi Jerome, > > > > On Thu, 3 Oct 2024 at 18:23, Jerome Forissier > > <jerome.forissier@linaro.org> wrote: > >> > >> When DSA_SANDBOX is not set, the sandbox tests fail as follows: > >> > >> $ ./test/py/test.py --build-dir=$(pwd) -k bootdev_test_any > >> [...] > >> Scanning for bootflows with label '9' > >> [...] > >> Cannot find '9' (err=-19) > >> > >> This is due to the device list containing two less entries than > >> expected. Therefore, look for label '7' when DSA_SANDBOX is disabled. > >> > >> The actual use case is NET_LWIP=y (to be introduced in later patches) > >> which implies DSA_SANDBOX=n for the time being. > >> > >> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> > >> --- > >> test/boot/bootflow.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c > >> index 6ad63afe90a..c440b8eb778 100644 > >> --- a/test/boot/bootflow.c > >> +++ b/test/boot/bootflow.c > >> @@ -109,9 +109,12 @@ static int bootflow_cmd_label(struct unit_test_state *uts) > >> * 8 [ ] OK mmc mmc2.bootdev > >> * 9 [ + ] OK mmc mmc1.bootdev > >> * a [ ] OK mmc mmc0.bootdev > >> + * > >> + * However with CONFIG_DSA_SANDBOX=n we have two less (dsa-test@0 and > >> + * dsa-test@1). > >> */ > >> - ut_assertok(run_command("bootflow scan -lH 9", 0)); > >> - ut_assert_nextline("Scanning for bootflows with label '9'"); > > > > Shouldn't this under and #ifdef, IS_ENABLED etc? > > In theory yes, but we can avoid the conditional by using index 7 which is always > valid, i.e., in all configurations we have at least 7 devices (even 8 actually). Ok, but I *think* Simon was trying to match the exact out put here, not 'at least 7'. I think we are better off being strict on this test Thanks /Ilias > > > > >> + ut_assertok(run_command("bootflow scan -lH 7", 0)); > >> + ut_assert_nextline("Scanning for bootflows with label '7'"); > >> ut_assert_skip_to_line("(1 bootflow, 1 valid)"); > >> > >> ut_assertok(run_command("bootflow scan -lH 0", 0)); > >> -- > >> 2.40.1 > >>
On 10/4/24 11:37, Ilias Apalodimas wrote: > On Fri, 4 Oct 2024 at 11:46, Jerome Forissier > <jerome.forissier@linaro.org> wrote: >> >> >> >> On 10/4/24 08:55, Ilias Apalodimas wrote: >>> Hi Jerome, >>> >>> On Thu, 3 Oct 2024 at 18:23, Jerome Forissier >>> <jerome.forissier@linaro.org> wrote: >>>> >>>> When DSA_SANDBOX is not set, the sandbox tests fail as follows: >>>> >>>> $ ./test/py/test.py --build-dir=$(pwd) -k bootdev_test_any >>>> [...] >>>> Scanning for bootflows with label '9' >>>> [...] >>>> Cannot find '9' (err=-19) >>>> >>>> This is due to the device list containing two less entries than >>>> expected. Therefore, look for label '7' when DSA_SANDBOX is disabled. >>>> >>>> The actual use case is NET_LWIP=y (to be introduced in later patches) >>>> which implies DSA_SANDBOX=n for the time being. >>>> >>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> >>>> --- >>>> test/boot/bootflow.c | 7 +++++-- >>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c >>>> index 6ad63afe90a..c440b8eb778 100644 >>>> --- a/test/boot/bootflow.c >>>> +++ b/test/boot/bootflow.c >>>> @@ -109,9 +109,12 @@ static int bootflow_cmd_label(struct unit_test_state *uts) >>>> * 8 [ ] OK mmc mmc2.bootdev >>>> * 9 [ + ] OK mmc mmc1.bootdev >>>> * a [ ] OK mmc mmc0.bootdev >>>> + * >>>> + * However with CONFIG_DSA_SANDBOX=n we have two less (dsa-test@0 and >>>> + * dsa-test@1). >>>> */ >>>> - ut_assertok(run_command("bootflow scan -lH 9", 0)); >>>> - ut_assert_nextline("Scanning for bootflows with label '9'"); >>> >>> Shouldn't this under and #ifdef, IS_ENABLED etc? >> >> In theory yes, but we can avoid the conditional by using index 7 which is always >> valid, i.e., in all configurations we have at least 7 devices (even 8 actually). > > Ok, but I *think* Simon was trying to match the exact out put here, > not 'at least 7'. > > I think we are better off being strict on this test No because there are 10 entries according to the comment ("a" hex being mmc0.bootdev). Simon, what do you suggest? Thanks,
Hi Jerome, On Fri, 4 Oct 2024 at 06:01, Jerome Forissier <jerome.forissier@linaro.org> wrote: > > > > On 10/4/24 11:37, Ilias Apalodimas wrote: > > On Fri, 4 Oct 2024 at 11:46, Jerome Forissier > > <jerome.forissier@linaro.org> wrote: > >> > >> > >> > >> On 10/4/24 08:55, Ilias Apalodimas wrote: > >>> Hi Jerome, > >>> > >>> On Thu, 3 Oct 2024 at 18:23, Jerome Forissier > >>> <jerome.forissier@linaro.org> wrote: > >>>> > >>>> When DSA_SANDBOX is not set, the sandbox tests fail as follows: > >>>> > >>>> $ ./test/py/test.py --build-dir=$(pwd) -k bootdev_test_any > >>>> [...] > >>>> Scanning for bootflows with label '9' > >>>> [...] > >>>> Cannot find '9' (err=-19) > >>>> > >>>> This is due to the device list containing two less entries than > >>>> expected. Therefore, look for label '7' when DSA_SANDBOX is disabled. > >>>> > >>>> The actual use case is NET_LWIP=y (to be introduced in later patches) > >>>> which implies DSA_SANDBOX=n for the time being. > >>>> > >>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> > >>>> --- > >>>> test/boot/bootflow.c | 7 +++++-- > >>>> 1 file changed, 5 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c > >>>> index 6ad63afe90a..c440b8eb778 100644 > >>>> --- a/test/boot/bootflow.c > >>>> +++ b/test/boot/bootflow.c > >>>> @@ -109,9 +109,12 @@ static int bootflow_cmd_label(struct unit_test_state *uts) > >>>> * 8 [ ] OK mmc mmc2.bootdev > >>>> * 9 [ + ] OK mmc mmc1.bootdev > >>>> * a [ ] OK mmc mmc0.bootdev > >>>> + * > >>>> + * However with CONFIG_DSA_SANDBOX=n we have two less (dsa-test@0 and > >>>> + * dsa-test@1). > >>>> */ > >>>> - ut_assertok(run_command("bootflow scan -lH 9", 0)); > >>>> - ut_assert_nextline("Scanning for bootflows with label '9'"); > >>> > >>> Shouldn't this under and #ifdef, IS_ENABLED etc? > >> > >> In theory yes, but we can avoid the conditional by using index 7 which is always > >> valid, i.e., in all configurations we have at least 7 devices (even 8 actually). > > > > Ok, but I *think* Simon was trying to match the exact out put here, > > not 'at least 7'. > > > > I think we are better off being strict on this test > > No because there are 10 entries according to the comment ("a" hex being > mmc0.bootdev). Simon, what do you suggest? I don't think this is a huge deal. Reviewed-by: Simon Glass <sjg@chromium.org> BTW, 'fewer', not 'less', if you can count them > > Thanks, > -- > Jerome > > > > > Thanks > > /Ilias > >> > >>> > >>>> + ut_assertok(run_command("bootflow scan -lH 7", 0)); > >>>> + ut_assert_nextline("Scanning for bootflows with label '7'"); > >>>> ut_assert_skip_to_line("(1 bootflow, 1 valid)"); > >>>> > >>>> ut_assertok(run_command("bootflow scan -lH 0", 0)); > >>>> -- > >>>> 2.40.1 > >>>>
On 10/7/24 17:23, Simon Glass wrote: > Hi Jerome, > > On Fri, 4 Oct 2024 at 06:01, Jerome Forissier > <jerome.forissier@linaro.org> wrote: >> >> >> >> On 10/4/24 11:37, Ilias Apalodimas wrote: >>> On Fri, 4 Oct 2024 at 11:46, Jerome Forissier >>> <jerome.forissier@linaro.org> wrote: >>>> >>>> >>>> >>>> On 10/4/24 08:55, Ilias Apalodimas wrote: >>>>> Hi Jerome, >>>>> >>>>> On Thu, 3 Oct 2024 at 18:23, Jerome Forissier >>>>> <jerome.forissier@linaro.org> wrote: >>>>>> >>>>>> When DSA_SANDBOX is not set, the sandbox tests fail as follows: >>>>>> >>>>>> $ ./test/py/test.py --build-dir=$(pwd) -k bootdev_test_any >>>>>> [...] >>>>>> Scanning for bootflows with label '9' >>>>>> [...] >>>>>> Cannot find '9' (err=-19) >>>>>> >>>>>> This is due to the device list containing two less entries than >>>>>> expected. Therefore, look for label '7' when DSA_SANDBOX is disabled. >>>>>> >>>>>> The actual use case is NET_LWIP=y (to be introduced in later patches) >>>>>> which implies DSA_SANDBOX=n for the time being. >>>>>> >>>>>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> >>>>>> --- >>>>>> test/boot/bootflow.c | 7 +++++-- >>>>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c >>>>>> index 6ad63afe90a..c440b8eb778 100644 >>>>>> --- a/test/boot/bootflow.c >>>>>> +++ b/test/boot/bootflow.c >>>>>> @@ -109,9 +109,12 @@ static int bootflow_cmd_label(struct unit_test_state *uts) >>>>>> * 8 [ ] OK mmc mmc2.bootdev >>>>>> * 9 [ + ] OK mmc mmc1.bootdev >>>>>> * a [ ] OK mmc mmc0.bootdev >>>>>> + * >>>>>> + * However with CONFIG_DSA_SANDBOX=n we have two less (dsa-test@0 and >>>>>> + * dsa-test@1). >>>>>> */ >>>>>> - ut_assertok(run_command("bootflow scan -lH 9", 0)); >>>>>> - ut_assert_nextline("Scanning for bootflows with label '9'"); >>>>> >>>>> Shouldn't this under and #ifdef, IS_ENABLED etc? >>>> >>>> In theory yes, but we can avoid the conditional by using index 7 which is always >>>> valid, i.e., in all configurations we have at least 7 devices (even 8 actually). >>> >>> Ok, but I *think* Simon was trying to match the exact out put here, >>> not 'at least 7'. >>> >>> I think we are better off being strict on this test >> >> No because there are 10 entries according to the comment ("a" hex being >> mmc0.bootdev). Simon, what do you suggest? > > I don't think this is a huge deal. > > Reviewed-by: Simon Glass <sjg@chromium.org> Unfortunately this patch breaks the default config (NET=y and therefore DSA_SANDBOX=y): => ut bootstd bootflow_cmd_label Test: bootflow_cmd_label: bootflow.c Scanning for bootflows with label 'mmc1' Seq Method State Uclass Part Name Filename --- ----------- ------ -------- ---- ------------------------ ---------------- Scanning bootdev 'mmc1.bootdev': 0 extlinux ready mmc 1 mmc1.bootdev.part_1 /extlinux/extlinux.conf No more bootdevs --- ----------- ------ -------- ---- ------------------------ ---------------- (1 bootflow, 1 valid) Scanning for bootflows with label '0' Seq Method State Uclass Part Name Filename --- ----------- ------ -------- ---- ------------------------ ---------------- --- ----------- ------ -------- ---- ------------------------ ---------------- (0 bootflows, 0 valid) Scanning for bootflows with label '7' Seq Method State Uclass Part Name Filename --- ----------- ------ -------- ---- ------------------------ ---------------- --- ----------- ------ -------- ---- ------------------------ ---------------- (0 bootflows, 0 valid) test/boot/bootflow.c:118, bootflow_cmd_label(): console: Expected '(1 bootflow, 1 valid)', got to '(0 bootflows, 0 valid)' [...] So for v12 I'll do what Ilias suggested: - ut_assertok(run_command("bootflow scan -lH 9", 0)); - ut_assert_nextline("Scanning for bootflows with label '9'"); + if (CONFIG_IS_ENABLED(DSA_SANDBOX)) { + ut_assertok(run_command("bootflow scan -lH 9", 0)); + ut_assert_nextline("Scanning for bootflows with label '9'"); + } else { + ut_assertok(run_command("bootflow scan -lH 7", 0)); + ut_assert_nextline("Scanning for bootflows with label '7'"); + } ut_assert_skip_to_line("(1 bootflow, 1 valid)"); Tested OK with DSA_SANDBOX=y as well as DSA_SANDBOX=n. > BTW, 'fewer', not 'less', if you can count them Sure :) Thanks,
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index 6ad63afe90a..c440b8eb778 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -109,9 +109,12 @@ static int bootflow_cmd_label(struct unit_test_state *uts) * 8 [ ] OK mmc mmc2.bootdev * 9 [ + ] OK mmc mmc1.bootdev * a [ ] OK mmc mmc0.bootdev + * + * However with CONFIG_DSA_SANDBOX=n we have two less (dsa-test@0 and + * dsa-test@1). */ - ut_assertok(run_command("bootflow scan -lH 9", 0)); - ut_assert_nextline("Scanning for bootflows with label '9'"); + ut_assertok(run_command("bootflow scan -lH 7", 0)); + ut_assert_nextline("Scanning for bootflows with label '7'"); ut_assert_skip_to_line("(1 bootflow, 1 valid)"); ut_assertok(run_command("bootflow scan -lH 0", 0));
When DSA_SANDBOX is not set, the sandbox tests fail as follows: $ ./test/py/test.py --build-dir=$(pwd) -k bootdev_test_any [...] Scanning for bootflows with label '9' [...] Cannot find '9' (err=-19) This is due to the device list containing two less entries than expected. Therefore, look for label '7' when DSA_SANDBOX is disabled. The actual use case is NET_LWIP=y (to be introduced in later patches) which implies DSA_SANDBOX=n for the time being. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> --- test/boot/bootflow.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)