diff mbox series

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

Message ID 20200820075949.19133-1-matthias.schiffer@ew.tq-group.com
State New
Headers show
Series [mmc-next,v2] mmc: allow setting slot index via device tree alias | expand

Commit Message

Matthias Schiffer Aug. 20, 2020, 7:59 a.m. UTC
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@pengutronix.de>
Link: https://lkml.org/lkml/2020/8/5/194
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

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

Comments

Ulf Hansson Aug. 21, 2020, 11:39 a.m. UTC | #1
On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> 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.

Can you please add some information why Part-UUID/labels don't work
well to solve this problem on some embedded systems?

I think that deserves to be in the changelog, after all the long
discussions we had in the history around this.

I will continue to review the code in a separate email, in a while.

Kind regards
Uffe

>
> 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@pengutronix.de>
> Link: https://lkml.org/lkml/2020/8/5/194
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>
> 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);
>         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);
> +
> +static void __init mmc_of_reserve_idx(void)
> +{
> +       int max;
> +
> +       max = of_alias_get_highest_id("mmc");
> +       if (max < 0)
> +               return;
> +
> +       mmc_max_reserved_idx = max;
> +
> +       pr_debug("MMC: reserving %d slots for OF aliases\n",
> +                mmc_max_reserved_idx + 1);
> +}
> +
>  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
>
Matthias Schiffer Aug. 24, 2020, 9:41 a.m. UTC | #2
On Fri, 2020-08-21 at 13:39 +0200, Ulf Hansson wrote:
> On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> <matthias.schiffer@ew.tq-group.com> 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.
> 
> Can you please add some information why Part-UUID/labels don't work
> well to solve this problem on some embedded systems?
> 
> I think that deserves to be in the changelog, after all the long
> discussions we had in the history around this.

Makes sense. I'll wait for your review before I send a v3 with an
updated description.

Matthias


> 
> I will continue to review the code in a separate email, in a while.
> 
> Kind regards
> Uffe
> 
> > 
> > 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@pengutronix.de>
> > Link: https://lkml.org/lkml/2020/8/5/194
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com
> > >
> > ---
> > 
> > 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);
> >         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);
> > +
> > +static void __init mmc_of_reserve_idx(void)
> > +{
> > +       int max;
> > +
> > +       max = of_alias_get_highest_id("mmc");
> > +       if (max < 0)
> > +               return;
> > +
> > +       mmc_max_reserved_idx = max;
> > +
> > +       pr_debug("MMC: reserving %d slots for OF aliases\n",
> > +                mmc_max_reserved_idx + 1);
> > +}
> > +
> >  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
> >
Ulf Hansson Aug. 25, 2020, 9:14 a.m. UTC | #3
On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> 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@pengutronix.de>
> Link: https://lkml.org/lkml/2020/8/5/194
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>
> 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
Ulf Hansson Aug. 25, 2020, 9:19 a.m. UTC | #4
On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> 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@pengutronix.de>
> Link: https://lkml.org/lkml/2020/8/5/194
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

One more thing, can you please also update the DT doc example in
mmc-controller.yaml, to show that mmc supports aliases. In a separate
patch, of course.

Kind regards
Uffe

> ---
>
> 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);
>         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);
> +
> +static void __init mmc_of_reserve_idx(void)
> +{
> +       int max;
> +
> +       max = of_alias_get_highest_id("mmc");
> +       if (max < 0)
> +               return;
> +
> +       mmc_max_reserved_idx = max;
> +
> +       pr_debug("MMC: reserving %d slots for OF aliases\n",
> +                mmc_max_reserved_idx + 1);
> +}
> +
>  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
>
Matthias Schiffer Aug. 25, 2020, 9:39 a.m. UTC | #5
On Tue, 2020-08-25 at 11:14 +0200, Ulf Hansson wrote:
> On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> <matthias.schiffer@ew.tq-group.com> 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@pengutronix.de>
> > Link: https://lkml.org/lkml/2020/8/5/194
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com
> > >
> > ---
> > 
> > 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?

You are right, that should be 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.

Ok.

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

It's not particularly costly (it just walks the list of aliases once
and does a string comparison for each entry), but it does so while
holding the of_mutex.

