[PATCH 1/2] mmc: core: Do not export MMC_NAME= and MODALIAS=mmc:block for SDIO cards

[PATCH 1/2] mmc: core: Do not export MMC_NAME= and MODALIAS=mmc:block for SDIO cards

[Pali Rohár]

SDIO non-combo cards are not handled by mmc_block driver and do not have
accessible CID register which is used for MMC_NAME= construction.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/mmc/core/bus.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 74de3f2dd..103eea7cd 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -93,15 +93,20 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 			return retval;
 	}
 
-	retval = add_uevent_var(env, "MMC_NAME=%s", mmc_card_name(card));
-	if (retval)
-		return retval;
-
-	/*
-	 * Request the mmc_block device.  Note: that this is a direct request
-	 * for the module it carries no information as to what is inserted.
-	 */
-	retval = add_uevent_var(env, "MODALIAS=mmc:block");
+	if (card->type != MMC_TYPE_SDIO) {
+		retval = add_uevent_var(env, "MMC_NAME=%s", mmc_card_name(card));
+		if (retval)
+			return retval;
+
+		/*
+		 * Request the mmc_block device.
+		 * Note: that this is a direct request for the module it carries
+		 * no information as to what is inserted.
+		 */
+		retval = add_uevent_var(env, "MODALIAS=mmc:block");
+		if (retval)
+			return retval;
+	}
 
 	return retval;
 }
-- 
2.20.1

[PATCH 2/2] mmc: core: Export device/vendor ids from Common CIS for SDIO cards

[Pali Rohár]

Device/vendor ids from Common CIS (Card Information Structure) may be
different as device/vendor ids from CIS on particular SDIO function.

Export these "main" device/vendor ids for SDIO and SD combo cards at top
level mmc device in sysfs so userspace can do better identification of
connected SDIO and SD combo cards.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/mmc/core/bus.c  |  7 +++++++
 drivers/mmc/core/sd.c   | 26 +++++++++++++++++++++++++-
 drivers/mmc/core/sdio.c | 20 +++++++++++++++++++-
 3 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 103eea7cd..5d4b28b29 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -93,6 +93,13 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 			return retval;
 	}
 
