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

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

[Michal Simek]

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@xilinx.com>
---

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.

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
  */
 int of_alias_get_alias_list(const struct of_device_id *matches,
 			     const char *stem, unsigned long *bitmap,
 			     unsigned int nbits)
 {
 	struct alias_prop *app;
+	int ret = 0;
 
 	/* Zero bitmap field to make sure that all the time it is clean */
 	bitmap_zero(bitmap, nbits);
@@ -1976,21 +1978,21 @@ int of_alias_get_alias_list(const struct of_device_id *matches,
 			continue;
 		}
 
-		if (app->id >= nbits) {
-			pr_debug("%s: ID %d greater then bitmap field %d\n",
-				__func__, app->id, nbits);
-			continue;
-		}
-
 		if (of_match_node(matches, app->np)) {
 			pr_debug("%s: Allocated ID %d\n", __func__, app->id);
-			set_bit(app->id, bitmap);
+
+			if (app->id >= nbits) {
+				pr_warn("%s: ID %d >= than bitmap field %d\n",
+					__func__, app->id, nbits);
+				ret = -EINVAL;
+			} else {
+				set_bit(app->id, bitmap);
+			}
 		}
-		/* Alias exists but is not compatible with matches */
 	}
 	mutex_unlock(&of_mutex);
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(of_alias_get_alias_list);
 
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index d452dceb0cb3..29a18103182d 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1393,7 +1393,7 @@ static int cdns_get_id(struct platform_device *pdev)
 	if (!alias_bitmap_initialized) {
 		ret = of_alias_get_alias_list(cdns_uart_of_match, "serial",
 					      alias_bitmap, MAX_UART_INSTANCES);
-		if (ret) {
+		if (ret && ret != -EINVAL) {
 			mutex_unlock(&bitmap_lock);
 			return ret;
 		}
-- 
1.9.1

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

[Rob Herring]

On Mon, Oct 8, 2018 at 7:19 AM Michal Simek <michal.simek@xilinx.com> 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@xilinx.com>
> ---
>
> 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.

>
> 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.

>   */
>  int of_alias_get_alias_list(const struct of_device_id *matches,
>                              const char *stem, unsigned long *bitmap,
>                              unsigned int nbits)
>  {
>         struct alias_prop *app;
> +       int ret = 0;
>
>         /* Zero bitmap field to make sure that all the time it is clean */
>         bitmap_zero(bitmap, nbits);
> @@ -1976,21 +1978,21 @@ int of_alias_get_alias_list(const struct of_device_id *matches,
>                         continue;
>                 }
>
> -               if (app->id >= nbits) {
> -                       pr_debug("%s: ID %d greater then bitmap field %d\n",
> -                               __func__, app->id, nbits);
> -                       continue;
> -               }
> -
>                 if (of_match_node(matches, app->np)) {
>                         pr_debug("%s: Allocated ID %d\n", __func__, app->id);
> -                       set_bit(app->id, bitmap);
> +
> +                       if (app->id >= nbits) {
> +                               pr_warn("%s: ID %d >= than bitmap field %d\n",
> +                                       __func__, app->id, nbits);
> +                               ret = -EINVAL;
> +                       } else {
> +                               set_bit(app->id, bitmap);
> +                       }
>                 }
> -               /* Alias exists but is not compatible with matches */
>         }
>         mutex_unlock(&of_mutex);
>
> -       return 0;
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_alias_get_alias_list);
>
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index d452dceb0cb3..29a18103182d 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -1393,7 +1393,7 @@ static int cdns_get_id(struct platform_device *pdev)
>         if (!alias_bitmap_initialized) {
>                 ret = of_alias_get_alias_list(cdns_uart_of_match, "serial",
>                                               alias_bitmap, MAX_UART_INSTANCES);
> -               if (ret) {
> +               if (ret && ret != -EINVAL) {
>                         mutex_unlock(&bitmap_lock);
>                         return ret;
>                 }
> --
> 1.9.1
>

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@xilinx.com> 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@xilinx.com>
>> ---
>>
>> 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