[PATCH 0/4] module autoloading fixes

[PATCH 0/4] module autoloading fixes

[Zhang Rui]

Hi, all,

This patch set fixes a couple of module autoloading problem.

Patch 1/4 fixes a bug in ACPI device 'modalias' and 'uevent' attributes,
          although the bug can rarely be reproduced (only if there is an
          output error of snprintf, or the ids are longer than 1024 bytes)
Patch 2/4 introduces two new APIs for exporting ACPI style 'modalias' and
          'uevent' attributes in other buses.
Patch 3/4 introduce support for ACPI style 'modalias' and 'uevent'
          attributes in platform, I2C and SPI bus.
Patch 4/4 add OF style 'modalias' support for platform bus.

I did some tests and can confirm that the code for ACPI enumerated platform
bus device works well.
I tried with a patch with convert ACPI Fan device/driver to platform bus,
and can confirm that the code for ACPI enumerated platform device works well,
both the platform Fan driver and device show their modalias as "acpi:PNP0C0B".

thanks,
rui

----------------------------------------------------------------
Zhang Rui (4):
      ACPI: fix create_modalias() return value handling
      ACPI: add module autoloading support for ACPI enumerated devices
      fix module autoloading for ACPI enumerated devices
      OF: introduce OF style 'modalias' support for platform bus.

 drivers/acpi/scan.c       |   73 +++++++++++++++++++++++++++++++++++++++++----
 drivers/base/platform.c   |   16 +++++++++-
 drivers/i2c/i2c-core.c    |   11 +++++++
 drivers/of/device.c       |    3 ++
 drivers/spi/spi.c         |   10 +++++++
 include/linux/acpi.h      |   15 ++++++++++
 include/linux/of_device.h |    6 ++++
 7 files changed, 127 insertions(+), 7 deletions(-)

[PATCH 1/4] ACPI: fix create_modalias() return value handling

[Zhang Rui]

Currently, create_modalias() handles the output truncated case in
an improper way (return -EINVAL).
Plus, acpi_device_uevent() and acpi_device_modalias_show() do
improper check for the create_modalias() return value as well.

This patch fixes create_modalias() to
return -EINVAL if there is an output error,
return -ENOMEM if the output is truncated,
and also fixes both acpi_device_uevent() and acpi_device_modalias_show()
to do proper return value check.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/scan.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index fd39459..c925321 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -85,6 +85,9 @@ int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
  * Creates hid/cid(s) string needed for modalias and uevent
  * e.g. on a device with hid:IBM0001 and cid:ACPI0001 you get:
  * char *modalias: "acpi:IBM0001:ACPI0001"
+ * Return: 0: no _HID and no _CID
+ *         -EINVAL: output error
+ *         -ENOMEM: output is truncated
 */
 static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
 			   int size)
@@ -101,8 +104,10 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
 
 	list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
 		count = snprintf(&modalias[len], size, "%s:", id->id);
-		if (count < 0 || count >= size)
-			return -EINVAL;
+		if (count < 0)
+			return EINVAL;
+		if (count >= size)
+			return -ENOMEM;
 		len += count;
 		size -= count;
 	}
@@ -116,10 +121,9 @@ acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, cha
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
 	int len;
 
-	/* Device has no HID and no CID or string is >1024 */
 	len = create_modalias(acpi_dev, buf, 1024);
 	if (len <= 0)
-		return 0;
+		return len;
 	buf[len++] = '\n';
 	return len;
 }
@@ -782,8 +786,8 @@ static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 		return -ENOMEM;
 	len = create_modalias(acpi_dev, &env->buf[env->buflen - 1],
 			      sizeof(env->buf) - env->buflen);
-	if (len >= (sizeof(env->buf) - env->buflen))
-		return -ENOMEM;
+	if (len <= 0)
+		return len;
 	env->buflen += len;
 	return 0;
 }
-- 
1.7.9.5

[PATCH 2/4] ACPI: add module autoloading support for ACPI enumerated devices

[Zhang Rui]

An ACPI enumerated device may have its compatible id strings.

To support the compatible ACPI ids (acpi_device->pnp.ids),
we introduced acpi_driver_match_device() to match
the driver->acpi_match_table and acpi_device->pnp.ids.

For those drivers, MODULE_DEVICE_TABLE(acpi, xxx) is used to
exports the driver module alias in the format of
"acpi:device_compatible_ids".
But in the mean time, the current code does not export the
ACPI compatible strings as part of the module_alias for the
ACPI enumerated devices, which will break the module autoloading.

Take the following piece of code for example,
static const struct acpi_device_id xxx_acpi_match[] = {
        { "INTABCD", 0 },
        { }
};
MODULE_DEVICE_TABLE(acpi, xxx_acpi_match);

If this piece of code is used in a platform driver for
an ACPI enumerated platform device, the platform driver module_alias
is "acpi:INTABCD", but the uevent attribute of its platform device node
is "platform:INTABCD:00" (PREFIX:platform_device->name).
If this piece of code is used in an i2c driver for an ACPI enumerated
i2c device, the i2c driver module_alias is "acpi:INTABCD", but
the uevent of its i2c device node is "i2c:INTABCD:00" (PREFIX:i2c_client->name).
If this piece of code is used in an spi driver for an ACPI enumerated
spi device, the spi driver module_alias is "acpi:INTABCD", but
the uevent of its spi device node is "spi:INTABCD" (PREFIX:spi_device->modalias).

The reason why the module autoloading is not broken for now is that
the uevent file of the ACPI device node is "acpi:INTABCD".
Thus it is the ACPI device node creation that loads the platform/i2c/spi driver.

So this is a problem that will affect us the day when the ACPI bus
is removed from device model.

This patch introduces two new APIs,
one for exporting ACPI ids in uevent MODALIAS field,
and another for exporting ACPI ids in device' modalias sysfs attribute.

For any bus that supports ACPI enumerated devices, it needs to invoke
these two functions for their uevent and modalias attribute.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/scan.c  |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h |   15 +++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index c925321..680bb56 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -116,6 +116,63 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
 	return len;
 }
 
