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

[Michael Walle]

Am 2019-12-18 21:00, schrieb Alexandru Marginean:
> Hi Michael,
> 
> On 12/18/2019 5:42 PM, 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.
>> 
>> 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.
>> 
>> Cc: Thomas Fitzsimmons <fitzsim at fitzsim.org>
>> Cc: Michal Simek <michal.simek at xilinx.com>
>> 
>> Signed-off-by: Michael Walle <michael at walle.cc>
>> ---
>>   drivers/core/uclass.c | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
>> index c520ef113a..c3b325141a 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,16 @@ 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
> 
> nits: there /is/ no and multi-line comment starts with just /* on the 
> 1st line

yeah, I'd prefer that style too. But checkpatch.pl would complain
about it.. or so I thought.. well seems only to be the case in linux and
only in "NETWORKING_BLOCK_COMMENT_STYLE" (only for net/ and 
drivers/net/).

I'll definitely fix that ;)

> 
>> +		 * 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)
>> 
> 
> 
> Reviewed-by: Alex Marginean <alexandru.marginean at nxp.com>
> Tested-by: Alex Marginean <alexandru.marginean at nxp.com>
> 
> This looks nice.
> One thing I noticed is that 'dm tree' indexes now don't match dev->seq
> which can be a bit confusing.  The index is produced by
> dev_get_uclass_index, which in effect counts devices in the uclass.
> Any issue if 'dm tree' prints dev->seq instead and maybe call the
> column Seq rather than Index?

Mhh, did this ever match if req_seq/aliases was used? "dm uclass" dumps
the seq and req_seq. So I don't know if that was ever intended to match.
But I'm open to suggestions.

-michael