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

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