Re: [PATCH] bus: mhi: core: Indexed MHI controller name

Re: [PATCH] bus: mhi: core: Indexed MHI controller name

[Jeffrey Hugo]

On 11/25/2020 8:43 AM, Loic Poulain wrote:
> Today the MHI controller name is simply cloned from the underlying
> bus device (its parent), that gives the following device structure
> for e.g. a MHI/PCI controller:
> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0
> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0/0000:02:00.0_IPCR
> ...
> 
> That's quite misleading/confusing and can cause device registering
> issues because of duplicate dev name (e.g. if a PCI device register
> two different MHI instances).
> 
> This patch changes MHI core to create indexed mhi controller names
> (mhi0, mhi1...) in the same way as other busses (i2c0, usb0...).
> 
> The previous example becomes:
> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0
> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR
> ...
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>


How does this change /sys/bus/mhi/devices/ ?

The point of having the bus name in the mhi device name is to give an 
easy way to correlate those devices back to the "root" device (I have a 
lot of users which do that).

Also, do we actually have some device that actually exposes multiple MHI 
interfaces?


-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] bus: mhi: core: Indexed MHI controller name

[Loic Poulain]

Today the MHI controller name is simply cloned from the underlying
bus device (its parent), that gives the following device structure
for e.g. a MHI/PCI controller:
devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0
devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0/0000:02:00.0_IPCR
...

That's quite misleading/confusing and can cause device registering
issues because of duplicate dev name (e.g. if a PCI device register
two different MHI instances).

This patch changes MHI core to create indexed mhi controller names
(mhi0, mhi1...) in the same way as other busses (i2c0, usb0...).

The previous example becomes:
devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0
devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR
...

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bus/mhi/core/init.c | 16 +++++++++++++++-
 drivers/bus/mhi/core/main.c |  2 +-
 include/linux/mhi.h         |  2 ++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index c7a7354..ecfffb0 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -8,6 +8,7 @@
 #include <linux/device.h>
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
+#include <linux/idr.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
 #include <linux/mhi.h>
@@ -18,6 +19,8 @@
 #include <linux/wait.h>
 #include "internal.h"
 