+/*
+ * Creates uevent modalias field for ACPI enumerated devices.
+ * Because the other buses does not support ACPI HIDs & CIDs.
+ * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
+ * "acpi:IBM0001:ACPI0001"
+ */
+int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
+{
+	struct acpi_device *acpi_dev;
+	int len;
+
+	acpi_dev = ACPI_COMPANION(dev);
+	if (!acpi_dev)
+		return -ENODEV;
+
+	/* Fall back to bus specific way of modalias exporting */
+	if (list_empty(&acpi_dev->pnp.ids))
+		return -ENODEV;
+
+	if (add_uevent_var(env, "MODALIAS="))
+		return -ENOMEM;
+	len = create_modalias(acpi_dev, &env->buf[env->buflen - 1],
+				sizeof(env->buf) - env->buflen);
+	if (len <= 0)
+		return len;
+	env->buflen += len;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
+
+/*
+ * Creates modalias sysfs attribute for ACPI enumerated devices.
+ * Because the other buses does not support ACPI HIDs & CIDs.
+ * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
+ * "acpi:IBM0001:ACPI0001"
+ */
+int acpi_device_modalias(struct device *dev, char *buf, int size)
+{
+	struct acpi_device *acpi_dev;
+	int len;
+
+	acpi_dev = ACPI_COMPANION(dev);
+	if (!acpi_dev)
+		return -ENODEV;
+
+	/* Fall back to bus specific way of modalias exporting */
+	if (list_empty(&acpi_dev->pnp.ids))
+		return -ENODEV;
+
+	len = create_modalias(acpi_dev, buf, size -1);
+	if (len <= 0)
+		return len;
+	buf[len++] = '\n';
+	return len;
+}
+EXPORT_SYMBOL_GPL(acpi_device_modalias);
+
 static ssize_t
 acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, char *buf) {
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d9099b1..22325ed 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -409,6 +409,9 @@ static inline bool acpi_driver_match_device(struct device *dev,
 	return !!acpi_match_device(drv->acpi_match_table, dev);
 }
 
+int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
+int acpi_device_modalias(struct device *, char *, int);
+
 #define ACPI_PTR(_ptr)	(_ptr)
 
 #else	/* !CONFIG_ACPI */
@@ -488,6 +491,18 @@ static inline bool acpi_driver_match_device(struct device *dev,
 	return false;
 }
 
+static inline int acpi_device_uevent_modalias(struct device *dev,
+				struct kobj_uevent_env *env)
+{
+	return -ENODEV;
+}
+
+static inline int acpi_device_modalias(struct device *dev,
+				char *buf, int size)
+{
+	return -ENODEV;
+}
+
 #define ACPI_PTR(_ptr)	(NULL)
 
 #endif	/* !CONFIG_ACPI */
-- 
1.7.9.5

[PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Zhang Rui]

ACPI enumerated devices has ACPI style _HID and _CID strings,
all of these strings can be used for both driver loading and matching.

Currently, in Platform, I2C and SPI bus, the ACPI style driver matching
is supported by invoking acpi_driver_match_device() in bus .match() callback.
But, the module autoloading is still broken.

For example, there is any ACPI device with _HID "INTABCD" that is
enumerated to platform bus, and we have a driver that can probe it.

The driver exports its module_alias as "acpi:INTABCD" use the following code
static const struct acpi_device_id xxx_acpi_match[] = {
        { "INTABCD", 0 },
        { }
};
MODULE_DEVICE_TABLE(acpi, xxx_acpi_match);

But, unfortunately, the device' modalias is shown as "platform:INTABCD:00",
please refer to modalias_show() and platform_uevent() in
drivers/base/platform.c.
This results in that the driver will not be loaded automatically when the
device node is created, because their modalias do not match.

This also applies to I2C and SPI bus.

With this patch, the device' modalias will be shown as "acpi:INTABCD" as well.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/base/platform.c |   12 +++++++++++-
 drivers/i2c/i2c-core.c  |   11 +++++++++++
 drivers/spi/spi.c       |   10 ++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3a94b79..2f4aea2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
 			     char *buf)
 {
 	struct platform_device	*pdev = to_platform_device(dev);
-	int len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
+	int len;
+
+	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
+	if (len != -ENODEV)
+		return len;
+
+	len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
 
 	return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
 }
@@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env)
 	if (rc != -ENODEV)
 		return rc;
 
+	rc = acpi_device_uevent_modalias(dev, env);
+	if (rc != -ENODEV)
+		return rc;
+
 	add_uevent_var(env, "MODALIAS=%s%s", PLATFORM_MODULE_PREFIX,
 			pdev->name);
 	return 0;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d74c0b3..c4c5588 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
 static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct i2c_client	*client = to_i2c_client(dev);
+	int rc;
+
+	rc = acpi_device_uevent_modalias(dev, env);
+	if (rc != -ENODEV)
+		return rc;
 
 	if (add_uevent_var(env, "MODALIAS=%s%s",
 			   I2C_MODULE_PREFIX, client->name))
@@ -409,6 +414,12 @@ static ssize_t
 show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	int len;
+
+	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
+	if (len != -ENODEV)
+		return len;
+
 	return sprintf(buf, "%s%s\n", I2C_MODULE_PREFIX, client->name);
 }
 
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 349ebba..ab70eda 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -58,6 +58,11 @@ static ssize_t
 modalias_show(struct device *dev, struct device_attribute *a, char *buf)
 {
 	const struct spi_device	*spi = to_spi_device(dev);
+	int len;
+
+	len = acpi_device_modalias(dev, buf, PAGE_SIZE);
+	if (len != -ENODEV)
+		return len;
 
 	return sprintf(buf, "%s%s\n", SPI_MODULE_PREFIX, spi->modalias);
 }
@@ -114,6 +119,11 @@ static int spi_match_device(struct device *dev, struct device_driver *drv)
 static int spi_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	const struct spi_device		*spi = to_spi_device(dev);
+	int rc;
+
+	rc = acpi_device_uevent_modalias(dev, env);
+	if (rc != -ENODEV)
+		return rc;
 
 	add_uevent_var(env, "MODALIAS=%s%s", SPI_MODULE_PREFIX, spi->modalias);
 	return 0;
-- 
1.7.9.5

[PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus.

[Zhang Rui]

Fix a problem that, the platform bus supports the OF style modalias
in .uevent() call, but not in its device 'modalias' sysfs attribute.

cc: devicetree@vger.kernel.org
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/base/platform.c   |    4 ++++
 drivers/of/device.c       |    3 +++
 include/linux/of_device.h |    6 ++++++
 3 files changed, 13 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 2f4aea2..bc78848 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -679,6 +679,10 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
 	struct platform_device	*pdev = to_platform_device(dev);
 	int len;
 
+	len = of_device_get_modalias(dev, buf, PAGE_SIZE -1);
+	if (len != -ENODEV)
+		return len;
+
 	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
 	if (len != -ENODEV)
 		return len;
diff --git a/drivers/of/device.c b/drivers/of/device.c
index f685e55..dafb973 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -85,6 +85,9 @@ ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len)
 	int cplen, i;
 	ssize_t tsize, csize, repend;
 
+	if ((!dev) || (!dev->of_node))
+		return -ENODEV;
+
 	/* Name & Type */
 	csize = snprintf(str, len, "of:N%sT%s", dev->of_node->name,
 			 dev->of_node->type);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 82ce324..8d7dd67 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -64,6 +64,12 @@ static inline int of_driver_match_device(struct device *dev,
 static inline void of_device_uevent(struct device *dev,
 			struct kobj_uevent_env *env) { }
 
+static inline int of_device_get_modalias(struct device *dev,
+				   char *str, ssize_t len)
+{
+	return -ENODEV;
+}
+
 static inline int of_device_uevent_modalias(struct device *dev,
 				   struct kobj_uevent_env *env)
 {
-- 
1.7.9.5

Re: [PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus.

[Rob Herring]

On Tue, Jan 14, 2014 at 2:46 AM, Zhang Rui <rui.zhang@intel.com> wrote:
> Fix a problem that, the platform bus supports the OF style modalias
> in .uevent() call, but not in its device 'modalias' sysfs attribute.
>
> cc: devicetree@vger.kernel.org
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Acked-by: Rob Herring <robh@kernel.org>

As there doesn't appear any dependency with the rest of this series, I
can take this.

One minor nit below.

> ---
>  drivers/base/platform.c   |    4 ++++
>  drivers/of/device.c       |    3 +++
>  include/linux/of_device.h |    6 ++++++
>  3 files changed, 13 insertions(+)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 2f4aea2..bc78848 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -679,6 +679,10 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
>         struct platform_device  *pdev = to_platform_device(dev);
>         int len;
>
> +       len = of_device_get_modalias(dev, buf, PAGE_SIZE -1);
> +       if (len != -ENODEV)
> +               return len;
> +
>         len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
>         if (len != -ENODEV)
>                 return len;
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index f685e55..dafb973 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -85,6 +85,9 @@ ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len)
>         int cplen, i;
>         ssize_t tsize, csize, repend;
>
> +       if ((!dev) || (!dev->of_node))\

Don't need the parentheses here.

> +               return -ENODEV;
> +
>         /* Name & Type */
>         csize = snprintf(str, len, "of:N%sT%s", dev->of_node->name,
>                          dev->of_node->type);
> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> index 82ce324..8d7dd67 100644
> --- a/include/linux/of_device.h
> +++ b/include/linux/of_device.h
> @@ -64,6 +64,12 @@ static inline int of_driver_match_device(struct device *dev,
>  static inline void of_device_uevent(struct device *dev,
>                         struct kobj_uevent_env *env) { }
>
> +static inline int of_device_get_modalias(struct device *dev,
> +                                  char *str, ssize_t len)
> +{
> +       return -ENODEV;
> +}
> +
>  static inline int of_device_uevent_modalias(struct device *dev,
>                                    struct kobj_uevent_env *env)
>  {
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] ACPI: fix create_modalias() return value handling

[Mika Westerberg]

On Tue, Jan 14, 2014 at 04:46:35PM +0800, Zhang Rui wrote:
> Currently, create_modalias() handles the output truncated case in
> an improper way (return -EINVAL).
> Plus, acpi_device_uevent() and acpi_device_modalias_show() do
> improper check for the create_modalias() return value as well.
> 
> This patch fixes create_modalias() to
> return -EINVAL if there is an output error,
> return -ENOMEM if the output is truncated,
> and also fixes both acpi_device_uevent() and acpi_device_modalias_show()
> to do proper return value check.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Mika Westerberg]

On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote:
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 3a94b79..2f4aea2 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
>  			     char *buf)
>  {
>  	struct platform_device	*pdev = to_platform_device(dev);
> -	int len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
> +	int len;
> +
> +	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);

Here you have PAGE_SIZE -1...

> +	if (len != -ENODEV)
> +		return len;
> +
> +	len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
>  
>  	return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
>  }
> @@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env)
>  	if (rc != -ENODEV)
>  		return rc;
>  
> +	rc = acpi_device_uevent_modalias(dev, env);
> +	if (rc != -ENODEV)
> +		return rc;
> +
>  	add_uevent_var(env, "MODALIAS=%s%s", PLATFORM_MODULE_PREFIX,
>  			pdev->name);
>  	return 0;
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index d74c0b3..c4c5588 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
>  static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
>  	struct i2c_client	*client = to_i2c_client(dev);
> +	int rc;
> +
> +	rc = acpi_device_uevent_modalias(dev, env);
> +	if (rc != -ENODEV)
> +		return rc;
>  
>  	if (add_uevent_var(env, "MODALIAS=%s%s",
>  			   I2C_MODULE_PREFIX, client->name))
> @@ -409,6 +414,12 @@ static ssize_t
>  show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> +	int len;
> +
> +	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);

and here

