Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias

[Ulf Hansson]

On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
<matthias.schiffer@xxxxxxxxxxxxxxx> wrote:
>
> As with GPIO, UART and others, allow specifying the device index via the
> aliases node in the device tree.
>
> On embedded devices, there is often a combination of removable (e.g.
> SD card) and non-removable MMC devices (e.g. eMMC).
> Therefore the index might change depending on
>
> * host of removable device
> * removable card present or not
>
> This makes it difficult to hardcode the root device, if it is on the
> non-removable device. E.g. if SD card is present eMMC will be mmcblk1,
> if SD card is not present at boot, eMMC will be mmcblk0.
>
> All indices defined in the aliases node will be reserved for use by the
> respective MMC device, moving the indices of devices that don't have an
> alias up into the non-reserved range. If the aliases node is not found,
> the driver will act as before.
>
> This is a rebased and slightly cleaned up version of
> https://www.spinics.net/lists/linux-mmc/msg26588.html .
>
> Based-on-patch-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Link: https://lkml.org/lkml/2020/8/5/194
> Signed-off-by: Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>
> ---
>
> v2: fix missing symbols for modular mmcblock
>
>  drivers/mmc/core/block.c | 13 +++++++++++--
>  drivers/mmc/core/core.c  | 40 ++++++++++++++++++++++++++++++++++++++++
>  drivers/mmc/core/core.h  |  3 +++
>  drivers/mmc/core/host.c  | 15 +++++++++++++--
>  4 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index 7896952de1ac..4620afaf0e50 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -38,6 +38,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/idr.h>
>  #include <linux/debugfs.h>
> +#include <linux/of.h>
>
>  #include <linux/mmc/ioctl.h>
>  #include <linux/mmc/card.h>
> @@ -2260,9 +2261,17 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
>                                               int area_type)
>  {
>         struct mmc_blk_data *md;
> -       int devidx, ret;
> +       int rsvidx, devidx = -1, ret;
> +
> +       rsvidx = mmc_get_reserved_index(card->host);
> +       if (rsvidx >= 0)
> +               devidx = ida_simple_get(&mmc_blk_ida, rsvidx, rsvidx + 1,
> +                                       GFP_KERNEL);
> +       if (devidx < 0)
> +               devidx = ida_simple_get(&mmc_blk_ida,
> +                                       mmc_first_nonreserved_index(),
> +                                       max_devices, GFP_KERNEL);
>
> -       devidx = ida_simple_get(&mmc_blk_ida, 0, max_devices, GFP_KERNEL);

I am not sure why you need to change any of this. Currently we use the
host->index, when creating the blockpartion name (dev.init_name).

This is done for both rpmb and regular partitions. Isn't that sufficient?

>         if (devidx < 0) {
>                 /*
>                  * We get -ENOSPC because there are no more any available
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 8ccae6452b9c..5bce281a5faa 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2419,10 +2419,50 @@ void mmc_unregister_pm_notifier(struct mmc_host *host)
>  }
>  #endif
>
> +static int mmc_max_reserved_idx = -1;
> +
> +/**
> + * mmc_first_nonreserved_index() - get the first index that is not reserved
> + */
> +int mmc_first_nonreserved_index(void)
> +{
> +       return mmc_max_reserved_idx + 1;
> +}
> +EXPORT_SYMBOL(mmc_first_nonreserved_index);
> +
> +/**
> + * mmc_get_reserved_index() - get the index reserved for this MMC host
> + *
> + * Returns:
> + *   The index reserved for this host on success,
> + *   negative error if no index is reserved for this host
> + */
> +int mmc_get_reserved_index(struct mmc_host *host)
> +{
> +       return of_alias_get_id(host->parent->of_node, "mmc");
> +}
> +EXPORT_SYMBOL(mmc_get_reserved_index);

I think you can drop this function, just call of_alias_get_id() from
where it's needed.

> +
> +static void __init mmc_of_reserve_idx(void)
> +{
> +       int max;
> +
> +       max = of_alias_get_highest_id("mmc");

Is calling of_alias_get_highest_id("mmc") costly from an execution
point of view?

If not, we might as well call it directly from mmc_alloc_host() each
time a host is allocated instead, to simplify the code a bit.

> +       if (max < 0)
> +               return;
> +
> +       mmc_max_reserved_idx = max;
> +
> +       pr_debug("MMC: reserving %d slots for OF aliases\n",
> +                mmc_max_reserved_idx + 1);

If this function is needed at all (see comments above), then please
drop this debug print.

> +}
> +
>  static int __init mmc_init(void)
>  {
>         int ret;
>
> +       mmc_of_reserve_idx();
> +
>         ret = mmc_register_bus();
>         if (ret)
>                 return ret;
> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
> index 575ac0257af2..6aef6cf4e90f 100644
> --- a/drivers/mmc/core/core.h
> +++ b/drivers/mmc/core/core.h
> @@ -79,6 +79,9 @@ int mmc_attach_mmc(struct mmc_host *host);
>  int mmc_attach_sd(struct mmc_host *host);
>  int mmc_attach_sdio(struct mmc_host *host);
>
> +int mmc_first_nonreserved_index(void);
> +int mmc_get_reserved_index(struct mmc_host *host);
> +
>  /* Module parameters */
>  extern bool use_spi_crc;
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index ce43f7573d80..386e15afde83 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -387,6 +387,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
>  {
>         int err;
>         struct mmc_host *host;
> +       int alias_id, min_idx, max_idx;
>
>         host = kzalloc(sizeof(struct mmc_host) + extra, GFP_KERNEL);
>         if (!host)
> @@ -395,7 +396,18 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
>         /* scanning will be enabled when we're ready */
>         host->rescan_disable = 1;
>
> -       err = ida_simple_get(&mmc_host_ida, 0, 0, GFP_KERNEL);
> +       host->parent = dev;
> +
> +       alias_id = mmc_get_reserved_index(host);
> +       if (alias_id >= 0) {
> +               min_idx = alias_id;
> +               max_idx = alias_id + 1;
> +       } 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;
> @@ -406,7 +418,6 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
>         dev_set_name(&host->class_dev, "mmc%d", host->index);
>         host->ws = wakeup_source_register(NULL, dev_name(&host->class_dev));
>
> -       host->parent = dev;
>         host->class_dev.parent = dev;
>         host->class_dev.class = &mmc_host_class;
>         device_initialize(&host->class_dev);
> --
> 2.17.1
>

Kind regards
Uffe