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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Fri, Nov 27, 2020 at 05:05:21PM +0100, 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>
> ---
>  v2: move index field at the end of mhi_controller struct (before bool)
>      to avoid breaking well packed alignment.
>  v3: Align debugfs directory name
> 
>  drivers/bus/mhi/core/debugfs.c |  2 +-
>  drivers/bus/mhi/core/init.c    | 18 ++++++++++++++++--
>  drivers/bus/mhi/core/main.c    |  2 +-
>  include/linux/mhi.h            |  2 ++
>  4 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/debugfs.c b/drivers/bus/mhi/core/debugfs.c
> index 7d43138..858d751 100644
> --- a/drivers/bus/mhi/core/debugfs.c
> +++ b/drivers/bus/mhi/core/debugfs.c
> @@ -377,7 +377,7 @@ static struct dentry *mhi_debugfs_root;
>  void mhi_create_debugfs(struct mhi_controller *mhi_cntrl)
>  {
>  	mhi_cntrl->debugfs_dentry =
> -			debugfs_create_dir(dev_name(mhi_cntrl->cntrl_dev),
> +			debugfs_create_dir(dev_name(&mhi_cntrl->mhi_dev->dev),
>  					   mhi_debugfs_root);

This change doesn't belong to this patch, right? This should be part of
"bus: mhi: core: Fix device hierarchy".

Thanks,
Mani

>  
>  	debugfs_create_file("states", 0444, mhi_cntrl->debugfs_dentry,
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index c7a7354..4acad28 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,8 +959,8 @@ 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));
> -	mhi_dev->name = dev_name(mhi_cntrl->cntrl_dev);
> +	dev_set_name(&mhi_dev->dev, "mhi%d", mhi_cntrl->index);
> +	mhi_dev->name = dev_name(&mhi_dev->dev);
>  
>  	/* Init wakeup source */
>  	device_init_wakeup(&mhi_dev->dev, true);
> @@ -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 7f3c990..ea39df0 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -350,7 +350,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..30c676d 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -356,6 +356,7 @@ struct mhi_controller_config {
>   * @read_reg: Read a MHI register via the physical link (required)
>   * @write_reg: Write a MHI register via the physical link (required)
>   * @buffer_len: Bounce buffer length
> + * @index: Index of the MHI controller instance
>   * @bounce_buf: Use of bounce buffer
>   * @fbc_download: MHI host needs to do complete image transfer (optional)
>   * @pre_init: MHI host needs to do pre-initialization before power up
> @@ -446,6 +447,7 @@ struct mhi_controller {
>  			  u32 val);
>  
>  	size_t buffer_len;
> +	int index;
>  	bool bounce_buf;
>  	bool fbc_download;
>  	bool pre_init;
> -- 
> 2.7.4
>