[PATCH 0/6] mfd: fix platform-device id collisions

Hot-pluggable multi-function devices should always be registered with
PLATFORM_DEVID_AUTO to avoid name collisions on the platform bus.

This series fix the two mfd drivers that currently fail to get this
right, and also adds a new helper function to assist any future driver
authors.

Included is also a fix of how mfd core generates the platform ids for
subdevices. Currently, the mfd cell-id is simply added to the id base
that mfd_add_devices is called with. This effectively prevents
mfd-devices from using PLATFORM_DEVID_AUTO (-2) while still having
non-zero cell ids.

In a different thread I mentioned that using for example

	bus_num << 8 | dev_num

as an id-base for USB multi-function devices would also avoid any
collisions, but encoding the bus topology in the id base like this
is not good idea. [1] Not only would it force any new transports to come
up with unique id bases, it would also be very ad-hoc differ from driver
to driver (consider multi-interface USB devices or non-zero cell ids).

Note that if userspace needs to find sibling interfaces it should
never rely on device naming anyway, but rather use the topology already
encoded in sysfs.

The only thing that is currently not possible trough sysfs is to figure
out which sibling interface is which should they have the same name but
unique cell ids (consider an MFD with multiple leds or gpio chips).
Again, parsing device ids is not an option, but if needed we could
simply let driver core export any mfd cell-id for platform devices. Note
that the final patch is a pre-requisite for this.

Johan

[1] http://marc.info/?l=linux-kernel&m=141094514827834&w=2


Johan Hovold (6):
  mfd: viperboard: fix platform-device id collision
  mfd: rtsx_usb: fix platform device-id collision
  mfd: core: add helper function to register hotplug devices
  mfd: use mfd_add_hotplug_devices helper
  HID: hid-sensor-hub: use mfd_add_hotplug_devices helper
  mfd: core: fix platform-device id generation

 drivers/hid/hid-sensor-hub.c | 8 +++-----
 drivers/mfd/mfd-core.c       | 8 +++++++-
 drivers/mfd/rtsx_usb.c       | 4 ++--
 drivers/mfd/viperboard.c     | 4 ++--
 include/linux/mfd/core.h     | 7 +++++++
 5 files changed, 21 insertions(+), 10 deletions(-)

-- 
1.8.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Make sure to always honour multi-function devices registered with
PLATFORM_DEVID_NONE (-1) or PLATFORM_DEVID_AUTO (-2) as id base. In this
case it does not make sense to append the cell id to the mfd-id base and
potentially change the requested behaviour.

Specifically this will allow multi-function devices to be registered
with PLATFORM_DEVID_AUTO while still having non-zero cell ids.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/mfd/mfd-core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 892d343193ad..79f25633d7db 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c