> +	if (len != -ENODEV)
> +		return len;
> +
>  	return sprintf(buf, "%s%s\n", I2C_MODULE_PREFIX, client->name);
>  }
>  
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 349ebba..ab70eda 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -58,6 +58,11 @@ static ssize_t
>  modalias_show(struct device *dev, struct device_attribute *a, char *buf)
>  {
>  	const struct spi_device	*spi = to_spi_device(dev);
> +	int len;
> +
> +	len = acpi_device_modalias(dev, buf, PAGE_SIZE);

but here it is PAGE_SIZE.

Perhaps it should be PAGE_SIZE in all sites?

Re: [PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus.

[Rafael J. Wysocki]

On Wednesday, January 15, 2014 07:45:18 AM Rob Herring wrote:
> On Tue, Jan 14, 2014 at 2:46 AM, Zhang Rui <rui.zhang@intel.com> wrote:
> > Fix a problem that, the platform bus supports the OF style modalias
> > in .uevent() call, but not in its device 'modalias' sysfs attribute.
> >
> > cc: devicetree@vger.kernel.org
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 
> As there doesn't appear any dependency with the rest of this series, I
> can take this.

Please do, thanks!

> One minor nit below.
> 
> > ---
> >  drivers/base/platform.c   |    4 ++++
> >  drivers/of/device.c       |    3 +++
> >  include/linux/of_device.h |    6 ++++++
> >  3 files changed, 13 insertions(+)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 2f4aea2..bc78848 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -679,6 +679,10 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
> >         struct platform_device  *pdev = to_platform_device(dev);
> >         int len;
> >
> > +       len = of_device_get_modalias(dev, buf, PAGE_SIZE -1);
> > +       if (len != -ENODEV)
> > +               return len;
> > +
> >         len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
> >         if (len != -ENODEV)
> >                 return len;
> > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > index f685e55..dafb973 100644
> > --- a/drivers/of/device.c
> > +++ b/drivers/of/device.c
> > @@ -85,6 +85,9 @@ ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len)
> >         int cplen, i;
> >         ssize_t tsize, csize, repend;
> >
> > +       if ((!dev) || (!dev->of_node))\
> 
> Don't need the parentheses here.
> 
> > +               return -ENODEV;
> > +
> >         /* Name & Type */
> >         csize = snprintf(str, len, "of:N%sT%s", dev->of_node->name,
> >                          dev->of_node->type);
> > diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> > index 82ce324..8d7dd67 100644
> > --- a/include/linux/of_device.h
> > +++ b/include/linux/of_device.h
> > @@ -64,6 +64,12 @@ static inline int of_driver_match_device(struct device *dev,
> >  static inline void of_device_uevent(struct device *dev,
> >                         struct kobj_uevent_env *env) { }
> >
> > +static inline int of_device_get_modalias(struct device *dev,
> > +                                  char *str, ssize_t len)
> > +{
> > +       return -ENODEV;
> > +}
> > +
> >  static inline int of_device_uevent_modalias(struct device *dev,
> >                                    struct kobj_uevent_env *env)
> >  {
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus.

[Zhang, Rui]

> -----Original Message-----
> From: Rob Herring [mailto:robherring2@gmail.com]
> Sent: Wednesday, January 15, 2014 9:45 PM
> To: Zhang, Rui
> Cc: linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; linux-
> i2c@vger.kernel.org; linux-spi@vger.kernel.org; wsa@the-dreams.de; Mark
> Brown; Greg Kroah-Hartman; Wysocki, Rafael J; Grant Likely; Rob Herring;
> jarkko.nikula@linux.intel.com; mika.westerberg@linux.intel.com;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH 4/4] OF: introduce OF style 'modalias' support for
> platform bus.
> Importance: High
> 
> On Tue, Jan 14, 2014 at 2:46 AM, Zhang Rui <rui.zhang@intel.com> wrote:
> > Fix a problem that, the platform bus supports the OF style modalias
> in
> > .uevent() call, but not in its device 'modalias' sysfs attribute.
> >
> > cc: devicetree@vger.kernel.org
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 
> As there doesn't appear any dependency with the rest of this series, I
> can take this.
> 
Thanks.

-rui
> One minor nit below.
> 
> > ---
> >  drivers/base/platform.c   |    4 ++++
> >  drivers/of/device.c       |    3 +++
> >  include/linux/of_device.h |    6 ++++++
> >  3 files changed, 13 insertions(+)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c index
> > 2f4aea2..bc78848 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -679,6 +679,10 @@ static ssize_t modalias_show(struct device *dev,
> struct device_attribute *a,
> >         struct platform_device  *pdev = to_platform_device(dev);
> >         int len;
> >
> > +       len = of_device_get_modalias(dev, buf, PAGE_SIZE -1);
> > +       if (len != -ENODEV)
> > +               return len;
> > +
> >         len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
> >         if (len != -ENODEV)
> >                 return len;
> > diff --git a/drivers/of/device.c b/drivers/of/device.c index
> > f685e55..dafb973 100644
> > --- a/drivers/of/device.c
> > +++ b/drivers/of/device.c
> > @@ -85,6 +85,9 @@ ssize_t of_device_get_modalias(struct device *dev,
> char *str, ssize_t len)
> >         int cplen, i;
> >         ssize_t tsize, csize, repend;
> >
> > +       if ((!dev) || (!dev->of_node))\
> 
> Don't need the parentheses here.
> 
> > +               return -ENODEV;
> > +
> >         /* Name & Type */
> >         csize = snprintf(str, len, "of:N%sT%s", dev->of_node->name,
> >                          dev->of_node->type); diff --git
> > a/include/linux/of_device.h b/include/linux/of_device.h index
> > 82ce324..8d7dd67 100644
> > --- a/include/linux/of_device.h
> > +++ b/include/linux/of_device.h
> > @@ -64,6 +64,12 @@ static inline int of_driver_match_device(struct
> > device *dev,  static inline void of_device_uevent(struct device *dev,
> >                         struct kobj_uevent_env *env) { }
> >
> > +static inline int of_device_get_modalias(struct device *dev,
> > +                                  char *str, ssize_t len) {
> > +       return -ENODEV;
> > +}
> > +
> >  static inline int of_device_uevent_modalias(struct device *dev,
> >                                    struct kobj_uevent_env *env)  {
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Zhang Rui]

On Wed, 2014-01-15 at 17:08 +0200, Mika Westerberg wrote:
> On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote:
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 3a94b79..2f4aea2 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
> >  			     char *buf)
> >  {
> >  	struct platform_device	*pdev = to_platform_device(dev);
> > -	int len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
> > +	int len;
> > +
> > +	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
> 
> Here you have PAGE_SIZE -1...
> 
> > +	if (len != -ENODEV)
> > +		return len;
> > +
> > +	len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
> >  
> >  	return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
> >  }
> > @@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env)
> >  	if (rc != -ENODEV)
> >  		return rc;
> >  
> > +	rc = acpi_device_uevent_modalias(dev, env);
> > +	if (rc != -ENODEV)
> > +		return rc;
> > +
> >  	add_uevent_var(env, "MODALIAS=%s%s", PLATFORM_MODULE_PREFIX,
> >  			pdev->name);
> >  	return 0;
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index d74c0b3..c4c5588 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
> >  static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> >  {
> >  	struct i2c_client	*client = to_i2c_client(dev);
> > +	int rc;
> > +
> > +	rc = acpi_device_uevent_modalias(dev, env);
> > +	if (rc != -ENODEV)
> > +		return rc;
> >  
> >  	if (add_uevent_var(env, "MODALIAS=%s%s",
> >  			   I2C_MODULE_PREFIX, client->name))
> > @@ -409,6 +414,12 @@ static ssize_t
> >  show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
> >  {
> >  	struct i2c_client *client = to_i2c_client(dev);
> > +	int len;
> > +
> > +	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
> 
> and here
> 
> > +	if (len != -ENODEV)
> > +		return len;
> > +
> >  	return sprintf(buf, "%s%s\n", I2C_MODULE_PREFIX, client->name);
> >  }
> >  
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 349ebba..ab70eda 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -58,6 +58,11 @@ static ssize_t
> >  modalias_show(struct device *dev, struct device_attribute *a, char *buf)
> >  {
> >  	const struct spi_device	*spi = to_spi_device(dev);
> > +	int len;
> > +
> > +	len = acpi_device_modalias(dev, buf, PAGE_SIZE);
> 
> but here it is PAGE_SIZE.
> 
good catch.

> Perhaps it should be PAGE_SIZE in all sites?

dev_attr_show() will give a warning message if modalias_show() returns
PAGE_SIZE, thus I'd prefer to use PAGE_SIZE - 1 for all sites.

thanks,
rui

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]


> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index d74c0b3..c4c5588 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
>  static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
>  	struct i2c_client	*client = to_i2c_client(dev);
> +	int rc;
> +
> +	rc = acpi_device_uevent_modalias(dev, env);
> +	if (rc != -ENODEV)
> +		return rc;
>  
>  	if (add_uevent_var(env, "MODALIAS=%s%s",
>  			   I2C_MODULE_PREFIX, client->name))

I wonder why we don't have/need that with CONFIG_OF? Because probably
nobody is using modules with i2c devices there?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 561 bytes --]

On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote:
> ACPI enumerated devices has ACPI style _HID and _CID strings,
> all of these strings can be used for both driver loading and matching.
> 
> Currently, in Platform, I2C and SPI bus, the ACPI style driver matching
> is supported by invoking acpi_driver_match_device() in bus .match() callback.
> But, the module autoloading is still broken.

Acked-by: Mark Brown <broonie@linaro.org>

modulo the PAGE_SIZE stuff Mika noted - unless this changes radically
please just assume I'm OK with it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Zhang Rui]

On Thu, 2014-01-16 at 12:28 +0000, Mark Brown wrote:
> On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote:
> > ACPI enumerated devices has ACPI style _HID and _CID strings,
> > all of these strings can be used for both driver loading and matching.
> > 
> > Currently, in Platform, I2C and SPI bus, the ACPI style driver matching
> > is supported by invoking acpi_driver_match_device() in bus .match() callback.
> > But, the module autoloading is still broken.
> 
> Acked-by: Mark Brown <broonie@linaro.org>
> 
> modulo the PAGE_SIZE stuff Mika noted - unless this changes radically
> please just assume I'm OK with it.

thanks.

-rui

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Zhang Rui]

On Thu, 2014-01-16 at 13:27 +0100, Wolfram Sang wrote:
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index d74c0b3..c4c5588 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
> >  static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> >  {
> >  	struct i2c_client	*client = to_i2c_client(dev);
> > +	int rc;
> > +
> > +	rc = acpi_device_uevent_modalias(dev, env);
> > +	if (rc != -ENODEV)
> > +		return rc;
> >  
> >  	if (add_uevent_var(env, "MODALIAS=%s%s",
> >  			   I2C_MODULE_PREFIX, client->name))
> 
> I wonder why we don't have/need that with CONFIG_OF? Because probably
> nobody is using modules with i2c devices there?
> 
This seems a gap to me but I'm not 100% sure.
I saw Grant Likely introduced the OF style MODALIAS to platform bus, and
OF style registration/binding to i2c bus, maybe he has an answer for
this.

thanks,
rui

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

On Thu, Jan 16, 2014 at 12:28:19PM +0000, Mark Brown wrote:
> On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote:
> > ACPI enumerated devices has ACPI style _HID and _CID strings,
> > all of these strings can be used for both driver loading and matching.
> > 
> > Currently, in Platform, I2C and SPI bus, the ACPI style driver matching
> > is supported by invoking acpi_driver_match_device() in bus .match() callback.
> > But, the module autoloading is still broken.
> 
> Acked-by: Mark Brown <broonie@linaro.org>
> 
> modulo the PAGE_SIZE stuff Mika noted - unless this changes radically
> please just assume I'm OK with it.

Ditto

Acked-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

On Thu, Jan 16, 2014 at 09:05:09PM +0800, Zhang Rui wrote:
> On Thu, 2014-01-16 at 13:27 +0100, Wolfram Sang wrote:

> > I wonder why we don't have/need that with CONFIG_OF? Because probably
> > nobody is using modules with i2c devices there?

> This seems a gap to me but I'm not 100% sure.
> I saw Grant Likely introduced the OF style MODALIAS to platform bus, and
> OF style registration/binding to i2c bus, maybe he has an answer for
> this.

This is needed for ACPI because we rewrite the device names to give us
stable names.  With OF for I2C and SPI nothing bus specific is done that 
affects this stuff so the default behaviour works.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/4] OF: introduce OF style 'modalias' support for platform bus.

[Rob Herring]

On Thu, Jan 16, 2014 at 1:04 AM, Zhang, Rui <rui.zhang@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Rob Herring [mailto:robherring2@gmail.com]
>> Sent: Wednesday, January 15, 2014 9:45 PM
>> To: Zhang, Rui
>> Cc: linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; linux-
>> i2c@vger.kernel.org; linux-spi@vger.kernel.org; wsa@the-dreams.de; Mark
>> Brown; Greg Kroah-Hartman; Wysocki, Rafael J; Grant Likely; Rob Herring;
>> jarkko.nikula@linux.intel.com; mika.westerberg@linux.intel.com;
>> devicetree@vger.kernel.org
>> Subject: Re: [PATCH 4/4] OF: introduce OF style 'modalias' support for
>> platform bus.
>> Importance: High
>>
>> On Tue, Jan 14, 2014 at 2:46 AM, Zhang Rui <rui.zhang@intel.com> wrote:
>> > Fix a problem that, the platform bus supports the OF style modalias
>> in
>> > .uevent() call, but not in its device 'modalias' sysfs attribute.
>> >
>> > cc: devicetree@vger.kernel.org
>> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>>
>> Acked-by: Rob Herring <robh@kernel.org>
>>
>> As there doesn't appear any dependency with the rest of this series, I
>> can take this.
>>
> Thanks.
>

Looks like there is actually a dependency in modalias_show, and the
comment about PAGE_SIZE would apply here too I think. So the whole
series should be taken together.

Rob

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Rafael J. Wysocki]

