[PATCH v3] dm: uclass: don't assign aliased seq numbers

[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,
Michal