Both variants exist in the current kernel: The I2C core stores the
hightest index in a global variable, while of_alias_get_highest_id() is
called once for each registered SPI controller. I have a slight
preference for the global variable solution.


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

Ok.

> 
> > +}
> > +
> >  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
Matthias Schiffer Aug. 25, 2020, 11:24 a.m. UTC | #6
On Tue, 2020-08-25 at 11:39 +0200, Matthias Schiffer wrote:
> On Tue, 2020-08-25 at 11:14 +0200, Ulf Hansson wrote:
> > On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> > <matthias.schiffer@ew.tq-group.com> wrote:
> > > +
> > > +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.
> 
> It's not particularly costly (it just walks the list of aliases once
> and does a string comparison for each entry), but it does so while
> holding the of_mutex.
> 
> Both variants exist in the current kernel: The I2C core stores the
> hightest index in a global variable, while of_alias_get_highest_id()
> is
> called once for each registered SPI controller. I have a slight
> preference for the global variable solution.

Looking at this again, it's pretty much the same as of_alias_get_id().
I'll remove the extra functions and global variable.

Kind regards,
Matthias
Matthias Schiffer Aug. 25, 2020, noon UTC | #7
On Tue, 2020-08-25 at 11:39 +0200, Matthias Schiffer wrote:
> On Tue, 2020-08-25 at 11:14 +0200, Ulf Hansson wrote:
> > On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> > <matthias.schiffer@ew.tq-group.com> wrote:
> > > --- 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);


One more question I came across when reworking my patch: Do we need a
fallback here for the case where the reserved index is already taken?
To handle an SD card being replaced while still mounted?


> > >         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
Ulf Hansson Aug. 25, 2020, 12:27 p.m. UTC | #8
On Tue, 25 Aug 2020 at 14:00, Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:
>
> On Tue, 2020-08-25 at 11:39 +0200, Matthias Schiffer wrote:
> > On Tue, 2020-08-25 at 11:14 +0200, Ulf Hansson wrote:
> > > On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> > > <matthias.schiffer@ew.tq-group.com> wrote:
> > > > --- 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);
>
>
> One more question I came across when reworking my patch: Do we need a
> fallback here for the case where the reserved index is already taken?
> To handle an SD card being replaced while still mounted?

Removal/insertion of an SD card should be fine, as that doesn't mean
that the host is removed. In other words, host->index remains the
same.

Although, for a bad DT configuration, where for example the same
aliases id is used twice, a fallback could make sense. On the other
hand, as such configuration would be wrong, we might as well just
print a message and return an error.

[...]

Kind regards
Uffe
Matthias Schiffer Aug. 25, 2020, 12:32 p.m. UTC | #9
On Tue, 2020-08-25 at 14:27 +0200, Ulf Hansson wrote:
> On Tue, 25 Aug 2020 at 14:00, Matthias Schiffer
> <matthias.schiffer@ew.tq-group.com> wrote:
> > 
> > On Tue, 2020-08-25 at 11:39 +0200, Matthias Schiffer wrote:
> > > On Tue, 2020-08-25 at 11:14 +0200, Ulf Hansson wrote:
> > > > On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> > > > <matthias.schiffer@ew.tq-group.com> wrote:
> > > > > --- 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);
> > 
> > 
> > One more question I came across when reworking my patch: Do we need
> > a
> > fallback here for the case where the reserved index is already
> > taken?
> > To handle an SD card being replaced while still mounted?
> 
> Removal/insertion of an SD card should be fine, as that doesn't mean
> that the host is removed. In other words, host->index remains the
> same.
> 
> Although, for a bad DT configuration, where for example the same
> aliases id is used twice, a fallback could make sense. On the other
> hand, as such configuration would be wrong, we might as well just
> print a message and return an error.

I don't think this can happen as long as we don't have DTs changing at
runtime: Each alias is a DT property name in /aliases, which can only exist once.


> 
> [...]
> 
> Kind regards
> Uffe
diff mbox series

Patch

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);
 	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);
+
+static void __init mmc_of_reserve_idx(void)
+{
+	int max;
+
+	max = of_alias_get_highest_id("mmc");
+	if (max < 0)
+		return;
+
+	mmc_max_reserved_idx = max;
+
+	pr_debug("MMC: reserving %d slots for OF aliases\n",
+		 mmc_max_reserved_idx + 1);
+}
+
 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);