On Thursday, January 16, 2014 04:04:35 PM Zhang Rui wrote:
> On Wed, 2014-01-15 at 17:08 +0200, Mika Westerberg wrote:
> > On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote:
> > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > > index 3a94b79..2f4aea2 100644
> > > --- a/drivers/base/platform.c
> > > +++ b/drivers/base/platform.c
> > > @@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
> > >  			     char *buf)
> > >  {
> > >  	struct platform_device	*pdev = to_platform_device(dev);
> > > -	int len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
> > > +	int len;
> > > +
> > > +	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
> > 
> > Here you have PAGE_SIZE -1...
> > 
> > > +	if (len != -ENODEV)
> > > +		return len;
> > > +
> > > +	len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
> > >  
> > >  	return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
> > >  }
> > > @@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env)
> > >  	if (rc != -ENODEV)
> > >  		return rc;
> > >  
> > > +	rc = acpi_device_uevent_modalias(dev, env);
> > > +	if (rc != -ENODEV)
> > > +		return rc;
> > > +
> > >  	add_uevent_var(env, "MODALIAS=%s%s", PLATFORM_MODULE_PREFIX,
> > >  			pdev->name);
> > >  	return 0;
> > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > index d74c0b3..c4c5588 100644
> > > --- a/drivers/i2c/i2c-core.c
> > > +++ b/drivers/i2c/i2c-core.c
> > > @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
> > >  static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> > >  {
> > >  	struct i2c_client	*client = to_i2c_client(dev);
> > > +	int rc;
> > > +
> > > +	rc = acpi_device_uevent_modalias(dev, env);
> > > +	if (rc != -ENODEV)
> > > +		return rc;
> > >  
> > >  	if (add_uevent_var(env, "MODALIAS=%s%s",
> > >  			   I2C_MODULE_PREFIX, client->name))
> > > @@ -409,6 +414,12 @@ static ssize_t
> > >  show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
> > >  {
> > >  	struct i2c_client *client = to_i2c_client(dev);
> > > +	int len;
> > > +
> > > +	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
> > 
> > and here
> > 
> > > +	if (len != -ENODEV)
> > > +		return len;
> > > +
> > >  	return sprintf(buf, "%s%s\n", I2C_MODULE_PREFIX, client->name);
> > >  }
> > >  
> > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > > index 349ebba..ab70eda 100644
> > > --- a/drivers/spi/spi.c
> > > +++ b/drivers/spi/spi.c
> > > @@ -58,6 +58,11 @@ static ssize_t
> > >  modalias_show(struct device *dev, struct device_attribute *a, char *buf)
> > >  {
> > >  	const struct spi_device	*spi = to_spi_device(dev);
> > > +	int len;
> > > +
> > > +	len = acpi_device_modalias(dev, buf, PAGE_SIZE);
> > 
> > but here it is PAGE_SIZE.
> > 
> good catch.
> 
> > Perhaps it should be PAGE_SIZE in all sites?
> 
> dev_attr_show() will give a warning message if modalias_show() returns
> PAGE_SIZE, thus I'd prefer to use PAGE_SIZE - 1 for all sites.

So I changed the PAGE_SIZE to PAGE_SIZE - 1 in the last instance and queued
up the whole series for 3.14 in linux-pm.git/linux-next.  Please have a look
at that and let me know if there's anything wrong with it.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Zhang Rui]

On Fri, 2014-01-17 at 02:28 +0100, Rafael J. Wysocki wrote:
> On Thursday, January 16, 2014 04:04:35 PM Zhang Rui wrote:
> > On Wed, 2014-01-15 at 17:08 +0200, Mika Westerberg wrote:
> > > On Tue, Jan 14, 2014 at 04:46:37PM +0800, Zhang Rui wrote:
> > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > > > index 3a94b79..2f4aea2 100644
> > > > --- a/drivers/base/platform.c
> > > > +++ b/drivers/base/platform.c
> > > > @@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
> > > >  			     char *buf)
> > > >  {
> > > >  	struct platform_device	*pdev = to_platform_device(dev);
> > > > -	int len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
> > > > +	int len;
> > > > +
> > > > +	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
> > > 
> > > Here you have PAGE_SIZE -1...
> > > 
> > > > +	if (len != -ENODEV)
> > > > +		return len;
> > > > +
> > > > +	len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
> > > >  
> > > >  	return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
> > > >  }
> > > > @@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env)
> > > >  	if (rc != -ENODEV)
> > > >  		return rc;
> > > >  
> > > > +	rc = acpi_device_uevent_modalias(dev, env);
> > > > +	if (rc != -ENODEV)
> > > > +		return rc;
> > > > +
> > > >  	add_uevent_var(env, "MODALIAS=%s%s", PLATFORM_MODULE_PREFIX,
> > > >  			pdev->name);
> > > >  	return 0;
> > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > > index d74c0b3..c4c5588 100644
> > > > --- a/drivers/i2c/i2c-core.c
> > > > +++ b/drivers/i2c/i2c-core.c
> > > > @@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
> > > >  static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> > > >  {
> > > >  	struct i2c_client	*client = to_i2c_client(dev);
> > > > +	int rc;
> > > > +
> > > > +	rc = acpi_device_uevent_modalias(dev, env);
> > > > +	if (rc != -ENODEV)
> > > > +		return rc;
> > > >  
> > > >  	if (add_uevent_var(env, "MODALIAS=%s%s",
> > > >  			   I2C_MODULE_PREFIX, client->name))
> > > > @@ -409,6 +414,12 @@ static ssize_t
> > > >  show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
> > > >  {
> > > >  	struct i2c_client *client = to_i2c_client(dev);
> > > > +	int len;
> > > > +
> > > > +	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
> > > 
> > > and here
> > > 
> > > > +	if (len != -ENODEV)
> > > > +		return len;
> > > > +
> > > >  	return sprintf(buf, "%s%s\n", I2C_MODULE_PREFIX, client->name);
> > > >  }
> > > >  
> > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > > > index 349ebba..ab70eda 100644
> > > > --- a/drivers/spi/spi.c
> > > > +++ b/drivers/spi/spi.c
> > > > @@ -58,6 +58,11 @@ static ssize_t
> > > >  modalias_show(struct device *dev, struct device_attribute *a, char *buf)
> > > >  {
> > > >  	const struct spi_device	*spi = to_spi_device(dev);
> > > > +	int len;
> > > > +
> > > > +	len = acpi_device_modalias(dev, buf, PAGE_SIZE);
> > > 
> > > but here it is PAGE_SIZE.
> > > 
> > good catch.
> > 
> > > Perhaps it should be PAGE_SIZE in all sites?
> > 
> > dev_attr_show() will give a warning message if modalias_show() returns
> > PAGE_SIZE, thus I'd prefer to use PAGE_SIZE - 1 for all sites.
> 
> So I changed the PAGE_SIZE to PAGE_SIZE - 1 in the last instance and queued
> up the whole series for 3.14 in linux-pm.git/linux-next.  Please have a look
> at that and let me know if there's anything wrong with it.
> 
the change looks okay to me.

thanks,
rui
> Thanks!
> 

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Jarkko Nikula]

On 01/16/2014 09:46 PM, Mark Brown wrote:
> On Thu, Jan 16, 2014 at 09:05:09PM +0800, Zhang Rui wrote:
>> On Thu, 2014-01-16 at 13:27 +0100, Wolfram Sang wrote:
>> This seems a gap to me but I'm not 100% sure.
>> I saw Grant Likely introduced the OF style MODALIAS to platform bus, and
>> OF style registration/binding to i2c bus, maybe he has an answer for
>> this.
> This is needed for ACPI because we rewrite the device names to give us
> stable names.  With OF for I2C and SPI nothing bus specific is done that
> affects this stuff so the default behaviour works.
Sidenote: actually this modalias/module loading issue is different and 
not related to stable ACPI i2c/spi slave device names.

-- 
Jarkko

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On Fri, Jan 17, 2014 at 09:37:56AM +0200, Jarkko Nikula wrote:
> On 01/16/2014 09:46 PM, Mark Brown wrote:

> >This is needed for ACPI because we rewrite the device names to give us
> >stable names.  With OF for I2C and SPI nothing bus specific is done that
> >affects this stuff so the default behaviour works.

> Sidenote: actually this modalias/module loading issue is different
> and not related to stable ACPI i2c/spi slave device names.

Oh, I'd been under the impression that it was the rewrite that was
triggering this?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Jarkko Nikula]