+static DEFINE_IDA(mhi_controller_ida);
+
 const char * const mhi_ee_str[MHI_EE_MAX] = {
 	[MHI_EE_PBL] = "PBL",
 	[MHI_EE_SBL] = "SBL",
@@ -940,6 +943,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 	mhi_cntrl->minor_version = (soc_info & SOC_HW_VERSION_MINOR_VER_BMSK) >>
 					SOC_HW_VERSION_MINOR_VER_SHFT;
 
+	mhi_cntrl->index = ida_alloc(&mhi_controller_ida, GFP_KERNEL);
+	if (mhi_cntrl->index < 0) {
+		ret = mhi_cntrl->index;
+		goto error_ida_alloc;
+	}
+
 	/* Register controller with MHI bus */
 	mhi_dev = mhi_alloc_device(mhi_cntrl);
 	if (IS_ERR(mhi_dev)) {
@@ -950,7 +959,7 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 
 	mhi_dev->dev_type = MHI_DEVICE_CONTROLLER;
 	mhi_dev->mhi_cntrl = mhi_cntrl;
-	dev_set_name(&mhi_dev->dev, "%s", dev_name(mhi_cntrl->cntrl_dev));
+	dev_set_name(&mhi_dev->dev, "mhi%d", mhi_cntrl->index);
 	mhi_dev->name = dev_name(mhi_cntrl->cntrl_dev);
 
 	/* Init wakeup source */
@@ -970,6 +979,9 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 	put_device(&mhi_dev->dev);
 
 error_alloc_dev:
+	ida_free(&mhi_controller_ida, mhi_cntrl->index);
+
+error_ida_alloc:
 	kfree(mhi_cntrl->mhi_cmd);
 
 error_alloc_cmd:
@@ -1004,6 +1016,8 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
 
 	device_del(&mhi_dev->dev);
 	put_device(&mhi_dev->dev);
+
+	ida_free(&mhi_controller_ida, mhi_cntrl->index);
 }
 EXPORT_SYMBOL_GPL(mhi_unregister_controller);
 
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 188501c0..4818f42 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -349,7 +349,7 @@ void mhi_create_devices(struct mhi_controller *mhi_cntrl)
 		/* Channel name is same for both UL and DL */
 		mhi_dev->name = mhi_chan->name;
 		dev_set_name(&mhi_dev->dev, "%s_%s",
-			     dev_name(mhi_cntrl->cntrl_dev),
+			     dev_name(&mhi_cntrl->mhi_dev->dev),
 			     mhi_dev->name);
 
 		/* Init wakeup source if available */
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 27078db..2a89533 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -297,6 +297,7 @@ struct mhi_controller_config {
  * @cntrl_dev: Pointer to the struct device of physical bus acting as the MHI
  *            controller (required)
  * @mhi_dev: MHI device instance for the controller
+ * @index: Index of the MHI controller instance
  * @debugfs_dentry: MHI controller debugfs directory
  * @regs: Base address of MHI MMIO register space (required)
  * @bhi: Points to base of MHI BHI register space
@@ -377,6 +378,7 @@ struct mhi_controller_config {
 struct mhi_controller {
 	struct device *cntrl_dev;
 	struct mhi_device *mhi_dev;
+	int index;
 	struct dentry *debugfs_dentry;
 	void __iomem *regs;
 	void __iomem *bhi;
-- 
2.7.4

Re: [PATCH] bus: mhi: core: Indexed MHI controller name

[Jeffrey Hugo]

On 11/25/2020 8:42 AM, Jeffrey Hugo wrote:
> On 11/25/2020 8:43 AM, Loic Poulain wrote:
>> Today the MHI controller name is simply cloned from the underlying
>> bus device (its parent), that gives the following device structure
>> for e.g. a MHI/PCI controller:
>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0
>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0/0000:02:00.0_IPCR 
>>
>> ...
>>
>> That's quite misleading/confusing and can cause device registering
>> issues because of duplicate dev name (e.g. if a PCI device register
>> two different MHI instances).
>>
>> This patch changes MHI core to create indexed mhi controller names
>> (mhi0, mhi1...) in the same way as other busses (i2c0, usb0...).
>>
>> The previous example becomes:
>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0
>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR
>> ...
>>
>> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> 
> 
> How does this change /sys/bus/mhi/devices/ ?
> 
> The point of having the bus name in the mhi device name is to give an 
> easy way to correlate those devices back to the "root" device (I have a 
> lot of users which do that).
> 
> Also, do we actually have some device that actually exposes multiple MHI 
> interfaces?
> 
> 

Looking at the code change itself, looks like the controller index is 
essentially random, and not persistent.  Is that expected?

I'm thinking it might be confusing if you have say 12 MHI controllers 
from 12 different devices, and some of those devices crash at roughly 
the same time.  The controllers get removed, and re-initialized, which 
means that you have essentially a race condition where the controller 
for the same device now has a different index.

-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] bus: mhi: core: Indexed MHI controller name

[Loic Poulain]

On Wed, 25 Nov 2020 at 16:42, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>
> On 11/25/2020 8:43 AM, Loic Poulain wrote:
> > Today the MHI controller name is simply cloned from the underlying
> > bus device (its parent), that gives the following device structure
> > for e.g. a MHI/PCI controller:
> > devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0
> > devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0/0000:02:00.0_IPCR
> > ...
> >
> > That's quite misleading/confusing and can cause device registering
> > issues because of duplicate dev name (e.g. if a PCI device register
> > two different MHI instances).
> >
> > This patch changes MHI core to create indexed mhi controller names
> > (mhi0, mhi1...) in the same way as other busses (i2c0, usb0...).
> >
> > The previous example becomes:
> > devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0
> > devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR
> > ...
> >
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>
>
> How does this change /sys/bus/mhi/devices/ ?

That does change sysfs device dir names:
$ ls -al /sys/bus/mhi/devices/
lrwxrwxrwx 1 root root 0 nov.  25 16:27 mhi0 ->
../../../devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0
lrwxrwxrwx 1 root root 0 nov.  25 16:28 mhi0_DIAG ->
../../../devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_DIAG
lrwxrwxrwx 1 root root 0 nov.  25 16:28 mhi0_IPCR ->
../../../devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR

> The point of having the bus name in the mhi device name is to give an
> easy way to correlate those devices back to the "root" device (I have a
> lot of users which do that).

I see your point but it's not a problem specific to MHI bus, user can
rely on sysfs/uevent to get device information such ass devices
attributes, children, or parent devices. Do you have a concrete
example in mind for your case?

>
> Also, do we actually have some device that actually exposes multiple MHI
> interfaces?

No, but better to fix possible problems ahead of time.

Regards,
Loic

Re: [PATCH] bus: mhi: core: Indexed MHI controller name

[Loic Poulain]

On Wed, 25 Nov 2020 at 16:49, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>
> On 11/25/2020 8:42 AM, Jeffrey Hugo wrote:
> > On 11/25/2020 8:43 AM, Loic Poulain wrote:
> >> Today the MHI controller name is simply cloned from the underlying
> >> bus device (its parent), that gives the following device structure
> >> for e.g. a MHI/PCI controller:
> >> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0
> >> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0/0000:02:00.0_IPCR
> >>
> >> ...
> >>
> >> That's quite misleading/confusing and can cause device registering
> >> issues because of duplicate dev name (e.g. if a PCI device register
> >> two different MHI instances).
> >>
> >> This patch changes MHI core to create indexed mhi controller names
> >> (mhi0, mhi1...) in the same way as other busses (i2c0, usb0...).
> >>
> >> The previous example becomes:
> >> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0
> >> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR
> >> ...
> >>
> >> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> >
> >
> > How does this change /sys/bus/mhi/devices/ ?
> >
> > The point of having the bus name in the mhi device name is to give an
> > easy way to correlate those devices back to the "root" device (I have a
> > lot of users which do that).
> >
> > Also, do we actually have some device that actually exposes multiple MHI
> > interfaces?
> >
> >
>
> Looking at the code change itself, looks like the controller index is
> essentially random, and not persistent.  Is that expected?
>
> I'm thinking it might be confusing if you have say 12 MHI controllers
> from 12 different devices, and some of those devices crash at roughly
> the same time.  The controllers get removed, and re-initialized, which
> means that you have essentially a race condition where the controller
> for the same device now has a different index.

There is no race since ID is atomically allocated, but yes your
controller can get a different index on unregister/register, like e.g
network interfaces index, usb devices ID, video devices, etc... can be
enumerated in various order. Not sure why the user should take care of
the MHI controller index.

>
> --
> Jeffrey Hugo
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] bus: mhi: core: Indexed MHI controller name

[Jeffrey Hugo]

On 11/25/2020 9:15 AM, Loic Poulain wrote:
> On Wed, 25 Nov 2020 at 16:42, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>>
>> On 11/25/2020 8:43 AM, Loic Poulain wrote:
>>> Today the MHI controller name is simply cloned from the underlying
>>> bus device (its parent), that gives the following device structure
>>> for e.g. a MHI/PCI controller:
>>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0
>>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0/0000:02:00.0_IPCR
>>> ...
>>>
>>> That's quite misleading/confusing and can cause device registering
>>> issues because of duplicate dev name (e.g. if a PCI device register
>>> two different MHI instances).
>>>
>>> This patch changes MHI core to create indexed mhi controller names
>>> (mhi0, mhi1...) in the same way as other busses (i2c0, usb0...).
>>>
>>> The previous example becomes:
>>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0
>>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR
>>> ...
>>>
>>> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>>
>>
>> How does this change /sys/bus/mhi/devices/ ?
> 
> That does change sysfs device dir names:
> $ ls -al /sys/bus/mhi/devices/
> lrwxrwxrwx 1 root root 0 nov.  25 16:27 mhi0 ->
> ../../../devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0
> lrwxrwxrwx 1 root root 0 nov.  25 16:28 mhi0_DIAG ->
> ../../../devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_DIAG
> lrwxrwxrwx 1 root root 0 nov.  25 16:28 mhi0_IPCR ->
> ../../../devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR
> 
>> The point of having the bus name in the mhi device name is to give an
>> easy way to correlate those devices back to the "root" device (I have a
>> lot of users which do that).
> 
> I see your point but it's not a problem specific to MHI bus, user can
> rely on sysfs/uevent to get device information such ass devices
> attributes, children, or parent devices. Do you have a concrete
> example in mind for your case?

3 I think -

1. I think its same to assume these names are going to trickle down to 
mhi_uci.  For a QAIC device, there is a /dev/qaic chardev for doing the 
AI work, and a /dev chardev from mhi_uci exposing the DIAG channel. 
Both of these are correlated to a specific PCI device by the BDF, which 
the user application will want to know so that relevant DIAG traffic for 
a qaic device is routed to the correct chardev, and thus the correct 
device.  For 1 device, this is simple.  For 12, some correlation needs 
to be done programmatically.

2. User is physically on the shell, and wants to look at 
/sys/bus/mhi/devices to see the state of a device.  Perhaps the expected 
mhi_uci devices are not present.

3. User is physically on the shell, and wants to invoke soc_reset on the 
controller.  The user needs to know which mhi controller instance 
(mhi1?) maps to a specific PCI BDF to make sure the right device is reset.

1 is used a lot by our userspace stack.  2/3 is used a lot by testers 
and developers.

> 
>>
>> Also, do we actually have some device that actually exposes multiple MHI
>> interfaces?
> 
> No, but better to fix possible problems ahead of time.

Good to know.  I was thinking this was not needed, and unlikely to be 
needed, but it just occurs to me I might have a usecase for this in 
development.

-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] bus: mhi: core: Indexed MHI controller name

[Jeffrey Hugo]

On 11/25/2020 9:23 AM, Loic Poulain wrote:
> On Wed, 25 Nov 2020 at 16:49, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>>
>> On 11/25/2020 8:42 AM, Jeffrey Hugo wrote:
>>> On 11/25/2020 8:43 AM, Loic Poulain wrote:
>>>> Today the MHI controller name is simply cloned from the underlying
>>>> bus device (its parent), that gives the following device structure
>>>> for e.g. a MHI/PCI controller:
>>>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0
>>>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0/0000:02:00.0_IPCR
>>>>
>>>> ...
>>>>
>>>> That's quite misleading/confusing and can cause device registering
>>>> issues because of duplicate dev name (e.g. if a PCI device register
>>>> two different MHI instances).
>>>>
>>>> This patch changes MHI core to create indexed mhi controller names
>>>> (mhi0, mhi1...) in the same way as other busses (i2c0, usb0...).
>>>>
>>>> The previous example becomes:
>>>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0
>>>> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR
>>>> ...
>>>>
>>>> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>>>
>>>
>>> How does this change /sys/bus/mhi/devices/ ?
>>>
>>> The point of having the bus name in the mhi device name is to give an
>>> easy way to correlate those devices back to the "root" device (I have a
>>> lot of users which do that).
>>>
>>> Also, do we actually have some device that actually exposes multiple MHI
>>> interfaces?
>>>
>>>
>>
>> Looking at the code change itself, looks like the controller index is
>> essentially random, and not persistent.  Is that expected?
>>
>> I'm thinking it might be confusing if you have say 12 MHI controllers
>> from 12 different devices, and some of those devices crash at roughly
>> the same time.  The controllers get removed, and re-initialized, which
>> means that you have essentially a race condition where the controller
>> for the same device now has a different index.
> 
> There is no race since ID is atomically allocated, but yes your
> controller can get a different index on unregister/register, like e.g
> network interfaces index, usb devices ID, video devices, etc... can be
> enumerated in various order. Not sure why the user should take care of
> the MHI controller index.

You said there is no race, but then literally described a race.

It appears we agree, the enumeration first time around may not be the 
same enumeration order later on.

The point is not that the user cares that they always talk to mhi0.  The 
point is that the user was talking to mhi0, that crashed, re-enumerated, 
and is now mhi3.

-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] bus: mhi: core: Indexed MHI controller name

