| Message ID | 20180519121355.18377-6-erosca@de.adit-jv.com |
|---|---|
| State | Accepted |
| Commit | 507cef3d15bee1df074e03d77e5335355463b17b |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,v3,1/6] kconfig: re-sync with Linux 4.17-rc4 | expand |
Hi Eugeniu, On 19 May 2018 at 06:13, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > After importing v4.17-rc1 Linux commit 9130ba884640 ("scripts/dtc: > Update to upstream version v1.4.6-9-gaadd0b65c987"), sandbox build > reports below warnings: > > arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' > arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' > arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' > arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' > > Silent them by applying the 's/_/-/' substitution in the names of the > 'fdt_dummy0', 'fdt_dummy1', 'fdt_dummy2', 'fdt_dummy3' properties. > > Similar DTC warnings have been recently fixed in Linux kernel, e.g. via > v4.17-rc1 commit d366c30d19f4 ("ARM: dts: STi: Fix aliases property name > for STi boards"). > > If done alone, the DTS update generates a failure of the > `ut dm fdt_translation` unit test in sandbox environment as seen below: > > $ ./u-boot -d arch/sandbox/dts/test.dtb > ---<-snip->--- > => ut dm fdt_translation > Test: dm_test_fdt_translation: test-fdt.c > test/dm/test-fdt.c:444, dm_test_fdt_translation(): 0 == uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 0, 1, &dev): Expected 0, got -19 > Test: dm_test_fdt_translation: test-fdt.c (flat tree) > test/dm/test-fdt.c:444, dm_test_fdt_translation(): 0 == uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 0, 1, &dev): Expected 0, got -19 > Failures: 2 > ---<-snip->--- > > Fix this issue in place, by updating the "name" string in the > UCLASS_DRIVER(fdt_dummy) definition, so that it matches the newly > updated aliases properties. After that, the test passes: > > $ ./u-boot -d arch/sandbox/dts/test.dtb > ---<-snip->--- > => ut dm fdt_translation > Test: dm_test_fdt_translation: test-fdt.c > Test: dm_test_fdt_translation: test-fdt.c (flat tree) > Failures: 0 > ---<-snip->--- > > Fixes: e8d5291824e2 ("core: ofnode: Fix translation for #size-cells == 0") > Reported-by: Petr Vorel <pvorel@suse.cz> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> > --- > > v2->v3: > * Fixed an issue in the test code (test/dm/test-fdt.c) generated by the > DTS update (arch/sandbox/dts/test.dts) in [PATCH v2]. > * Changed commit summary line, to cover test/dm/test-fdt.c. > * Added: Reported-by: Petr Vorel <pvorel@suse.cz> > * [Due to update] Dropped: Reviewed-by: Simon Glass <sjg@chromium.org> > > v1->v2: > * Newly pushed > > arch/sandbox/dts/test.dts | 8 ++++---- > test/dm/test-fdt.c | 2 +- > 2 files changed, 5 insertions(+), 5 deletions(-) > Reviewed-by: Simon Glass <sjg@chromium.org> See below > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts > index 683b1970e0af..3e87c5c0f3fd 100644 > --- a/arch/sandbox/dts/test.dts > +++ b/arch/sandbox/dts/test.dts > @@ -27,10 +27,10 @@ > testfdt3 = "/b-test"; > testfdt5 = "/some-bus/c-test@5"; > testfdt8 = "/a-test"; > - fdt_dummy0 = "/translation-test@8000/dev@0,0"; > - fdt_dummy1 = "/translation-test@8000/dev@1,100"; > - fdt_dummy2 = "/translation-test@8000/dev@2,200"; > - fdt_dummy3 = "/translation-test@8000/noxlatebus@3,300/dev@42"; > + fdt-dummy0 = "/translation-test@8000/dev@0,0"; > + fdt-dummy1 = "/translation-test@8000/dev@1,100"; > + fdt-dummy2 = "/translation-test@8000/dev@2,200"; > + fdt-dummy3 = "/translation-test@8000/noxlatebus@3,300/dev@42"; > usb0 = &usb_0; > usb1 = &usb_1; > usb2 = &usb_2; > diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c > index 8196844e89a7..66d0df5629a2 100644 > --- a/test/dm/test-fdt.c > +++ b/test/dm/test-fdt.c > @@ -425,7 +425,7 @@ static const struct udevice_id fdt_dummy_ids[] = { > }; > > UCLASS_DRIVER(fdt_dummy) = { > - .name = "fdt_dummy", > + .name = "fdt-dummy", You should not need to change this one, and I worry that it is confusing since this is the driver name, not a compatible string. > .id = UCLASS_TEST_DUMMY, > .flags = DM_UC_FLAG_SEQ_ALIAS, > }; > -- > 2.17.0 > Regards, Simon
Hi Simon, On Tue, May 22, 2018 at 05:30:40PM -0600, Simon Glass wrote: > Hi Eugeniu, > > On 19 May 2018 at 06:13, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: --snip-- > > v2->v3: > > * Fixed an issue in the test code (test/dm/test-fdt.c) generated by the > > DTS update (arch/sandbox/dts/test.dts) in [PATCH v2]. > > * Changed commit summary line, to cover test/dm/test-fdt.c. > > * Added: Reported-by: Petr Vorel <pvorel@suse.cz> > > * [Due to update] Dropped: Reviewed-by: Simon Glass <sjg@chromium.org> > > > > v1->v2: > > * Newly pushed > > > > arch/sandbox/dts/test.dts | 8 ++++---- > > test/dm/test-fdt.c | 2 +- > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > See below > > > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts > > index 683b1970e0af..3e87c5c0f3fd 100644 > > --- a/arch/sandbox/dts/test.dts > > +++ b/arch/sandbox/dts/test.dts > > @@ -27,10 +27,10 @@ > > testfdt3 = "/b-test"; > > testfdt5 = "/some-bus/c-test@5"; > > testfdt8 = "/a-test"; > > - fdt_dummy0 = "/translation-test@8000/dev@0,0"; > > - fdt_dummy1 = "/translation-test@8000/dev@1,100"; > > - fdt_dummy2 = "/translation-test@8000/dev@2,200"; > > - fdt_dummy3 = "/translation-test@8000/noxlatebus@3,300/dev@42"; > > + fdt-dummy0 = "/translation-test@8000/dev@0,0"; > > + fdt-dummy1 = "/translation-test@8000/dev@1,100"; > > + fdt-dummy2 = "/translation-test@8000/dev@2,200"; > > + fdt-dummy3 = "/translation-test@8000/noxlatebus@3,300/dev@42"; > > usb0 = &usb_0; > > usb1 = &usb_1; > > usb2 = &usb_2; > > diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c > > index 8196844e89a7..66d0df5629a2 100644 > > --- a/test/dm/test-fdt.c > > +++ b/test/dm/test-fdt.c > > @@ -425,7 +425,7 @@ static const struct udevice_id fdt_dummy_ids[] = { > > }; > > > > UCLASS_DRIVER(fdt_dummy) = { > > - .name = "fdt_dummy", > > + .name = "fdt-dummy", > > You should not need to change this one, and I worry that it is > confusing since this is the driver name, not a compatible string. I am not familiar with the in-tree U-boot unit tests, but doing a small experiment I get clear evidence that there is a tight relationship/dependency between the name of the entries in the aliases DTS node and the "name" field of UCLASS_DRIVER using that DTS. The experiment is running 'ut dm' in sandbox before and after below patch (I also had to fix two null pointer dereferences in test/dm/bus.c to avoid segmentation faults with the patch applied): diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c index 8196844e89a7..5b715e965269 100644 --- a/test/dm/test-fdt.c +++ b/test/dm/test-fdt.c @@ -94,7 +94,7 @@ int testfdt_ping(struct udevice *dev, int pingval, int *pingret) } UCLASS_DRIVER(testfdt) = { - .name = "testfdt", + .name = "test_fdt", .id = UCLASS_TEST_FDT, .flags = DM_UC_FLAG_SEQ_ALIAS, }; Before the patch, I get: Failures: 9 After the patch, I get: Failures: 25 So, while the aliases entries are certainly not compatible strings, not keeping them aligned to UCLASS_DRIVER->name values leads to test failures. I am not sure if this is something specific to architecture of the tests or applies generically to any driver which fetches its configuration from DTS. I was hoping to get an assessment from somebody with more experience in this area. Besides the above, it is not clear to me if your Reviewed-by applies to to this patch partially (since you expressed some concerns) or applies globally, in which case the concerns are not major? > > > .id = UCLASS_TEST_DUMMY, > > .flags = DM_UC_FLAG_SEQ_ALIAS, > > }; > > -- > > 2.17.0 > > > > Regards, > Simon Best regards, Eugeniu.
Hi, On 24 May 2018 at 16:04, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > Hi Simon, > > On Tue, May 22, 2018 at 05:30:40PM -0600, Simon Glass wrote: >> Hi Eugeniu, >> >> On 19 May 2018 at 06:13, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > --snip-- > >> > v2->v3: >> > * Fixed an issue in the test code (test/dm/test-fdt.c) generated by the >> > DTS update (arch/sandbox/dts/test.dts) in [PATCH v2]. >> > * Changed commit summary line, to cover test/dm/test-fdt.c. >> > * Added: Reported-by: Petr Vorel <pvorel@suse.cz> >> > * [Due to update] Dropped: Reviewed-by: Simon Glass <sjg@chromium.org> >> > >> > v1->v2: >> > * Newly pushed >> > >> > arch/sandbox/dts/test.dts | 8 ++++---- >> > test/dm/test-fdt.c | 2 +- >> > 2 files changed, 5 insertions(+), 5 deletions(-) >> > >> >> Reviewed-by: Simon Glass <sjg@chromium.org> >> >> See below >> >> > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts >> > index 683b1970e0af..3e87c5c0f3fd 100644 >> > --- a/arch/sandbox/dts/test.dts >> > +++ b/arch/sandbox/dts/test.dts >> > @@ -27,10 +27,10 @@ >> > testfdt3 = "/b-test"; >> > testfdt5 = "/some-bus/c-test@5"; >> > testfdt8 = "/a-test"; >> > - fdt_dummy0 = "/translation-test@8000/dev@0,0"; >> > - fdt_dummy1 = "/translation-test@8000/dev@1,100"; >> > - fdt_dummy2 = "/translation-test@8000/dev@2,200"; >> > - fdt_dummy3 = "/translation-test@8000/noxlatebus@3,300/dev@42"; >> > + fdt-dummy0 = "/translation-test@8000/dev@0,0"; >> > + fdt-dummy1 = "/translation-test@8000/dev@1,100"; >> > + fdt-dummy2 = "/translation-test@8000/dev@2,200"; >> > + fdt-dummy3 = "/translation-test@8000/noxlatebus@3,300/dev@42"; >> > usb0 = &usb_0; >> > usb1 = &usb_1; >> > usb2 = &usb_2; >> > diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c >> > index 8196844e89a7..66d0df5629a2 100644 >> > --- a/test/dm/test-fdt.c >> > +++ b/test/dm/test-fdt.c >> > @@ -425,7 +425,7 @@ static const struct udevice_id fdt_dummy_ids[] = { >> > }; >> > >> > UCLASS_DRIVER(fdt_dummy) = { >> > - .name = "fdt_dummy", >> > + .name = "fdt-dummy", >> >> You should not need to change this one, and I worry that it is >> confusing since this is the driver name, not a compatible string. > > I am not familiar with the in-tree U-boot unit tests, but doing a small > experiment I get clear evidence that there is a tight > relationship/dependency between the name of the entries in the aliases > DTS node and the "name" field of UCLASS_DRIVER using that DTS. Yes you are right - the alias uses the uclass name, sorry. > > The experiment is running 'ut dm' in sandbox before and after below > patch (I also had to fix two null pointer dereferences in > test/dm/bus.c to avoid segmentation faults with the patch applied): > > diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c > index 8196844e89a7..5b715e965269 100644 > --- a/test/dm/test-fdt.c > +++ b/test/dm/test-fdt.c > @@ -94,7 +94,7 @@ int testfdt_ping(struct udevice *dev, int pingval, int *pingret) > } > > UCLASS_DRIVER(testfdt) = { > - .name = "testfdt", > + .name = "test_fdt", > .id = UCLASS_TEST_FDT, > .flags = DM_UC_FLAG_SEQ_ALIAS, > }; > > Before the patch, I get: Failures: 9 > After the patch, I get: Failures: 25 > > So, while the aliases entries are certainly not compatible strings, > not keeping them aligned to UCLASS_DRIVER->name values leads to test > failures. I am not sure if this is something specific to architecture of > the tests or applies generically to any driver which fetches its > configuration from DTS. I was hoping to get an assessment from somebody > with more experience in this area. > > Besides the above, it is not clear to me if your Reviewed-by applies to > to this patch partially (since you expressed some concerns) or applies > globally, in which case the concerns are not major? It means that I've reviewed the patch and I'd like some changes, but don't want to review it after you have made those changes, so you should add the Reviewed-by tag when doing the next version. But in this case your change is correct, so please don't worry. It's unfortunate that the uclass name needs a hypen, but I understand why. Regards, Simon
Hi Simon, On Fri, May 25, 2018 at 08:07:37PM -0600, Simon Glass wrote: > Hi, > > On 24 May 2018 at 16:04, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > > Besides the above, it is not clear to me if your Reviewed-by applies to > > to this patch partially (since you expressed some concerns) or applies > > globally, in which case the concerns are not major? > > It means that I've reviewed the patch and I'd like some changes, but > don't want to review it after you have made those changes, so you > should add the Reviewed-by tag when doing the next version. > > But in this case your change is correct, so please don't worry. It's > unfortunate that the uclass name needs a hypen, but I understand why. Thanks for shedding light both on technical aspects and the review process itself, which is very helpful. Regarding the state of the whole patch-set, I think we reached a point in which all the review comments have been handled. Please, let me know if you still see any open topics. Otherwise, I will be waiting for Tom's feedback and if it doesn't come in the next 1-2 weeks, I will send a friendly reminder. > Regards, > Simon Best regards, Eugeniu.
Hi, On 26 May 2018 at 05:04, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > Hi Simon, > > On Fri, May 25, 2018 at 08:07:37PM -0600, Simon Glass wrote: >> Hi, >> >> On 24 May 2018 at 16:04, Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: >> > Besides the above, it is not clear to me if your Reviewed-by applies to >> > to this patch partially (since you expressed some concerns) or applies >> > globally, in which case the concerns are not major? >> >> It means that I've reviewed the patch and I'd like some changes, but >> don't want to review it after you have made those changes, so you >> should add the Reviewed-by tag when doing the next version. >> >> But in this case your change is correct, so please don't worry. It's >> unfortunate that the uclass name needs a hypen, but I understand why. > > Thanks for shedding light both on technical aspects and the review > process itself, which is very helpful. > > Regarding the state of the whole patch-set, I think we reached a point > in which all the review comments have been handled. Please, let me know > if you still see any open topics. Otherwise, I will be waiting for > Tom's feedback and if it doesn't come in the next 1-2 weeks, I will send > a friendly reminder. Sounds good, I don't have anything else. Regards, Simon
On Sat, May 19, 2018 at 02:13:55PM +0200, Eugeniu Rosca wrote: > After importing v4.17-rc1 Linux commit 9130ba884640 ("scripts/dtc: > Update to upstream version v1.4.6-9-gaadd0b65c987"), sandbox build > reports below warnings: > > arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' > arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' > arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' > arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' > > Silent them by applying the 's/_/-/' substitution in the names of the > 'fdt_dummy0', 'fdt_dummy1', 'fdt_dummy2', 'fdt_dummy3' properties. > > Similar DTC warnings have been recently fixed in Linux kernel, e.g. via > v4.17-rc1 commit d366c30d19f4 ("ARM: dts: STi: Fix aliases property name > for STi boards"). > > If done alone, the DTS update generates a failure of the > `ut dm fdt_translation` unit test in sandbox environment as seen below: > > $ ./u-boot -d arch/sandbox/dts/test.dtb Applied to u-boot/master, thanks!
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 683b1970e0af..3e87c5c0f3fd 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -27,10 +27,10 @@ testfdt3 = "/b-test"; testfdt5 = "/some-bus/c-test@5"; testfdt8 = "/a-test"; - fdt_dummy0 = "/translation-test@8000/dev@0,0"; - fdt_dummy1 = "/translation-test@8000/dev@1,100"; - fdt_dummy2 = "/translation-test@8000/dev@2,200"; - fdt_dummy3 = "/translation-test@8000/noxlatebus@3,300/dev@42"; + fdt-dummy0 = "/translation-test@8000/dev@0,0"; + fdt-dummy1 = "/translation-test@8000/dev@1,100"; + fdt-dummy2 = "/translation-test@8000/dev@2,200"; + fdt-dummy3 = "/translation-test@8000/noxlatebus@3,300/dev@42"; usb0 = &usb_0; usb1 = &usb_1; usb2 = &usb_2; diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c index 8196844e89a7..66d0df5629a2 100644 --- a/test/dm/test-fdt.c +++ b/test/dm/test-fdt.c @@ -425,7 +425,7 @@ static const struct udevice_id fdt_dummy_ids[] = { }; UCLASS_DRIVER(fdt_dummy) = { - .name = "fdt_dummy", + .name = "fdt-dummy", .id = UCLASS_TEST_DUMMY, .flags = DM_UC_FLAG_SEQ_ALIAS, };
After importing v4.17-rc1 Linux commit 9130ba884640 ("scripts/dtc: Update to upstream version v1.4.6-9-gaadd0b65c987"), sandbox build reports below warnings: arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' arch/sandbox/dts/test.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-' Silent them by applying the 's/_/-/' substitution in the names of the 'fdt_dummy0', 'fdt_dummy1', 'fdt_dummy2', 'fdt_dummy3' properties. Similar DTC warnings have been recently fixed in Linux kernel, e.g. via v4.17-rc1 commit d366c30d19f4 ("ARM: dts: STi: Fix aliases property name for STi boards"). If done alone, the DTS update generates a failure of the `ut dm fdt_translation` unit test in sandbox environment as seen below: $ ./u-boot -d arch/sandbox/dts/test.dtb ---<-snip->--- => ut dm fdt_translation Test: dm_test_fdt_translation: test-fdt.c test/dm/test-fdt.c:444, dm_test_fdt_translation(): 0 == uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 0, 1, &dev): Expected 0, got -19 Test: dm_test_fdt_translation: test-fdt.c (flat tree) test/dm/test-fdt.c:444, dm_test_fdt_translation(): 0 == uclass_find_device_by_seq(UCLASS_TEST_DUMMY, 0, 1, &dev): Expected 0, got -19 Failures: 2 ---<-snip->--- Fix this issue in place, by updating the "name" string in the UCLASS_DRIVER(fdt_dummy) definition, so that it matches the newly updated aliases properties. After that, the test passes: $ ./u-boot -d arch/sandbox/dts/test.dtb ---<-snip->--- => ut dm fdt_translation Test: dm_test_fdt_translation: test-fdt.c Test: dm_test_fdt_translation: test-fdt.c (flat tree) Failures: 0 ---<-snip->--- Fixes: e8d5291824e2 ("core: ofnode: Fix translation for #size-cells == 0") Reported-by: Petr Vorel <pvorel@suse.cz> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com> --- v2->v3: * Fixed an issue in the test code (test/dm/test-fdt.c) generated by the DTS update (arch/sandbox/dts/test.dts) in [PATCH v2]. * Changed commit summary line, to cover test/dm/test-fdt.c. * Added: Reported-by: Petr Vorel <pvorel@suse.cz> * [Due to update] Dropped: Reviewed-by: Simon Glass <sjg@chromium.org> v1->v2: * Newly pushed arch/sandbox/dts/test.dts | 8 ++++---- test/dm/test-fdt.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)