Re: (EXT) Re: (EXT) Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
On Tue, 2020-08-25 at 14:27 +0200, Ulf Hansson wrote:
> On Tue, 25 Aug 2020 at 14:00, Matthias Schiffer
> 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
> > > > 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(_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(_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
Re: (EXT) Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
On Tue, 25 Aug 2020 at 14:00, Matthias Schiffer
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
> > > 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(_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(_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
Re: (EXT) Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
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
> > 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(_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(_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(>class_dev, "mmc%d", host->index);
> > > host->ws = wakeup_source_register(NULL, dev_name(
> > > > class_dev));
> > >
> > > - host->parent = dev;
> > > host->class_dev.parent = dev;
> > > host->class_dev.class = _host_class;
> > > device_initialize(>class_dev);
> > > --
> > > 2.17.1
> > >
> >
> > Kind regards
> > Uffe
Re: (EXT) Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
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
> > 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
Re: (EXT) Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
On Tue, 2020-08-25 at 11:14 +0200, Ulf Hansson wrote:
> On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> 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
> > Link: https://lkml.org/lkml/2020/8/5/194
> > Signed-off-by: Matthias Schiffer > >
> > ---
> >
> > 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
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -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(_blk_ida, rsvidx,
> > rsvidx + 1,
> > + GFP_KERNEL);
> > + if (devidx < 0)
> > + devidx = ida_simple_get(_blk_ida,
> > + mmc_first_nonreserved_index
> > (),
> > + max_devices, GFP_KERNEL);
> >
> > - devidx = ida_simple_get(_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
Re: (EXT) Re: [PATCH mmc-next v2] mmc: allow setting slot index via device tree alias
On Fri, 2020-08-21 at 13:39 +0200, Ulf Hansson wrote:
> On Thu, 20 Aug 2020 at 09:59, Matthias Schiffer
> 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
> > Link: https://lkml.org/lkml/2020/8/5/194
> > Signed-off-by: Matthias Schiffer > >
> > ---
> >
> > 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
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -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(_blk_ida, rsvidx,
> > rsvidx + 1,
> > + GFP_KERNEL);
> > + if (devidx < 0)
> > + devidx = ida_simple_get(_blk_ida,
> > + mmc_first_nonreserved_index
> > (),
> > + max_devices, GFP_KERNEL);
>
>
>
> >
> > - devidx = ida_simple_get(_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
