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