On 01/17/2014 05:57 PM, Mark Brown wrote:
> On Fri, Jan 17, 2014 at 09:37:56AM +0200, Jarkko Nikula wrote:
>> Sidenote: actually this modalias/module loading issue is different
>> and not related to stable ACPI i2c/spi slave device names.
> Oh, I'd been under the impression that it was the rewrite that was
> triggering this?
IIRC issue has been there since when ACPI slave device support was added.

I have a partial fix for it in cf9eb39c6f7b ("spi: Fix modalias for ACPI 
enumerated SPI devices") and when doing similar change for i2c Rui 
pointed me that he has a better fix that takes care of _CID string and 
platform code too.

-- 
Jarkko

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Zhang Rui]

On Tue, 2014-01-14 at 14:41 +0000, Mark Brown wrote:
> On Tue, Jan 14, 2014 at 04:00:17PM +0800, Zhang Rui wrote:
> > On Mon, 2014-01-13 at 17:35 +0000, Mark Brown wrote:
> > > On Mon, Jan 13, 2014 at 09:48:31PM +0800, Zhang Rui wrote:
> > > > ACPI enumerated devices has ACPI style _HID and _CID strings,
> > > > all of these strings can be used for both driver loading and matching.
> 
> > If this piece of code is used in an *SPI* driver for an ACPI enumerated
> > spi device, the spi driver module_alias is "acpi:INTABCD", but
> > the uevent of its spi device node is
> > "spi:INTABCD" (PREFIX:spi_device->modalias).
> 
> OK that makes sense, but what does this have to do with the _HID and
> _CID methods?

