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

[Jeffrey Hugo <jhugo@codeaurora.org>]

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.