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

[Alexandru Marginean]

On 12/18/2019 10:45 PM, Michael Walle wrote:
> 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

It's certainly not something that should be part of this patch and since 
I think it should be changed I can send a patch for it.

Thanks!
Alex