If an ACPI enumerated SPI device has a _HID and a _CID,
both of them need to be exposed in 'uevent', so that a driver that
matches _CID can also have a chance to be probed. This can not be done
by the current code, thus we need special handling of ACPI enumerated
SPI devices.

>   Surely we're just replacing spi: with acpi: in the uevent?

yes.

thanks,
rui

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On Tue, Jan 14, 2014 at 04:00:17PM +0800, Zhang Rui wrote:
> On Mon, 2014-01-13 at 17:35 +0000, Mark Brown wrote:
> > On Mon, Jan 13, 2014 at 09:48:31PM +0800, Zhang Rui wrote:
> > > ACPI enumerated devices has ACPI style _HID and _CID strings,
> > > all of these strings can be used for both driver loading and matching.

> If this piece of code is used in an *SPI* driver for an ACPI enumerated
> spi device, the spi driver module_alias is "acpi:INTABCD", but
> the uevent of its spi device node is
> "spi:INTABCD" (PREFIX:spi_device->modalias).

OK that makes sense, but what does this have to do with the _HID and
_CID methods?  Surely we're just replacing spi: with acpi: in the uevent?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Zhang Rui]

On Mon, 2014-01-13 at 17:35 +0000, Mark Brown wrote:
> On Mon, Jan 13, 2014 at 09:48:31PM +0800, Zhang Rui wrote:
> > ACPI enumerated devices has ACPI style _HID and _CID strings,
> > all of these strings can be used for both driver loading and matching.
> 
> > But currently, in Platform, I2C and SPI bus, only the ACPI style
> > driver matching is supported by invoking acpi_driver_match_device()
> > in bus .match() callback.
> 
> I don't understand what this means, sorry.

sorry, I should be clearer about this.

>   As far as I can tell "ACPI
> style _HID and _CID strings" are something different to "ACPI style
> driver matching" but what that actually means is not at all clear to me
> so I don't know what problem this is intended to fix.
> 
I gave a more detailed description about the problem in the changelog of
patch 2/4.