@@ -87,9 +87,15 @@ static int mfd_add_device(struct device *parent, int id,
 	struct platform_device *pdev;
 	struct device_node *np = NULL;
 	int ret = -ENOMEM;
+	int platform_id;
 	int r;
 
-	pdev = platform_device_alloc(cell->name, id + cell->id);
+	if (id < 0)
+		platform_id = id;
+	else
+		platform_id = id + cell->id;
+
+	pdev = platform_device_alloc(cell->name, platform_id);
 	if (!pdev)
 		goto fail_alloc;
 

-- 
1.8.5.5

Use mfd_add_hotplug_devices helper to register the subdevices.

Compile-only tested.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/hid/hid-sensor-hub.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 2ac25760a9a9..fbe1a6d2aa53 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c

@@ -641,9 +641,6 @@ static int sensor_hub_probe(struct hid_device *hdev,
 					goto err_stop_hw;
 			}
 			sd->hid_sensor_hub_client_devs[
-				sd->hid_sensor_client_cnt].id =
-							PLATFORM_DEVID_AUTO;
-			sd->hid_sensor_hub_client_devs[
 				sd->hid_sensor_client_cnt].name = name;
 			sd->hid_sensor_hub_client_devs[
 				sd->hid_sensor_client_cnt].platform_data =

@@ -659,8 +656,9 @@ static int sensor_hub_probe(struct hid_device *hdev,
 	if (last_hsdev)
 		last_hsdev->end_collection_index = i;
 
-	ret = mfd_add_devices(&hdev->dev, 0, sd->hid_sensor_hub_client_devs,
-		sd->hid_sensor_client_cnt, NULL, 0, NULL);
+	ret = mfd_add_hotplug_devices(&hdev->dev,
+			sd->hid_sensor_hub_client_devs,
+			sd->hid_sensor_client_cnt);
 	if (ret < 0)
 		goto err_stop_hw;
 

-- 
1.8.5.5

Hot-pluggable multi-function devices should use PLATFORM_DEVID_AUTO to
avoid name collisions on the platform bus.

This driver currently uses the USB-device address as an id. This makes
name collisions unlikely, but it could still happen if two devices are
connected to separate buses and gets assigned the same address.

Signed-off-by: Johan Hovold <redacted>
---
 drivers/mfd/rtsx_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/rtsx_usb.c b/drivers/mfd/rtsx_usb.c
index 71f387ce8cbd..78073e4b87e4 100644
--- a/drivers/mfd/rtsx_usb.c
+++ b/drivers/mfd/rtsx_usb.c

@@ -647,7 +647,7 @@ static int rtsx_usb_probe(struct usb_interface *intf,
 	/* initialize USB SG transfer timer */
 	setup_timer(&ucr->sg_timer, rtsx_usb_sg_timed_out, (unsigned long) ucr);
 
-	ret = mfd_add_devices(&intf->dev, usb_dev->devnum, rtsx_usb_cells,
+	ret = mfd_add_devices(&intf->dev, PLATFORM_DEVID_AUTO, rtsx_usb_cells,
 			ARRAY_SIZE(rtsx_usb_cells), NULL, 0, NULL);
 	if (ret)
 		goto out_init_fail;

-- 
1.8.5.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Allow more than one viperboard to be connected by registering with
PLATFORM_DEVID_AUTO instead of PLATFORM_DEVID_NONE.

The subdevices are currently registered with PLATFORM_DEVID_NONE, which
will cause a name collision on the platform bus when a second viperboard
is plugged in:

viperboard 1-2.4:1.0: version 0.00 found at bus 001 address 004
------------[ cut here ]------------
WARNING: CPU: 0 PID: 181 at /home/johan/work/omicron/src/linux/fs/sysfs/dir.c:31 sysfs_warn_dup+0x74/0x84()
sysfs: cannot create duplicate filename '/bus/platform/devices/viperboard-gpio'
Modules linked in: i2c_viperboard viperboard netconsole [last unloaded: viperboard]
CPU: 0 PID: 181 Comm: bash Tainted: G        W      3.17.0-rc6 #1
[<c0016bf4>] (unwind_backtrace) from [<c0013860>] (show_stack+0x20/0x24)
[<c0013860>] (show_stack) from [<c04305f8>] (dump_stack+0x24/0x28)
[<c04305f8>] (dump_stack) from [<c0040fb4>] (warn_slowpath_common+0x80/0x98)
[<c0040fb4>] (warn_slowpath_common) from [<c004100c>] (warn_slowpath_fmt+0x40/0x48)
[<c004100c>] (warn_slowpath_fmt) from [<c016f1bc>] (sysfs_warn_dup+0x74/0x84)
[<c016f1bc>] (sysfs_warn_dup) from [<c016f548>] (sysfs_do_create_link_sd.isra.2+0xcc/0xd0)
[<c016f548>] (sysfs_do_create_link_sd.isra.2) from [<c016f588>] (sysfs_create_link+0x3c/0x48)
[<c016f588>] (sysfs_create_link) from [<c02867ec>] (bus_add_device+0x12c/0x1e0)
[<c02867ec>] (bus_add_device) from [<c0284820>] (device_add+0x410/0x584)
[<c0284820>] (device_add) from [<c0289440>] (platform_device_add+0xd8/0x26c)
[<c0289440>] (platform_device_add) from [<c02a5ae4>] (mfd_add_device+0x240/0x344)
[<c02a5ae4>] (mfd_add_device) from [<c02a5ce0>] (mfd_add_devices+0xb8/0x110)
[<c02a5ce0>] (mfd_add_devices) from [<bf00d1c8>] (vprbrd_probe+0x160/0x1b0 [viperboard])
[<bf00d1c8>] (vprbrd_probe [viperboard]) from [<c030c000>] (usb_probe_interface+0x1bc/0x2a8)
[<c030c000>] (usb_probe_interface) from [<c028768c>] (driver_probe_device+0x14c/0x3ac)
[<c028768c>] (driver_probe_device) from [<c02879e4>] (__driver_attach+0xa4/0xa8)
[<c02879e4>] (__driver_attach) from [<c0285698>] (bus_for_each_dev+0x70/0xa4)
[<c0285698>] (bus_for_each_dev) from [<c0287030>] (driver_attach+0x2c/0x30)
[<c0287030>] (driver_attach) from [<c030a288>] (usb_store_new_id+0x170/0x1ac)
[<c030a288>] (usb_store_new_id) from [<c030a2f8>] (new_id_store+0x34/0x3c)
[<c030a2f8>] (new_id_store) from [<c02853ec>] (drv_attr_store+0x30/0x3c)
[<c02853ec>] (drv_attr_store) from [<c016eaa8>] (sysfs_kf_write+0x5c/0x60)
[<c016eaa8>] (sysfs_kf_write) from [<c016dc68>] (kernfs_fop_write+0xd4/0x194)
[<c016dc68>] (kernfs_fop_write) from [<c010fe40>] (vfs_write+0xb4/0x1c0)
[<c010fe40>] (vfs_write) from [<c01104a8>] (SyS_write+0x4c/0xa0)
[<c01104a8>] (SyS_write) from [<c000f900>] (ret_fast_syscall+0x0/0x48)
---[ end trace 98e8603c22d65817 ]---
viperboard 1-2.4:1.0: Failed to add mfd devices to core.
viperboard: probe of 1-2.4:1.0 failed with error -17

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/mfd/viperboard.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/viperboard.c b/drivers/mfd/viperboard.c
index e00f5340ed87..3c2b8f9e3c84 100644
--- a/drivers/mfd/viperboard.c
+++ b/drivers/mfd/viperboard.c

@@ -93,8 +93,9 @@ static int vprbrd_probe(struct usb_interface *interface,
 		 version >> 8, version & 0xff,
 		 vb->usb_dev->bus->busnum, vb->usb_dev->devnum);
 
-	ret = mfd_add_devices(&interface->dev, -1, vprbrd_devs,
-				ARRAY_SIZE(vprbrd_devs), NULL, 0, NULL);
+	ret = mfd_add_devices(&interface->dev, PLATFORM_DEVID_AUTO,
+				vprbrd_devs, ARRAY_SIZE(vprbrd_devs), NULL, 0,
+				NULL);
 	if (ret != 0) {
 		dev_err(&interface->dev, "Failed to add mfd devices to core.");
 		goto error;

-- 
1.8.5.5

Hot-pluggable multi-function devices should always be registered with
PLATFORM_DEVID_AUTO to avoid name collisions on the platform bus. This
helper also hides the memory map and irq parameters, which aren't used
by hot-pluggable (e.g. USB-based) devices.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 include/linux/mfd/core.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index f543de91ce19..1e47262a1c63 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h

@@ -108,6 +108,13 @@ extern int mfd_add_devices(struct device *parent, int id,
 			   struct resource *mem_base,
 			   int irq_base, struct irq_domain *irq_domain);
 
+static inline int mfd_add_hotplug_devices(struct device *parent,
+		const struct mfd_cell *cells, int n_devs)
+{
+	return mfd_add_devices(parent, PLATFORM_DEVID_AUTO, cells, n_devs,
+			NULL, 0, NULL);
+}
+
 extern void mfd_remove_devices(struct device *parent);
 
 #endif

-- 
1.8.5.5

Use mfd_add_hotplug_devices helper to register the subdevices.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/mfd/rtsx_usb.c   | 4 ++--
 drivers/mfd/viperboard.c | 5 ++---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/rtsx_usb.c b/drivers/mfd/rtsx_usb.c
index 78073e4b87e4..5b17339fce7c 100644
--- a/drivers/mfd/rtsx_usb.c
+++ b/drivers/mfd/rtsx_usb.c

@@ -647,8 +647,8 @@ static int rtsx_usb_probe(struct usb_interface *intf,
 	/* initialize USB SG transfer timer */
 	setup_timer(&ucr->sg_timer, rtsx_usb_sg_timed_out, (unsigned long) ucr);
 
-	ret = mfd_add_devices(&intf->dev, PLATFORM_DEVID_AUTO, rtsx_usb_cells,
-			ARRAY_SIZE(rtsx_usb_cells), NULL, 0, NULL);
+	ret = mfd_add_hotplug_devices(&intf->dev, rtsx_usb_cells,
+			ARRAY_SIZE(rtsx_usb_cells));
 	if (ret)
 		goto out_init_fail;
 

diff --git a/drivers/mfd/viperboard.c b/drivers/mfd/viperboard.c
index 3c2b8f9e3c84..be450ebcb52d 100644
--- a/drivers/mfd/viperboard.c
+++ b/drivers/mfd/viperboard.c

@@ -93,9 +93,8 @@ static int vprbrd_probe(struct usb_interface *interface,
 		 version >> 8, version & 0xff,
 		 vb->usb_dev->bus->busnum, vb->usb_dev->devnum);
 
-	ret = mfd_add_devices(&interface->dev, PLATFORM_DEVID_AUTO,
-				vprbrd_devs, ARRAY_SIZE(vprbrd_devs), NULL, 0,
-				NULL);
+	ret = mfd_add_hotplug_devices(&interface->dev, vprbrd_devs,
+			ARRAY_SIZE(vprbrd_devs));
 	if (ret != 0) {
 		dev_err(&interface->dev, "Failed to add mfd devices to core.");
 		goto error;

-- 
1.8.5.5

On Fri, 26 Sep 2014, Johan Hovold wrote:

Use mfd_add_hotplug_devices helper to register the subdevices.

Compile-only tested.

Signed-off-by: Johan Hovold <johan@kernel.org>

As I expect this to be going through MFD tree together with the rest of 
the patchset

	Acked-by: Jiri Kosina [off-list ref]

-- 
Jiri Kosina
SUSE Labs

On Fri, 26 Sep 2014, Johan Hovold wrote:

Allow more than one viperboard to be connected by registering with
PLATFORM_DEVID_AUTO instead of PLATFORM_DEVID_NONE.

The subdevices are currently registered with PLATFORM_DEVID_NONE, which
will cause a name collision on the platform bus when a second viperboard
is plugged in:

Applied to fixes.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, 26 Sep 2014, Johan Hovold wrote:

Hot-pluggable multi-function devices should use PLATFORM_DEVID_AUTO to
avoid name collisions on the platform bus.

This driver currently uses the USB-device address as an id. This makes
name collisions unlikely, but it could still happen if two devices are
connected to separate buses and gets assigned the same address.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/mfd/rtsx_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This patch is superfluous.

Just wait until the hotpluggable version is applied, then use it.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, 26 Sep 2014, Johan Hovold wrote:

Hot-pluggable multi-function devices should always be registered with
PLATFORM_DEVID_AUTO to avoid name collisions on the platform bus. This
helper also hides the memory map and irq parameters, which aren't used
by hot-pluggable (e.g. USB-based) devices.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 include/linux/mfd/core.h | 7 +++++++
 1 file changed, 7 insertions(+)

Applied for v3.19.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

On Fri, 26 Sep 2014, Johan Hovold wrote:

Use mfd_add_hotplug_devices helper to register the subdevices.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/mfd/rtsx_usb.c   | 4 ++--
 drivers/mfd/viperboard.c | 5 ++---
 2 files changed, 4 insertions(+), 5 deletions(-)

Applied for v3.19.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, 26 Sep 2014, Johan Hovold wrote:

Use mfd_add_hotplug_devices helper to register the subdevices.

Compile-only tested.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/hid/hid-sensor-hub.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Applied for v3.19 with Jiri's Ack.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, 26 Sep 2014, Johan Hovold wrote:

Make sure to always honour multi-function devices registered with
PLATFORM_DEVID_NONE (-1) or PLATFORM_DEVID_AUTO (-2) as id base. In this
case it does not make sense to append the cell id to the mfd-id base and
potentially change the requested behaviour.

Specifically this will allow multi-function devices to be registered
with PLATFORM_DEVID_AUTO while still having non-zero cell ids.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/mfd/mfd-core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Applied for v3.19.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Oct 07, 2014 at 10:22:58AM +0100, Lee Jones wrote:

On Fri, 26 Sep 2014, Johan Hovold wrote:

quoted

Hot-pluggable multi-function devices should use PLATFORM_DEVID_AUTO to
avoid name collisions on the platform bus.

This driver currently uses the USB-device address as an id. This makes
name collisions unlikely, but it could still happen if two devices are
connected to separate buses and gets assigned the same address.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/mfd/rtsx_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This patch is superfluous.

Well, it would have documented the fact that there could be probe
failures due to id collisions with current and older kernels.

Just wait until the hotpluggable version is applied, then use it.

Fine with me. The collisions are quite unlikely anyway.

Thanks,
Johan

quoted

diff --git a/drivers/mfd/rtsx_usb.c b/drivers/mfd/rtsx_usb.c
index 71f387ce8cbd..78073e4b87e4 100644
--- a/drivers/mfd/rtsx_usb.c
+++ b/drivers/mfd/rtsx_usb.c

@@ -647,7 +647,7 @@ static int rtsx_usb_probe(struct usb_interface *intf,
 	/* initialize USB SG transfer timer */
 	setup_timer(&ucr->sg_timer, rtsx_usb_sg_timed_out, (unsigned long) ucr);
 
-	ret = mfd_add_devices(&intf->dev, usb_dev->devnum, rtsx_usb_cells,
+	ret = mfd_add_devices(&intf->dev, PLATFORM_DEVID_AUTO, rtsx_usb_cells,
 			ARRAY_SIZE(rtsx_usb_cells), NULL, 0, NULL);
 	if (ret)
 		goto out_init_fail;