+	if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) {
+		retval = add_uevent_var(env, "SDIO_ID=%04X:%04X",
+					card->cis.vendor, card->cis.device);
+		if (retval)
+			return retval;
+	}
+
 	if (card->type != MMC_TYPE_SDIO) {
 		retval = add_uevent_var(env, "MMC_NAME=%s", mmc_card_name(card));
 		if (retval)
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 76c7add36..ee1a51ff6 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -707,7 +707,12 @@ static ssize_t mmc_dsr_show(struct device *dev,
 
 static DEVICE_ATTR(dsr, S_IRUGO, mmc_dsr_show, NULL);
 
+MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
+MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
+
 static struct attribute *sd_std_attrs[] = {
+	&dev_attr_vendor.attr,
+	&dev_attr_device.attr,
 	&dev_attr_cid.attr,
 	&dev_attr_csd.attr,
 	&dev_attr_scr.attr,
@@ -726,7 +731,26 @@ static struct attribute *sd_std_attrs[] = {
 	&dev_attr_dsr.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(sd_std);
+
+static umode_t sd_std_is_visible(struct kobject *kobj, struct attribute *attr,
+				 int index)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct mmc_card *card = mmc_dev_to_card(dev);
+
+	/* CIS vendor and device ids are available only for Combo cards */
+	if ((attr == &dev_attr_vendor.attr || attr == &dev_attr_device.attr) &&
+	    card->type != MMC_TYPE_SD_COMBO)
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group sd_std_group = {
+	.attrs = sd_std_attrs,
+	.is_visible = sd_std_is_visible,
+};
+__ATTRIBUTE_GROUPS(sd_std);
 
 struct device_type sd_type = {
 	.groups = sd_std_groups,
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index ebb387aa5..d708e0fbc 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -27,6 +27,24 @@
 #include "sdio_ops.h"
 #include "sdio_cis.h"
 
+MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
+MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
+MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
+MMC_DEV_ATTR(rca, "0x%04x\n", card->rca);
+
+static struct attribute *sdio_std_attrs[] = {
+	&dev_attr_vendor.attr,
+	&dev_attr_device.attr,
+	&dev_attr_ocr.attr,
+	&dev_attr_rca.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(sdio_std);
+
+struct device_type sdio_type = {
+	.groups = sdio_std_groups,
+};
+
 static int sdio_read_fbr(struct sdio_func *func)
 {
 	int ret;
@@ -598,7 +616,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	/*
 	 * Allocate card structure.
 	 */
-	card = mmc_alloc_card(host, NULL);
+	card = mmc_alloc_card(host, &sdio_type);
 	if (IS_ERR(card)) {
 		err = PTR_ERR(card);
 		goto err;
-- 
2.20.1

Re: [PATCH 2/2] mmc: core: Export device/vendor ids from Common CIS for SDIO cards

[Ulf Hansson]

On Tue, 26 May 2020 at 17:43, Pali Rohár <pali@kernel.org> wrote:
>
> Device/vendor ids from Common CIS (Card Information Structure) may be
> different as device/vendor ids from CIS on particular SDIO function.
>
> Export these "main" device/vendor ids for SDIO and SD combo cards at top
> level mmc device in sysfs so userspace can do better identification of
> connected SDIO and SD combo cards.

What would userspace do with this information, more exactly?

Isn't it just sufficient to give events per SDIO func, as we already
do in sdio_bus_uevent()?

Kind regards
Uffe

>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/mmc/core/bus.c  |  7 +++++++
>  drivers/mmc/core/sd.c   | 26 +++++++++++++++++++++++++-
>  drivers/mmc/core/sdio.c | 20 +++++++++++++++++++-
>  3 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 103eea7cd..5d4b28b29 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -93,6 +93,13 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>                         return retval;
>         }
>
> +       if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) {
> +               retval = add_uevent_var(env, "SDIO_ID=%04X:%04X",
> +                                       card->cis.vendor, card->cis.device);
> +               if (retval)
> +                       return retval;
> +       }
> +
>         if (card->type != MMC_TYPE_SDIO) {
>                 retval = add_uevent_var(env, "MMC_NAME=%s", mmc_card_name(card));
>                 if (retval)
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 76c7add36..ee1a51ff6 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -707,7 +707,12 @@ static ssize_t mmc_dsr_show(struct device *dev,
>
>  static DEVICE_ATTR(dsr, S_IRUGO, mmc_dsr_show, NULL);
>
> +MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
> +MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
> +
>  static struct attribute *sd_std_attrs[] = {
> +       &dev_attr_vendor.attr,
> +       &dev_attr_device.attr,
>         &dev_attr_cid.attr,
>         &dev_attr_csd.attr,
>         &dev_attr_scr.attr,
> @@ -726,7 +731,26 @@ static struct attribute *sd_std_attrs[] = {
>         &dev_attr_dsr.attr,
>         NULL,
>  };
> -ATTRIBUTE_GROUPS(sd_std);
> +
> +static umode_t sd_std_is_visible(struct kobject *kobj, struct attribute *attr,
> +                                int index)
> +{
> +       struct device *dev = container_of(kobj, struct device, kobj);
> +       struct mmc_card *card = mmc_dev_to_card(dev);
> +
> +       /* CIS vendor and device ids are available only for Combo cards */
> +       if ((attr == &dev_attr_vendor.attr || attr == &dev_attr_device.attr) &&
> +           card->type != MMC_TYPE_SD_COMBO)
> +               return 0;
> +
> +       return attr->mode;
> +}
> +
> +static const struct attribute_group sd_std_group = {
> +       .attrs = sd_std_attrs,
> +       .is_visible = sd_std_is_visible,
> +};
> +__ATTRIBUTE_GROUPS(sd_std);
>
>  struct device_type sd_type = {
>         .groups = sd_std_groups,
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index ebb387aa5..d708e0fbc 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -27,6 +27,24 @@
>  #include "sdio_ops.h"
>  #include "sdio_cis.h"
>
> +MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
> +MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
> +MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
> +MMC_DEV_ATTR(rca, "0x%04x\n", card->rca);
> +
> +static struct attribute *sdio_std_attrs[] = {
> +       &dev_attr_vendor.attr,
> +       &dev_attr_device.attr,
> +       &dev_attr_ocr.attr,
> +       &dev_attr_rca.attr,
> +       NULL,
> +};
> +ATTRIBUTE_GROUPS(sdio_std);
> +
> +struct device_type sdio_type = {
> +       .groups = sdio_std_groups,
> +};
> +
>  static int sdio_read_fbr(struct sdio_func *func)
>  {
>         int ret;
> @@ -598,7 +616,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
>         /*
>          * Allocate card structure.
>          */
> -       card = mmc_alloc_card(host, NULL);
> +       card = mmc_alloc_card(host, &sdio_type);
>         if (IS_ERR(card)) {
>                 err = PTR_ERR(card);
>                 goto err;
> --
> 2.20.1
>

Re: [PATCH 2/2] mmc: core: Export device/vendor ids from Common CIS for SDIO cards

[Pali Rohár]

On Wednesday 27 May 2020 09:39:50 Ulf Hansson wrote:
> On Tue, 26 May 2020 at 17:43, Pali Rohár <pali@kernel.org> wrote:
> >
> > Device/vendor ids from Common CIS (Card Information Structure) may be
> > different as device/vendor ids from CIS on particular SDIO function.
> >
> > Export these "main" device/vendor ids for SDIO and SD combo cards at top
> > level mmc device in sysfs so userspace can do better identification of
> > connected SDIO and SD combo cards.
> 
> What would userspace do with this information, more exactly?

Userspace can e.g. write udev rules based on Common CIS vendor/device
id. Or can exactly identify SDIO card by CIS vendor/device id. Also it
can be suitable for "lssdio" tool to print all information about SDIO
card.

Currently I do not know any way how userspace can retrieve these ids for
particular SDIO card. And correct identification is important.

Other important thing is that kernel on some places (e.g. mmc quirks)
uses Common CIS vendor/device id and on other places (e.g. binding
drivers) it uses SDIO function device/vendor ids.

So for debugging and developing kernel drivers it is needed to know
correct Common CIS vendor/device id and SDIO functions vendor/device
ids.

> Isn't it just sufficient to give events per SDIO func, as we already
> do in sdio_bus_uevent()?

No because some SDIO cards have different Common CIS vendor/device id
and different vendor/device ids for particular SDIO functions.

Common CIS vendor/device id is the "main" identification of SDIO card,
functions vendor/device ids just identify one of those functions.

For example look at my patch "mmc: sdio: Fix macro name for Marvell
device with ID 0x9134" [1]. Without knowing correct CIS vendor/device id
I was not able to fully understand problem and mess with names and ids.
Because mmc quirks list uses CIS vendor/device ids (I guess for obvious
reason as SDIO functions are not enumerated yet).

[1] - https://lore.kernel.org/linux-mmc/20200522144412.19712-2-pali@kernel.org/

> Kind regards
> Uffe
> 
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  drivers/mmc/core/bus.c  |  7 +++++++
> >  drivers/mmc/core/sd.c   | 26 +++++++++++++++++++++++++-
> >  drivers/mmc/core/sdio.c | 20 +++++++++++++++++++-
> >  3 files changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> > index 103eea7cd..5d4b28b29 100644
> > --- a/drivers/mmc/core/bus.c
> > +++ b/drivers/mmc/core/bus.c
> > @@ -93,6 +93,13 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> >                         return retval;
> >         }
> >
> > +       if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) {
> > +               retval = add_uevent_var(env, "SDIO_ID=%04X:%04X",
> > +                                       card->cis.vendor, card->cis.device);
> > +               if (retval)
> > +                       return retval;
> > +       }
> > +
> >         if (card->type != MMC_TYPE_SDIO) {
> >                 retval = add_uevent_var(env, "MMC_NAME=%s", mmc_card_name(card));
> >                 if (retval)
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > index 76c7add36..ee1a51ff6 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -707,7 +707,12 @@ static ssize_t mmc_dsr_show(struct device *dev,
> >
> >  static DEVICE_ATTR(dsr, S_IRUGO, mmc_dsr_show, NULL);
> >
> > +MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
> > +MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
> > +
> >  static struct attribute *sd_std_attrs[] = {
> > +       &dev_attr_vendor.attr,
> > +       &dev_attr_device.attr,
> >         &dev_attr_cid.attr,
> >         &dev_attr_csd.attr,
> >         &dev_attr_scr.attr,
> > @@ -726,7 +731,26 @@ static struct attribute *sd_std_attrs[] = {
> >         &dev_attr_dsr.attr,
> >         NULL,
> >  };
> > -ATTRIBUTE_GROUPS(sd_std);
> > +
> > +static umode_t sd_std_is_visible(struct kobject *kobj, struct attribute *attr,
> > +                                int index)
> > +{
> > +       struct device *dev = container_of(kobj, struct device, kobj);
> > +       struct mmc_card *card = mmc_dev_to_card(dev);
> > +
> > +       /* CIS vendor and device ids are available only for Combo cards */
> > +       if ((attr == &dev_attr_vendor.attr || attr == &dev_attr_device.attr) &&
> > +           card->type != MMC_TYPE_SD_COMBO)
> > +               return 0;
> > +
> > +       return attr->mode;
> > +}
> > +
> > +static const struct attribute_group sd_std_group = {
> > +       .attrs = sd_std_attrs,
> > +       .is_visible = sd_std_is_visible,
> > +};
> > +__ATTRIBUTE_GROUPS(sd_std);
> >
> >  struct device_type sd_type = {
> >         .groups = sd_std_groups,
> > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> > index ebb387aa5..d708e0fbc 100644
> > --- a/drivers/mmc/core/sdio.c
> > +++ b/drivers/mmc/core/sdio.c
> > @@ -27,6 +27,24 @@
> >  #include "sdio_ops.h"
> >  #include "sdio_cis.h"
> >
> > +MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
> > +MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
> > +MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
> > +MMC_DEV_ATTR(rca, "0x%04x\n", card->rca);
> > +
> > +static struct attribute *sdio_std_attrs[] = {
> > +       &dev_attr_vendor.attr,
> > +       &dev_attr_device.attr,
> > +       &dev_attr_ocr.attr,
> > +       &dev_attr_rca.attr,
> > +       NULL,
> > +};
> > +ATTRIBUTE_GROUPS(sdio_std);
> > +
> > +struct device_type sdio_type = {
> > +       .groups = sdio_std_groups,
> > +};
> > +
> >  static int sdio_read_fbr(struct sdio_func *func)
> >  {
> >         int ret;
> > @@ -598,7 +616,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
> >         /*
> >          * Allocate card structure.
> >          */
> > -       card = mmc_alloc_card(host, NULL);
> > +       card = mmc_alloc_card(host, &sdio_type);
> >         if (IS_ERR(card)) {
> >                 err = PTR_ERR(card);
> >                 goto err;
> > --
> > 2.20.1
> >

Re: [PATCH 2/2] mmc: core: Export device/vendor ids from Common CIS for SDIO cards

[Ulf Hansson]

On Wed, 27 May 2020 at 10:00, Pali Rohár <pali@kernel.org> wrote:
>
> On Wednesday 27 May 2020 09:39:50 Ulf Hansson wrote:
> > On Tue, 26 May 2020 at 17:43, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > Device/vendor ids from Common CIS (Card Information Structure) may be
> > > different as device/vendor ids from CIS on particular SDIO function.
> > >
> > > Export these "main" device/vendor ids for SDIO and SD combo cards at top
> > > level mmc device in sysfs so userspace can do better identification of
> > > connected SDIO and SD combo cards.
> >
> > What would userspace do with this information, more exactly?
>
> Userspace can e.g. write udev rules based on Common CIS vendor/device
> id. Or can exactly identify SDIO card by CIS vendor/device id. Also it
> can be suitable for "lssdio" tool to print all information about SDIO
> card.
>
> Currently I do not know any way how userspace can retrieve these ids for
> particular SDIO card. And correct identification is important.
>
> Other important thing is that kernel on some places (e.g. mmc quirks)
> uses Common CIS vendor/device id and on other places (e.g. binding
> drivers) it uses SDIO function device/vendor ids.
>
> So for debugging and developing kernel drivers it is needed to know
> correct Common CIS vendor/device id and SDIO functions vendor/device
> ids.
>
> > Isn't it just sufficient to give events per SDIO func, as we already
> > do in sdio_bus_uevent()?
>
> No because some SDIO cards have different Common CIS vendor/device id
> and different vendor/device ids for particular SDIO functions.
>
> Common CIS vendor/device id is the "main" identification of SDIO card,
> functions vendor/device ids just identify one of those functions.
>
> For example look at my patch "mmc: sdio: Fix macro name for Marvell
> device with ID 0x9134" [1]. Without knowing correct CIS vendor/device id
> I was not able to fully understand problem and mess with names and ids.
> Because mmc quirks list uses CIS vendor/device ids (I guess for obvious
> reason as SDIO functions are not enumerated yet).

Good points, much appreciated to understand the use cases better!

May I suggest that you put some of this information into the commit message?

>
> [1] - https://lore.kernel.org/linux-mmc/20200522144412.19712-2-pali@kernel.org/
>

[...]

Kind regards
Uffe

[PATCH v2 1/2] mmc: core: Do not export MMC_NAME= and MODALIAS=mmc:block for SDIO cards

[Pali Rohár]

SDIO non-combo cards are not handled by mmc_block driver and do not have
accessible CID register which is used for MMC_NAME= construction.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>

---
Changes in V2:
* Use early returns pattern
---
 drivers/mmc/core/bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 74de3f2dd..b1cb447da 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -93,6 +93,13 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 			return retval;
 	}
 
+	/*
+	 * SDIO (non-combo) cards are not handled by mmc_block driver and do not
+	 * have accessible CID register which used by mmc_card_name() function.
+	 */
+	if (card->type == MMC_TYPE_SDIO)
+		return 0;
+
 	retval = add_uevent_var(env, "MMC_NAME=%s", mmc_card_name(card));
 	if (retval)
 		return retval;
-- 
2.20.1

[PATCH v2 2/2] mmc: core: Export device/vendor ids from Common CIS for SDIO cards

[Pali Rohár]

Device/vendor ids from Common CIS (Card Information Structure) may be
different as device/vendor ids from CIS on particular SDIO function.

Kernel currently exports only device/vendor ids from SDIO functions and not
"main" device/vendor ids from Common CIS.

This patch exports "main" device/vendor ids for SDIO and SD combo cards at
top level mmc device in sysfs hierarchy.

Userspace can use e.g. udev rules to correctly match whole SDIO card based
on Common CIS device/vendor id and not only one particular SDIO function.
Having this information in userspace also helps developers to debug whole
SDIO card as e.g. kernel mmc quirks use device/vendor ids from Common CIS
and not from particular SDIO function. Also it allows to write userspace
applications which list all connected SDIO cards based on CIS ids.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>

---
Changes in V2:
* Make sd_std_group static
* Put more information into commit message
---
 drivers/mmc/core/bus.c  |  7 +++++++
 drivers/mmc/core/sd.c   | 26 +++++++++++++++++++++++++-
 drivers/mmc/core/sdio.c | 20 +++++++++++++++++++-
 3 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index b1cb447da..70207f11a 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -93,6 +93,13 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 			return retval;
 	}
 
+	if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) {
+		retval = add_uevent_var(env, "SDIO_ID=%04X:%04X",
+					card->cis.vendor, card->cis.device);
+		if (retval)
+			return retval;
+	}
+
 	/*
 	 * SDIO (non-combo) cards are not handled by mmc_block driver and do not
 	 * have accessible CID register which used by mmc_card_name() function.
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 76c7add36..ee1a51ff6 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -707,7 +707,12 @@ static ssize_t mmc_dsr_show(struct device *dev,
 
 static DEVICE_ATTR(dsr, S_IRUGO, mmc_dsr_show, NULL);
 
+MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
+MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
+
 static struct attribute *sd_std_attrs[] = {
+	&dev_attr_vendor.attr,
+	&dev_attr_device.attr,
 	&dev_attr_cid.attr,
 	&dev_attr_csd.attr,
 	&dev_attr_scr.attr,
@@ -726,7 +731,26 @@ static struct attribute *sd_std_attrs[] = {
 	&dev_attr_dsr.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(sd_std);
+
+static umode_t sd_std_is_visible(struct kobject *kobj, struct attribute *attr,
+				 int index)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct mmc_card *card = mmc_dev_to_card(dev);
+
+	/* CIS vendor and device ids are available only for Combo cards */
+	if ((attr == &dev_attr_vendor.attr || attr == &dev_attr_device.attr) &&
+	    card->type != MMC_TYPE_SD_COMBO)
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group sd_std_group = {
+	.attrs = sd_std_attrs,
+	.is_visible = sd_std_is_visible,
+};
+__ATTRIBUTE_GROUPS(sd_std);
 
 struct device_type sd_type = {
 	.groups = sd_std_groups,
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index ebb387aa5..2d86a9db5 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -27,6 +27,24 @@
 #include "sdio_ops.h"
 #include "sdio_cis.h"
 
+MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
+MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
+MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
+MMC_DEV_ATTR(rca, "0x%04x\n", card->rca);
+
+static struct attribute *sdio_std_attrs[] = {
+	&dev_attr_vendor.attr,
+	&dev_attr_device.attr,
+	&dev_attr_ocr.attr,
+	&dev_attr_rca.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(sdio_std);
+
+static struct device_type sdio_type = {
+	.groups = sdio_std_groups,
+};
+
 static int sdio_read_fbr(struct sdio_func *func)
 {
 	int ret;
@@ -598,7 +616,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	/*
 	 * Allocate card structure.
 	 */
-	card = mmc_alloc_card(host, NULL);
+	card = mmc_alloc_card(host, &sdio_type);
 	if (IS_ERR(card)) {
 		err = PTR_ERR(card);
 		goto err;
-- 
2.20.1

Re: [PATCH v2 2/2] mmc: core: Export device/vendor ids from Common CIS for SDIO cards

[Ulf Hansson]

On Wed, 27 May 2020 at 13:09, Pali Rohár <pali@kernel.org> wrote:
>
> Device/vendor ids from Common CIS (Card Information Structure) may be
> different as device/vendor ids from CIS on particular SDIO function.
>
> Kernel currently exports only device/vendor ids from SDIO functions and not
> "main" device/vendor ids from Common CIS.
>
> This patch exports "main" device/vendor ids for SDIO and SD combo cards at
> top level mmc device in sysfs hierarchy.
>
> Userspace can use e.g. udev rules to correctly match whole SDIO card based
> on Common CIS device/vendor id and not only one particular SDIO function.
> Having this information in userspace also helps developers to debug whole
> SDIO card as e.g. kernel mmc quirks use device/vendor ids from Common CIS
> and not from particular SDIO function. Also it allows to write userspace
> applications which list all connected SDIO cards based on CIS ids.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Applied for next, thanks!

Kind regards
Uffe


>
> ---
> Changes in V2:
> * Make sd_std_group static
> * Put more information into commit message
> ---
>  drivers/mmc/core/bus.c  |  7 +++++++
>  drivers/mmc/core/sd.c   | 26 +++++++++++++++++++++++++-
>  drivers/mmc/core/sdio.c | 20 +++++++++++++++++++-
>  3 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index b1cb447da..70207f11a 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -93,6 +93,13 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>                         return retval;
>         }
>
> +       if (card->type == MMC_TYPE_SDIO || card->type == MMC_TYPE_SD_COMBO) {
> +               retval = add_uevent_var(env, "SDIO_ID=%04X:%04X",
> +                                       card->cis.vendor, card->cis.device);
> +               if (retval)
> +                       return retval;
> +       }
> +
>         /*
>          * SDIO (non-combo) cards are not handled by mmc_block driver and do not
>          * have accessible CID register which used by mmc_card_name() function.
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 76c7add36..ee1a51ff6 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -707,7 +707,12 @@ static ssize_t mmc_dsr_show(struct device *dev,
>
>  static DEVICE_ATTR(dsr, S_IRUGO, mmc_dsr_show, NULL);
>
> +MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
> +MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
> +
>  static struct attribute *sd_std_attrs[] = {
> +       &dev_attr_vendor.attr,
> +       &dev_attr_device.attr,
>         &dev_attr_cid.attr,
>         &dev_attr_csd.attr,
>         &dev_attr_scr.attr,
> @@ -726,7 +731,26 @@ static struct attribute *sd_std_attrs[] = {
>         &dev_attr_dsr.attr,
>         NULL,
>  };
> -ATTRIBUTE_GROUPS(sd_std);
> +
> +static umode_t sd_std_is_visible(struct kobject *kobj, struct attribute *attr,
> +                                int index)
> +{
> +       struct device *dev = container_of(kobj, struct device, kobj);
> +       struct mmc_card *card = mmc_dev_to_card(dev);
> +
> +       /* CIS vendor and device ids are available only for Combo cards */
> +       if ((attr == &dev_attr_vendor.attr || attr == &dev_attr_device.attr) &&
> +           card->type != MMC_TYPE_SD_COMBO)
> +               return 0;
> +
> +       return attr->mode;
> +}
> +
> +static const struct attribute_group sd_std_group = {
> +       .attrs = sd_std_attrs,
> +       .is_visible = sd_std_is_visible,
> +};
> +__ATTRIBUTE_GROUPS(sd_std);
>
>  struct device_type sd_type = {
>         .groups = sd_std_groups,
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index ebb387aa5..2d86a9db5 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -27,6 +27,24 @@
>  #include "sdio_ops.h"
>  #include "sdio_cis.h"
>
> +MMC_DEV_ATTR(vendor, "0x%04x\n", card->cis.vendor);
> +MMC_DEV_ATTR(device, "0x%04x\n", card->cis.device);
> +MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
> +MMC_DEV_ATTR(rca, "0x%04x\n", card->rca);
> +
> +static struct attribute *sdio_std_attrs[] = {
> +       &dev_attr_vendor.attr,
> +       &dev_attr_device.attr,
> +       &dev_attr_ocr.attr,
> +       &dev_attr_rca.attr,
> +       NULL,
> +};
> +ATTRIBUTE_GROUPS(sdio_std);
> +
> +static struct device_type sdio_type = {
> +       .groups = sdio_std_groups,
> +};
> +
>  static int sdio_read_fbr(struct sdio_func *func)
>  {
>         int ret;
> @@ -598,7 +616,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
>         /*
>          * Allocate card structure.
>          */
> -       card = mmc_alloc_card(host, NULL);
> +       card = mmc_alloc_card(host, &sdio_type);
>         if (IS_ERR(card)) {
>                 err = PTR_ERR(card);
>                 goto err;
> --
> 2.20.1
>

Re: [PATCH v2 1/2] mmc: core: Do not export MMC_NAME= and MODALIAS=mmc:block for SDIO cards

[Ulf Hansson]

On Wed, 27 May 2020 at 13:09, Pali Rohár <pali@kernel.org> wrote:
>
> SDIO non-combo cards are not handled by mmc_block driver and do not have
> accessible CID register which is used for MMC_NAME= construction.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Applied for next, thanks!

Kind regards
Uffe


>
> ---
> Changes in V2:
> * Use early returns pattern
> ---
>  drivers/mmc/core/bus.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 74de3f2dd..b1cb447da 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -93,6 +93,13 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>                         return retval;
>         }
>
> +       /*
> +        * SDIO (non-combo) cards are not handled by mmc_block driver and do not
> +        * have accessible CID register which used by mmc_card_name() function.
> +        */
> +       if (card->type == MMC_TYPE_SDIO)
> +               return 0;
> +
>         retval = add_uevent_var(env, "MMC_NAME=%s", mmc_card_name(card));
>         if (retval)
>                 return retval;
> --
> 2.20.1
>

Re: [PATCH 1/2] mmc: core: Do not export MMC_NAME= and MODALIAS=mmc:block for SDIO cards

[Ulf Hansson]

On Tue, 26 May 2020 at 17:43, Pali Rohár <pali@kernel.org> wrote:
>
> SDIO non-combo cards are not handled by mmc_block driver and do not have
> accessible CID register which is used for MMC_NAME= construction.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/mmc/core/bus.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 74de3f2dd..103eea7cd 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -93,15 +93,20 @@ mmc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>                         return retval;
>         }
>
> -       retval = add_uevent_var(env, "MMC_NAME=%s", mmc_card_name(card));
> -       if (retval)
> -               return retval;
> -
> -       /*
> -        * Request the mmc_block device.  Note: that this is a direct request
> -        * for the module it carries no information as to what is inserted.
> -        */
> -       retval = add_uevent_var(env, "MODALIAS=mmc:block");
> +       if (card->type != MMC_TYPE_SDIO) {
> +               retval = add_uevent_var(env, "MMC_NAME=%s", mmc_card_name(card));
> +               if (retval)
> +                       return retval;
> +
> +               /*
> +                * Request the mmc_block device.
> +                * Note: that this is a direct request for the module it carries
> +                * no information as to what is inserted.
> +                */
> +               retval = add_uevent_var(env, "MODALIAS=mmc:block");
> +               if (retval)
> +                       return retval;
> +       }
>
>         return retval;
>  }
> --
> 2.20.1
>

Overall this change makes sense to me, but at nitpick...

Rather than adding new nestled if-sentences, I suggest converting the
function into using the "early returns" pattern instead.

For example, already in the switch loop, above the code you change, we
could just return 0 instead of setting type = NULL. Likewise, you can
check "if (card->type == MMC_TYPE_SDIO) return 0;"

Kind regards
Uffe