Re: [PATCH] of: base: Change logic in of_alias_get_alias_list()

[Michal Simek]

On 8.10.2018 17:07, Rob Herring wrote:
> On Mon, Oct 8, 2018 at 7:19 AM Michal Simek <michal.simek@xxxxxxxxxx> wrote:
>>
>> Check compatible string first before setting up bit in bitmap to also
>> cover cases that allocated bitfield is not big enough.
>> Show warning about it but let driver to continue to work with allocated
>> bitfield to keep at least some devices (included console which
>> is commonly close to serial0) to work.
>>
>> Fixes: b1078c355d76 ("of: base: Introduce of_alias_get_alias_list() to check alias IDs")
>> Fixes: ae1cca3fa347 ("serial: uartps: Change uart ID port allocation")
>> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
>> ---
>>
>> I have looked at it and I don't think there should be necessary to
>> report error immediately back with partially initialized bitfield.
>> The reason is that still there could be a console device which is most
>> likely below that max limit and it is worth to return at least that nbits
>> properly filled.
>> It will also enable cases that you can still continue to use aliases
>> higher then fields prepared for devices without alias.
> 
> Seems reasonable. Plus if you had a new dtb which added an alias
> greater than what the OS version supports, you would break that
> system.

I was checking that with our uart ps driver before I send this patch and
I found out that there is no reason not to support these cases.
Driver will simply find out ids which are free and ready for devices
which don't have alias.

> 
>>
>> To be fixed patches are present in tty-next branch.
>>
>> ---
>>  drivers/of/base.c                  | 22 ++++++++++++----------
>>  drivers/tty/serial/xilinx_uartps.c |  2 +-
>>  2 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 908de45f966b..0b9611e196d1 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -1953,13 +1953,15 @@ int of_alias_get_id(struct device_node *np, const char *stem)
>>   * The function travels the lookup table to record alias ids for the given
>>   * device match structures and alias stem.
>>   *
>> - * Return:     0 or -ENOSYS when !CONFIG_OF
>> + * Return:     0 or -ENOSYS when !CONFIG_OF or
>> + *             -EINVAL if alias ID is greater then allocated nbits
> 
> I think EOVERFLOW or ERANGE would be better as those are less common
> and I take EINVAL as the caller made an error.

That's a good point I was also thinking if EINVAL is good reaction on
this case. I will use EOVERFLOW if there is no issue with it.

Thanks,
Michal