Take the following piece of code for example,
static const struct acpi_device_id xxx_acpi_match[] = {
        { "INTABCD", 0 },
        { }
};
MODULE_DEVICE_TABLE(acpi, xxx_acpi_match);

this can be seen in a platform/I2C/SPI driver that supports an ACPI
enumerated device, right?

If this piece of code is used in a platform driver for
an ACPI enumerated platform device, the platform DRIVER module_alias
is "acpi:INTABCD", but the uevent attribute of its platform device node
is "platform:INTABCD:00" (PREFIX:platform_device->name).
If this piece of code is used in an I2C driver for an ACPI enumerated
i2c device, the i2c driver module_alias is "acpi:INTABCD", but
the uevent of its i2c device node is
"i2c:INTABCD:00" (PREFIX:i2c_client->name).
If this piece of code is used in an *SPI* driver for an ACPI enumerated
spi device, the spi driver module_alias is "acpi:INTABCD", but
the uevent of its spi device node is
"spi:INTABCD" (PREFIX:spi_device->modalias).

thus when the device node is created, the driver will not be loaded
automatically because their modalias do not match.

> Please also always remember to CC maintainers on patches.
> 
okay, will resend the patches later.

thanks,
rui
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 349ebba..ab70eda 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -58,6 +58,11 @@ static ssize_t
> >  modalias_show(struct device *dev, struct device_attribute *a, char *buf)
> >  {
> >  	const struct spi_device	*spi = to_spi_device(dev);
> > +	int len;
> > +
> > +	len = acpi_device_modalias(dev, buf, PAGE_SIZE);
> > +	if (len != -ENODEV)
> > +		return len;
> >  
> >  	return sprintf(buf, "%s%s\n", SPI_MODULE_PREFIX, spi->modalias);
> >  }
> 
> What does this do and why can't acpi_driver_match_device() handle this
> like it does for other ACPI devices?  We don't need to add such code for
> other firmware interfaces...

Re: [PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]

On Mon, Jan 13, 2014 at 09:48:31PM +0800, Zhang Rui wrote:
> ACPI enumerated devices has ACPI style _HID and _CID strings,
> all of these strings can be used for both driver loading and matching.

> But currently, in Platform, I2C and SPI bus, only the ACPI style
> driver matching is supported by invoking acpi_driver_match_device()
> in bus .match() callback.

I don't understand what this means, sorry.  As far as I can tell "ACPI
style _HID and _CID strings" are something different to "ACPI style
driver matching" but what that actually means is not at all clear to me
so I don't know what problem this is intended to fix.

Please also always remember to CC maintainers on patches.

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 349ebba..ab70eda 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -58,6 +58,11 @@ static ssize_t
>  modalias_show(struct device *dev, struct device_attribute *a, char *buf)
>  {
>  	const struct spi_device	*spi = to_spi_device(dev);
> +	int len;
> +
> +	len = acpi_device_modalias(dev, buf, PAGE_SIZE);
> +	if (len != -ENODEV)
> +		return len;
>  
>  	return sprintf(buf, "%s%s\n", SPI_MODULE_PREFIX, spi->modalias);
>  }

What does this do and why can't acpi_driver_match_device() handle this
like it does for other ACPI devices?  We don't need to add such code for
other firmware interfaces...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/4] fix module autoloading for ACPI enumerated devices

[Zhang Rui]

ACPI enumerated devices has ACPI style _HID and _CID strings,
all of these strings can be used for both driver loading and matching.

But currently, in Platform, I2C and SPI bus, only the ACPI style
driver matching is supported by invoking acpi_driver_match_device()
in bus .match() callback.

This patch fixes module autoloading on those buses for ACPI enumerated devices.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/base/platform.c |   12 +++++++++++-
 drivers/i2c/i2c-core.c  |   11 +++++++++++
 drivers/spi/spi.c       |   10 ++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3a94b79..2f4aea2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -677,7 +677,13 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
 			     char *buf)
 {
 	struct platform_device	*pdev = to_platform_device(dev);
-	int len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
+	int len;
+
+	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
+	if (len != -ENODEV)
+		return len;
+
+	len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
 
 	return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
 }
@@ -699,6 +705,10 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env)
 	if (rc != -ENODEV)
 		return rc;
 
+	rc = acpi_device_uevent_modalias(dev, env);
+	if (rc != -ENODEV)
+		return rc;
+
 	add_uevent_var(env, "MODALIAS=%s%s", PLATFORM_MODULE_PREFIX,
 			pdev->name);
 	return 0;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d74c0b3..c4c5588 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -104,6 +104,11 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
 static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct i2c_client	*client = to_i2c_client(dev);
+	int rc;
+
+	rc = acpi_device_uevent_modalias(dev, env);
+	if (rc != -ENODEV)
+		return rc;
 
 	if (add_uevent_var(env, "MODALIAS=%s%s",
 			   I2C_MODULE_PREFIX, client->name))
@@ -409,6 +414,12 @@ static ssize_t
 show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	int len;
+
+	len = acpi_device_modalias(dev, buf, PAGE_SIZE -1);
+	if (len != -ENODEV)
+		return len;
+
 	return sprintf(buf, "%s%s\n", I2C_MODULE_PREFIX, client->name);
 }
 
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 349ebba..ab70eda 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -58,6 +58,11 @@ static ssize_t
 modalias_show(struct device *dev, struct device_attribute *a, char *buf)
 {
 	const struct spi_device	*spi = to_spi_device(dev);
+	int len;
+
+	len = acpi_device_modalias(dev, buf, PAGE_SIZE);
+	if (len != -ENODEV)
+		return len;
 
 	return sprintf(buf, "%s%s\n", SPI_MODULE_PREFIX, spi->modalias);
 }
@@ -114,6 +119,11 @@ static int spi_match_device(struct device *dev, struct device_driver *drv)
 static int spi_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	const struct spi_device		*spi = to_spi_device(dev);
+	int rc;
+
+	rc = acpi_device_uevent_modalias(dev, env);
+	if (rc != -ENODEV)
+		return rc;
 
 	add_uevent_var(env, "MODALIAS=%s%s", SPI_MODULE_PREFIX, spi->modalias);
 	return 0;
-- 
1.7.9.5