Re: [PATCH] mmc: core: Don't allocate IDA for OF aliases

[Ulf Hansson]

On Tue, 13 Apr 2021 at 02:36, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> There's a chance that the IDA allocated in mmc_alloc_host() is not freed
> for some time because it's freed as part of a class' release function
> (see mmc_host_classdev_release() where the IDA is freed). If another
> thread is holding a reference to the class, then only once all balancing
> device_put() calls (in turn calling kobject_put()) have been made will
> the IDA be released and usable again.
>
> Normally this isn't a problem because the kobject is released before
> anything else that may want to use the same number tries to again, but
> with CONFIG_DEBUG_KOBJECT_RELEASE=y and OF aliases it becomes pretty
> easy to try to allocate an alias from the IDA twice while the first time
> it was allocated is still pending a call to ida_simple_remove(). It's
> also possible to trigger it by using CONFIG_DEBUG_KOBJECT_RELEASE and
> probe defering a driver at boot that calls mmc_alloc_host() before
> trying to get resources that may defer likes clks or regulators.

Thanks for a very nice description of the problem.

>
> Instead of allocating from the IDA in this scenario, let's just skip it
> if we know this is an OF alias. The number is already "claimed" and
> devices that aren't using OF aliases won't try to use the claimed
> numbers anyway (see mmc_first_nonreserved_index()). This should avoid
> any issues with mmc_alloc_host() returning failures from the
> ida_simple_get() in the case that we're using an OF alias.

At first glance, this seems like a good idea, but I am not completely
sure, yet. See more below.

>
> Cc: Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>
> Cc: Sujit Kautkar <sujitka@xxxxxxxxxxxx>
> Reported-by: Zubin Mithra <zsm@xxxxxxxxxxxx>
> Fixes: fa2d0aa96941 ("mmc: core: Allow setting slot index via device tree alias")
> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> ---
>  drivers/mmc/core/host.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 9b89a91b6b47..137b4a769f62 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -39,7 +39,8 @@ static void mmc_host_classdev_release(struct device *dev)
>  {
>         struct mmc_host *host = cls_dev_to_mmc_host(dev);
>         wakeup_source_unregister(host->ws);
> -       ida_simple_remove(&mmc_host_ida, host->index);
> +       if (of_alias_get_id(host->parent->of_node, "mmc") < 0)
> +               ida_simple_remove(&mmc_host_ida, host->index);
>         kfree(host);
>  }
>
> @@ -444,7 +445,7 @@ static int mmc_first_nonreserved_index(void)
>   */
>  struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
>  {
> -       int err;
> +       int index;
>         struct mmc_host *host;
>         int alias_id, min_idx, max_idx;
>
> @@ -457,20 +458,19 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
>
>         alias_id = of_alias_get_id(dev->of_node, "mmc");
>         if (alias_id >= 0) {
> -               min_idx = alias_id;
> -               max_idx = alias_id + 1;
> +               index = alias_id;
>         } else {
>                 min_idx = mmc_first_nonreserved_index();
>                 max_idx = 0;
> -       }
>
> -       err = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
> -       if (err < 0) {
> -               kfree(host);
> -               return NULL;
> +               index = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
> +               if (index < 0) {
> +                       kfree(host);
> +                       return NULL;
> +               }

This means that a DTB that is screwed up in a way that it has two mmc
aliases with the same index, would be allowed to use the same index.

What will happen when we continue the probe sequence in such a case?

>         }
>
> -       host->index = err;
> +       host->index = index;
>
>         dev_set_name(&host->class_dev, "mmc%d", host->index);
>         host->ws = wakeup_source_register(NULL, dev_name(&host->class_dev));

Another concern that could potentially be a problem, is that the
"thread" that holds the reference that prevents ida from being
removed, how would that react to a new mmc device to become
re-registered with the same index?

I wonder if we perhaps should return -EPROBE_DEFER instead, when
ida_simple_get() fails?

Kind regards
Uffe