[PATCH v3] dm: uclass: don't assign aliased seq numbers
Michael Walle
michael at walle.cc
Thu Feb 20 10:52:32 CET 2020
Hi Michal,
Am 2020-02-20 09:30, schrieb Michal Simek:
> On 03. 02. 20 18:11, Michael Walle wrote:
>> If there are aliases for an uclass, set the base for the "dynamically"
>> allocated numbers next to the highest alias.
>>
>> Please note, that this might lead to holes in the sequences, depending
>> on the device tree. For example if there is only an alias "ethernet1",
>> the next device seq number would be 2.
>>
>> In particular this fixes a problem with boards which are using
>> ethernet
>> aliases but also might have network add-in cards like the E1000. If
>> the
>> board is started with the add-in card and depending on the order of
>> the
>> drivers, the E1000 might occupy the first ethernet device and mess up
>> all the hardware addresses, because the devices are now shifted by
>> one.
>>
>> Also adapt the test cases to the new handling and add test cases
>> checking the holes in the seq numbers.
>>
>> Signed-off-by: Michael Walle <michael at walle.cc>
>> Reviewed-by: Alex Marginean <alexandru.marginean at nxp.com>
>> Tested-by: Alex Marginean <alexandru.marginean at nxp.com>
>> Acked-by: Vladimir Oltean <olteanv at gmail.com>
>> ---
>>
>> Please note that I've kept the R-b, T-b, and A-b tags although they
>> were
>> for an older version. They only affects the drivers/core/uclass.c not
>> the
>> test/dm/ part. OTOH none of the actual implementation has changed.
>>
>> I couldn't split the patch, otherwise the tests would fail.
>>
>> As a side effect, this should also make the following commits
>> superfluous:
>> - 7f3289bf6d ("dm: device: Request next sequence number")
>> - 61607225d1 ("i2c: Fill req_seq in i2c_post_bind()")
>> Although I don't understand the root cause of the said problem.
>>
>> Thomas, Michal, could you please test this and then I'd add a second
>> patch removing the old code.
>>
>> changes since v2:
>> - adapt/new test cases, thanks Simon
>>
>> changes since v1:
>> - move notice about superfluous commits from commit message to this
>> section.
>> - fix the comment style
>>
>> arch/sandbox/dts/test.dts | 4 ++--
>> drivers/core/uclass.c | 21 +++++++++++++++------
>> include/configs/sandbox.h | 6 +++---
>> test/dm/eth.c | 14 +++++++-------
>> test/dm/test-fdt.c | 22 +++++++++++++++++-----
>> 5 files changed, 44 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
>> index e529c54d8d..d448892a65 100644
>> --- a/arch/sandbox/dts/test.dts
>> +++ b/arch/sandbox/dts/test.dts
>> @@ -19,8 +19,8 @@
>> pci0 = &pci0;
>> pci1 = &pci1;
>> pci2 = &pci2;
>> - remoteproc1 = &rproc_1;
>> - remoteproc2 = &rproc_2;
>> + remoteproc0 = &rproc_1;
>> + remoteproc1 = &rproc_2;
>> rtc0 = &rtc_0;
>> rtc1 = &rtc_1;
>> spi0 = "/spi at 0";
>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
>> index c520ef113a..3c216221e0 100644
>> --- a/drivers/core/uclass.c
>> +++ b/drivers/core/uclass.c
>> @@ -675,13 +675,14 @@ int uclass_unbind_device(struct udevice *dev)
>>
>> int uclass_resolve_seq(struct udevice *dev)
>> {
>> + struct uclass *uc = dev->uclass;
>> + struct uclass_driver *uc_drv = uc->uc_drv;
>> struct udevice *dup;
>> - int seq;
>> + int seq = 0;
>> int ret;
>>
>> assert(dev->seq == -1);
>> - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id,
>> dev->req_seq,
>> - false, &dup);
>> + ret = uclass_find_device_by_seq(uc_drv->id, dev->req_seq, false,
>> &dup);
>> if (!ret) {
>> dm_warn("Device '%s': seq %d is in use by '%s'\n",
>> dev->name, dev->req_seq, dup->name);
>> @@ -693,9 +694,17 @@ int uclass_resolve_seq(struct udevice *dev)
>> return ret;
>> }
>>
>> - for (seq = 0; seq < DM_MAX_SEQ; seq++) {
>> - ret = uclass_find_device_by_seq(dev->uclass->uc_drv->id, seq,
>> - false, &dup);
>> + if (CONFIG_IS_ENABLED(DM_SEQ_ALIAS) &&
>> + (uc_drv->flags & DM_UC_FLAG_SEQ_ALIAS)) {
>> + /*
>> + * dev_read_alias_highest_id() will return -1 if there no
>> + * alias. Thus we can always add one.
>> + */
>> + seq = dev_read_alias_highest_id(uc_drv->name) + 1;
>> + }
>> +
>> + for (; seq < DM_MAX_SEQ; seq++) {
>> + ret = uclass_find_device_by_seq(uc_drv->id, seq, false, &dup);
>> if (ret == -ENODEV)
>> break;
>> if (ret)
>> diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
>> index 1c13055cdc..b02c362fed 100644
>> --- a/include/configs/sandbox.h
>> +++ b/include/configs/sandbox.h
>> @@ -97,9 +97,9 @@
>> #endif
>>
>> #define SANDBOX_ETH_SETTINGS "ethaddr=00:00:11:22:33:44\0" \
>> - "eth1addr=00:00:11:22:33:45\0" \
>> - "eth3addr=00:00:11:22:33:46\0" \
>> - "eth5addr=00:00:11:22:33:47\0" \
>> + "eth3addr=00:00:11:22:33:45\0" \
>> + "eth5addr=00:00:11:22:33:46\0" \
>> + "eth6addr=00:00:11:22:33:47\0" \
>> "ipaddr=1.2.3.4\0"
>>
>> #define MEM_LAYOUT_ENV_SETTINGS \
>> diff --git a/test/dm/eth.c b/test/dm/eth.c
>> index ad5354b4bf..75315a0c6d 100644
>> --- a/test/dm/eth.c
>> +++ b/test/dm/eth.c
>> @@ -47,7 +47,7 @@ static int dm_test_eth_alias(struct unit_test_state
>> *uts)
>> ut_assertok(net_loop(PING));
>> ut_asserteq_str("eth at 10002000", env_get("ethact"));
>>
>> - env_set("ethact", "eth1");
>> + env_set("ethact", "eth6");
>> ut_assertok(net_loop(PING));
>> ut_asserteq_str("eth at 10004000", env_get("ethact"));
>>
>> @@ -104,7 +104,7 @@ static int dm_test_eth_act(struct unit_test_state
>> *uts)
>> const char *ethname[DM_TEST_ETH_NUM] = {"eth at 10002000",
>> "eth at 10003000",
>> "sbe5", "eth at 10004000"};
>> const char *addrname[DM_TEST_ETH_NUM] = {"ethaddr", "eth5addr",
>> - "eth3addr", "eth1addr"};
>> + "eth3addr", "eth6addr"};
>> char ethaddr[DM_TEST_ETH_NUM][18];
>> int i;
>>
>> @@ -187,15 +187,15 @@ static int dm_test_eth_rotate(struct
>> unit_test_state *uts)
>>
>> /* Invalidate eth1's MAC address */
>> memset(ethaddr, '\0', sizeof(ethaddr));
>> - strncpy(ethaddr, env_get("eth1addr"), 17);
>> - /* Must disable access protection for eth1addr before clearing */
>> - env_set(".flags", "eth1addr");
>> - env_set("eth1addr", NULL);
>> + strncpy(ethaddr, env_get("eth6addr"), 17);
>> + /* Must disable access protection for eth6addr before clearing */
>> + env_set(".flags", "eth6addr");
>> + env_set("eth6addr", NULL);
>>
>> retval = _dm_test_eth_rotate1(uts);
>>
>> /* Restore the env */
>> - env_set("eth1addr", ethaddr);
>> + env_set("eth6addr", ethaddr);
>> env_set("ethrotate", NULL);
>>
>> if (!retval) {
>> diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
>> index d59c449ce0..a4e5c64a1f 100644
>> --- a/test/dm/test-fdt.c
>> +++ b/test/dm/test-fdt.c
>> @@ -359,20 +359,32 @@ static int dm_test_fdt_uclass_seq(struct
>> unit_test_state *uts)
>> ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 2, &dev));
>> ut_asserteq_str("d-test", dev->name);
>>
>> - /* d-test actually gets 0 */
>> - ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 0, &dev));
>> + /*
>> + * d-test actually gets 9, because thats the next free one after the
>> + * aliases.
>> + */
>> + ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 9, &dev));
>> ut_asserteq_str("d-test", dev->name);
>>
>> - /* initially no one wants seq 1 */
>> - ut_asserteq(-ENODEV, uclass_get_device_by_seq(UCLASS_TEST_FDT, 1,
>> + /* initially no one wants seq 10 */
>> + ut_asserteq(-ENODEV, uclass_get_device_by_seq(UCLASS_TEST_FDT, 10,
>> &dev));
>> ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 0, &dev));
>> ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 4, &dev));
>>
>> /* But now that it is probed, we can find it */
>> - ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 1, &dev));
>> + ut_assertok(uclass_get_device_by_seq(UCLASS_TEST_FDT, 10, &dev));
>> ut_asserteq_str("f-test", dev->name);
>>
>> + /*
>> + * And we should still have holes in our sequence numbers, that is 2
>> + * and 4 should not be used.
>> + */
>> + ut_asserteq(-ENODEV, uclass_find_device_by_seq(UCLASS_TEST_FDT, 2,
>> + true, &dev));
>> + ut_asserteq(-ENODEV, uclass_find_device_by_seq(UCLASS_TEST_FDT, 4,
>> + true, &dev));
>> +
>> return 0;
>> }
>> DM_TEST(dm_test_fdt_uclass_seq, DM_TESTF_SCAN_PDATA |
>> DM_TESTF_SCAN_FDT);
>>
>
> Tested-by: Michal Simek <michal.simek at xilinx.com>
Thanks!
Just to be sure, did you test this patch or did you also revert
61607225d1 ("i2c: Fill req_seq in i2c_post_bind()")
before testing?
-michael
More information about the U-Boot
mailing list