[Jeffrey Hugo]

On 11/25/2020 8:43 AM, Loic Poulain wrote:
> Today the MHI controller name is simply cloned from the underlying
> bus device (its parent), that gives the following device structure
> for e.g. a MHI/PCI controller:
> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0
> devices/pci0000:00/0000:00:01.2/0000:02:00.0/0000:02:00.0/0000:02:00.0_IPCR
> ...
> 
> That's quite misleading/confusing and can cause device registering
> issues because of duplicate dev name (e.g. if a PCI device register
> two different MHI instances).
> 
> This patch changes MHI core to create indexed mhi controller names
> (mhi0, mhi1...) in the same way as other busses (i2c0, usb0...).
> 
> The previous example becomes:
> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0
> devices/pci0000:00/0000:00:01.2/0000:02:00.0/mhi0/mhi0_IPCR
> ...
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

Based on various discussions, I have no objections.  I think now is a 
good time to sort out this ABI.  However I do have one nit below.

> ---
>   drivers/bus/mhi/core/init.c | 16 +++++++++++++++-
>   drivers/bus/mhi/core/main.c |  2 +-
>   include/linux/mhi.h         |  2 ++
>   3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index c7a7354..ecfffb0 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -8,6 +8,7 @@
>   #include <linux/device.h>
>   #include <linux/dma-direction.h>
>   #include <linux/dma-mapping.h>
> +#include <linux/idr.h>
>   #include <linux/interrupt.h>
>   #include <linux/list.h>
>   #include <linux/mhi.h>
> @@ -18,6 +19,8 @@
>   #include <linux/wait.h>
>   #include "internal.h"
>   
> +static DEFINE_IDA(mhi_controller_ida);
> +
>   const char * const mhi_ee_str[MHI_EE_MAX] = {
>   	[MHI_EE_PBL] = "PBL",
>   	[MHI_EE_SBL] = "SBL",
> @@ -940,6 +943,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>   	mhi_cntrl->minor_version = (soc_info & SOC_HW_VERSION_MINOR_VER_BMSK) >>
>   					SOC_HW_VERSION_MINOR_VER_SHFT;
>   
> +	mhi_cntrl->index = ida_alloc(&mhi_controller_ida, GFP_KERNEL);
> +	if (mhi_cntrl->index < 0) {
> +		ret = mhi_cntrl->index;
> +		goto error_ida_alloc;
> +	}
> +
>   	/* Register controller with MHI bus */
>   	mhi_dev = mhi_alloc_device(mhi_cntrl);
>   	if (IS_ERR(mhi_dev)) {
> @@ -950,7 +959,7 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>   
>   	mhi_dev->dev_type = MHI_DEVICE_CONTROLLER;
>   	mhi_dev->mhi_cntrl = mhi_cntrl;
> -	dev_set_name(&mhi_dev->dev, "%s", dev_name(mhi_cntrl->cntrl_dev));
> +	dev_set_name(&mhi_dev->dev, "mhi%d", mhi_cntrl->index);
>   	mhi_dev->name = dev_name(mhi_cntrl->cntrl_dev);
>   
>   	/* Init wakeup source */
> @@ -970,6 +979,9 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>   	put_device(&mhi_dev->dev);
>   
>   error_alloc_dev:
> +	ida_free(&mhi_controller_ida, mhi_cntrl->index);
> +
> +error_ida_alloc:
>   	kfree(mhi_cntrl->mhi_cmd);
>   
>   error_alloc_cmd:
> @@ -1004,6 +1016,8 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
>   
>   	device_del(&mhi_dev->dev);
>   	put_device(&mhi_dev->dev);
> +
> +	ida_free(&mhi_controller_ida, mhi_cntrl->index);
>   }
>   EXPORT_SYMBOL_GPL(mhi_unregister_controller);
>   
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 188501c0..4818f42 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -349,7 +349,7 @@ void mhi_create_devices(struct mhi_controller *mhi_cntrl)
>   		/* Channel name is same for both UL and DL */
>   		mhi_dev->name = mhi_chan->name;
>   		dev_set_name(&mhi_dev->dev, "%s_%s",
> -			     dev_name(mhi_cntrl->cntrl_dev),
> +			     dev_name(&mhi_cntrl->mhi_dev->dev),
>   			     mhi_dev->name);
>   
>   		/* Init wakeup source if available */
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 27078db..2a89533 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -297,6 +297,7 @@ struct mhi_controller_config {
>    * @cntrl_dev: Pointer to the struct device of physical bus acting as the MHI
>    *            controller (required)
>    * @mhi_dev: MHI device instance for the controller
> + * @index: Index of the MHI controller instance
>    * @debugfs_dentry: MHI controller debugfs directory
>    * @regs: Base address of MHI MMIO register space (required)
>    * @bhi: Points to base of MHI BHI register space
> @@ -377,6 +378,7 @@ struct mhi_controller_config {
>   struct mhi_controller {
>   	struct device *cntrl_dev;
>   	struct mhi_device *mhi_dev;
> +	int index;
>   	struct dentry *debugfs_dentry;
>   	void __iomem *regs;
>   	void __iomem *bhi;
> 

Is there a good reason to have this in the middle of the struct?  Seems 
like it would cause some padding because its a 32-bit value in the 
middle of a series of 64-bit values.

-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.