mbox series

[v3,00/16] Add MHI bus support

Message ID 20200220095854.4804-1-manivannan.sadhasivam@linaro.org
Headers show
Series Add MHI bus support | expand

Message

Manivannan Sadhasivam Feb. 20, 2020, 9:58 a.m. UTC 
Hello,

This is the second attempt at adding the MHI (Modem Host Interface) bus
interface to Linux kernel. MHI is a communication protocol used by the
host processors to control and communicate with modems over a high
speed peripheral bus or shared memory. The MHI protocol has been
designed and developed by Qualcomm Innovation Center, Inc., for use
in their modems.

The first submission was made by Sujeev Dias of Qualcomm:

https://lkml.org/lkml/2018/4/26/1159
https://lkml.org/lkml/2018/7/9/987

This series addresses most of the review comments by Greg and Arnd for
the initial patchset. Furthermore, in order to ease the review process
I've splitted the patches logically and dropped few of them which were
not required for this initial submission.

Below is the high level changelog:

1. Removed all DT related code
2. Got rid of pci specific struct members from top level mhi structs
3. Moved device specific callbacks like ul_xfer() to driver struct. It
   doesn’t make sense to have callbacks in device struct as suggested by
   Greg
4. Used priv data of `struct device` instead of own priv data in
   `mhi_device` as suggested by Greg. This will allow us to use
    dev_set{get}_drvdata() APIs in client drivers
5. Removed all debugfs related code
6. Changes to the APIs to look uniform
7. Converted the documentation to .rst and placed in its own subdirectory
8. Changes to the MHI device naming
9. Converted all uppercase variable names to appropriate lowercase ones
10. Removed custom debug code and used the dev_* ones where applicable
11. Dropped timesync, DTR, UCI, and Qcom controller related codes
12. Added QRTR client driver patch
13. Added modalias support for the MHI stack as well as client driver for
    autoloading of modules (client drivers) by udev once the MHI devices
    are created

This series includes the MHI stack as well as the QRTR client driver which
falls under the networking subsystem.

The reference controller implementation is here:
https://git.linaro.org/people/manivannan.sadhasivam/linux.git/tree/drivers/net/wireless/ath/ath11k/mhi.c?h=ath11k-qca6390-mhi
It will be submitted later along with ath11k patches.

Following developers deserve explicit acknowledgements for their
contributions to the MHI code:

Sujeev Dias
Siddartha Mohanadoss
Hemant Kumar
Jeff Hugo

Thanks,
Mani

Changes in v3:

* Fixed the MHI dev refcounting issue and avoided the use of nullifying the
  mhi_dev pointer in mhi_release_device
* Fixed the kref issue in QRTR MHI client driver
* Renamed mhi_cntrl->dev to mhi_cntrl->cntrl_dev to avoid confusion
* Exposed mhi_queue_* APIs to client drivers and got rid of queue_xfer callback
* Removed mhi_buf_type as it is no longer required
* Misc cleanups in the MHI stack
* Added Jeff's Reviewed-by and Tested-by tags

Changes in v2:

* Added put_device to mhi_dealloc_device
* Removed unused members from struct mhi_controller
* Removed the atomicity of dev_wake in struct mhi_device as it is not required
* Reordered MHI structs to avoid holes
* Used struct device name for the controller device
* Marked the required and optional mhi_controller members for helping the
  controller driver implementation
* Cleanups to the MHI doc
* Removed _relaxed variants and used readl/writel
* Added comments for MHI specific acronyms
* Removed the usage of bitfields and used bitmasks for mhi_event_ctxt and
  mhi_chan_ctxt
* Used __64/__u32 types for structures representing hw states
* Added Hemant as a co-maintainer of MHI bus. He is from the MHI team of
  Qualcomm and he will take up reviews and if possible, maintainership
  in future.

Manivannan Sadhasivam (16):
  docs: Add documentation for MHI bus
  bus: mhi: core: Add support for registering MHI controllers
  bus: mhi: core: Add support for registering MHI client drivers
  bus: mhi: core: Add support for creating and destroying MHI devices
  bus: mhi: core: Add support for ringing channel/event ring doorbells
  bus: mhi: core: Add support for PM state transitions
  bus: mhi: core: Add support for basic PM operations
  bus: mhi: core: Add support for downloading firmware over BHIe
  bus: mhi: core: Add support for downloading RDDM image during panic
  bus: mhi: core: Add support for processing events from client device
  bus: mhi: core: Add support for data transfer
  bus: mhi: core: Add uevent support for module autoloading
  MAINTAINERS: Add entry for MHI bus
  net: qrtr: Add MHI transport layer
  net: qrtr: Do not depend on ARCH_QCOM
  soc: qcom: Do not depend on ARCH_QCOM for QMI helpers

 Documentation/index.rst           |    1 +
 Documentation/mhi/index.rst       |   18 +
 Documentation/mhi/mhi.rst         |  218 +++++
 Documentation/mhi/topology.rst    |   60 ++
 MAINTAINERS                       |   10 +
 drivers/bus/Kconfig               |    1 +
 drivers/bus/Makefile              |    3 +
 drivers/bus/mhi/Kconfig           |   14 +
 drivers/bus/mhi/Makefile          |    2 +
 drivers/bus/mhi/core/Makefile     |    3 +
 drivers/bus/mhi/core/boot.c       |  507 ++++++++++
 drivers/bus/mhi/core/init.c       | 1264 ++++++++++++++++++++++++
 drivers/bus/mhi/core/internal.h   |  677 +++++++++++++
 drivers/bus/mhi/core/main.c       | 1516 +++++++++++++++++++++++++++++
 drivers/bus/mhi/core/pm.c         |  969 ++++++++++++++++++
 drivers/soc/qcom/Kconfig          |    1 -
 include/linux/mhi.h               |  666 +++++++++++++
 include/linux/mod_devicetable.h   |   13 +
 net/qrtr/Kconfig                  |    8 +-
 net/qrtr/Makefile                 |    2 +
 net/qrtr/mhi.c                    |  209 ++++
 scripts/mod/devicetable-offsets.c |    3 +
 scripts/mod/file2alias.c          |   10 +
 23 files changed, 6173 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/mhi/index.rst
 create mode 100644 Documentation/mhi/mhi.rst
 create mode 100644 Documentation/mhi/topology.rst
 create mode 100644 drivers/bus/mhi/Kconfig
 create mode 100644 drivers/bus/mhi/Makefile
 create mode 100644 drivers/bus/mhi/core/Makefile
 create mode 100644 drivers/bus/mhi/core/boot.c
 create mode 100644 drivers/bus/mhi/core/init.c
 create mode 100644 drivers/bus/mhi/core/internal.h
 create mode 100644 drivers/bus/mhi/core/main.c
 create mode 100644 drivers/bus/mhi/core/pm.c
 create mode 100644 include/linux/mhi.h
 create mode 100644 net/qrtr/mhi.c

Comments

Greg Kroah-Hartman March 18, 2020, 1:36 p.m. UTC | #1 
On Thu, Feb 20, 2020 at 03:28:41PM +0530, Manivannan Sadhasivam wrote:
> This commit adds support for registering MHI client drivers with the
> MHI stack. MHI client drivers binds to one or more MHI devices inorder
> to sends and receive the upper-layer protocol packets like IP packets,
> modem control messages, and diagnostics messages over MHI bus.
> 
> This is based on the patch submitted by Sujeev Dias:
> https://lkml.org/lkml/2018/7/9/987
> 
> Signed-off-by: Sujeev Dias <sdias@codeaurora.org>
> Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> [mani: splitted and cleaned up for upstream]
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
> Tested-by: Jeffrey Hugo <jhugo@codeaurora.org>
> ---
>  drivers/bus/mhi/core/init.c | 149 ++++++++++++++++++++++++++++++++++++
>  include/linux/mhi.h         |  39 ++++++++++
>  2 files changed, 188 insertions(+)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 6f24c21284ec..12e386862b3f 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -374,8 +374,157 @@ struct mhi_device *mhi_alloc_device(struct mhi_controller *mhi_cntrl)
>  	return mhi_dev;
>  }
>  
> +static int mhi_driver_probe(struct device *dev)
> +{
> +	struct mhi_device *mhi_dev = to_mhi_device(dev);
> +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> +	struct device_driver *drv = dev->driver;
> +	struct mhi_driver *mhi_drv = to_mhi_driver(drv);
> +	struct mhi_event *mhi_event;
> +	struct mhi_chan *ul_chan = mhi_dev->ul_chan;
> +	struct mhi_chan *dl_chan = mhi_dev->dl_chan;
> +
> +	if (ul_chan) {
> +		/*
> +		 * If channel supports LPM notifications then status_cb should
> +		 * be provided
> +		 */
> +		if (ul_chan->lpm_notify && !mhi_drv->status_cb)
> +			return -EINVAL;
> +
> +		/* For non-offload channels then xfer_cb should be provided */
> +		if (!ul_chan->offload_ch && !mhi_drv->ul_xfer_cb)
> +			return -EINVAL;
> +
> +		ul_chan->xfer_cb = mhi_drv->ul_xfer_cb;
> +	}
> +
> +	if (dl_chan) {
> +		/*
> +		 * If channel supports LPM notifications then status_cb should
> +		 * be provided
> +		 */
> +		if (dl_chan->lpm_notify && !mhi_drv->status_cb)
> +			return -EINVAL;
> +
> +		/* For non-offload channels then xfer_cb should be provided */
> +		if (!dl_chan->offload_ch && !mhi_drv->dl_xfer_cb)
> +			return -EINVAL;
> +
> +		mhi_event = &mhi_cntrl->mhi_event[dl_chan->er_index];
> +
> +		/*
> +		 * If the channel event ring is managed by client, then
> +		 * status_cb must be provided so that the framework can
> +		 * notify pending data
> +		 */
> +		if (mhi_event->cl_manage && !mhi_drv->status_cb)
> +			return -EINVAL;
> +
> +		dl_chan->xfer_cb = mhi_drv->dl_xfer_cb;
> +	}
> +
> +	/* Call the user provided probe function */
> +	return mhi_drv->probe(mhi_dev, mhi_dev->id);
> +}
> +
> +static int mhi_driver_remove(struct device *dev)
> +{
> +	struct mhi_device *mhi_dev = to_mhi_device(dev);
> +	struct mhi_driver *mhi_drv = to_mhi_driver(dev->driver);
> +	struct mhi_chan *mhi_chan;
> +	enum mhi_ch_state ch_state[] = {
> +		MHI_CH_STATE_DISABLED,
> +		MHI_CH_STATE_DISABLED
> +	};
> +	int dir;
> +
> +	/* Skip if it is a controller device */
> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> +		return 0;
> +
> +	/* Reset both channels */
> +	for (dir = 0; dir < 2; dir++) {
> +		mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
> +
> +		if (!mhi_chan)
> +			continue;
> +
> +		/* Wake all threads waiting for completion */
> +		write_lock_irq(&mhi_chan->lock);
> +		mhi_chan->ccs = MHI_EV_CC_INVALID;
> +		complete_all(&mhi_chan->completion);
> +		write_unlock_irq(&mhi_chan->lock);
> +
> +		/* Set the channel state to disabled */
> +		mutex_lock(&mhi_chan->mutex);
> +		write_lock_irq(&mhi_chan->lock);
> +		ch_state[dir] = mhi_chan->ch_state;
> +		mhi_chan->ch_state = MHI_CH_STATE_SUSPENDED;
> +		write_unlock_irq(&mhi_chan->lock);
> +
> +		mutex_unlock(&mhi_chan->mutex);
> +	}
> +
> +	mhi_drv->remove(mhi_dev);
> +
> +	/* De-init channel if it was enabled */
> +	for (dir = 0; dir < 2; dir++) {
> +		mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
> +
> +		if (!mhi_chan)
> +			continue;
> +
> +		mutex_lock(&mhi_chan->mutex);
> +
> +		mhi_chan->ch_state = MHI_CH_STATE_DISABLED;
> +
> +		mutex_unlock(&mhi_chan->mutex);
> +	}
> +
> +	return 0;
> +}
> +
> +int mhi_driver_register(struct mhi_driver *mhi_drv)
> +{
> +	struct device_driver *driver = &mhi_drv->driver;
> +
> +	if (!mhi_drv->probe || !mhi_drv->remove)
> +		return -EINVAL;
> +
> +	driver->bus = &mhi_bus_type;
> +	driver->probe = mhi_driver_probe;
> +	driver->remove = mhi_driver_remove;
> +
> +	return driver_register(driver);
> +}
> +EXPORT_SYMBOL_GPL(mhi_driver_register);

You don't care about module owners of the driver?  Odd :(

(hint, you probably should...)

greg k-h
Jeffrey Hugo March 18, 2020, 1:54 p.m. UTC | #2 
On 3/18/2020 7:36 AM, Greg KH wrote:
> On Thu, Feb 20, 2020 at 03:28:41PM +0530, Manivannan Sadhasivam wrote:
>> This commit adds support for registering MHI client drivers with the
>> MHI stack. MHI client drivers binds to one or more MHI devices inorder
>> to sends and receive the upper-layer protocol packets like IP packets,
>> modem control messages, and diagnostics messages over MHI bus.
>>
>> This is based on the patch submitted by Sujeev Dias:
>> https://lkml.org/lkml/2018/7/9/987
>>
>> Signed-off-by: Sujeev Dias <sdias@codeaurora.org>
>> Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
>> [mani: splitted and cleaned up for upstream]
>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
>> Tested-by: Jeffrey Hugo <jhugo@codeaurora.org>
>> ---
>>   drivers/bus/mhi/core/init.c | 149 ++++++++++++++++++++++++++++++++++++
>>   include/linux/mhi.h         |  39 ++++++++++
>>   2 files changed, 188 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> index 6f24c21284ec..12e386862b3f 100644
>> --- a/drivers/bus/mhi/core/init.c
>> +++ b/drivers/bus/mhi/core/init.c
>> @@ -374,8 +374,157 @@ struct mhi_device *mhi_alloc_device(struct mhi_controller *mhi_cntrl)
>>   	return mhi_dev;
>>   }
>>   
>> +static int mhi_driver_probe(struct device *dev)
>> +{
>> +	struct mhi_device *mhi_dev = to_mhi_device(dev);
>> +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>> +	struct device_driver *drv = dev->driver;
>> +	struct mhi_driver *mhi_drv = to_mhi_driver(drv);
>> +	struct mhi_event *mhi_event;
>> +	struct mhi_chan *ul_chan = mhi_dev->ul_chan;
>> +	struct mhi_chan *dl_chan = mhi_dev->dl_chan;
>> +
>> +	if (ul_chan) {
>> +		/*
>> +		 * If channel supports LPM notifications then status_cb should
>> +		 * be provided
>> +		 */
>> +		if (ul_chan->lpm_notify && !mhi_drv->status_cb)
>> +			return -EINVAL;
>> +
>> +		/* For non-offload channels then xfer_cb should be provided */
>> +		if (!ul_chan->offload_ch && !mhi_drv->ul_xfer_cb)
>> +			return -EINVAL;
>> +
>> +		ul_chan->xfer_cb = mhi_drv->ul_xfer_cb;
>> +	}
>> +
>> +	if (dl_chan) {
>> +		/*
>> +		 * If channel supports LPM notifications then status_cb should
>> +		 * be provided
>> +		 */
>> +		if (dl_chan->lpm_notify && !mhi_drv->status_cb)
>> +			return -EINVAL;
>> +
>> +		/* For non-offload channels then xfer_cb should be provided */
>> +		if (!dl_chan->offload_ch && !mhi_drv->dl_xfer_cb)
>> +			return -EINVAL;
>> +
>> +		mhi_event = &mhi_cntrl->mhi_event[dl_chan->er_index];
>> +
>> +		/*
>> +		 * If the channel event ring is managed by client, then
>> +		 * status_cb must be provided so that the framework can
>> +		 * notify pending data
>> +		 */
>> +		if (mhi_event->cl_manage && !mhi_drv->status_cb)
>> +			return -EINVAL;
>> +
>> +		dl_chan->xfer_cb = mhi_drv->dl_xfer_cb;
>> +	}
>> +
>> +	/* Call the user provided probe function */
>> +	return mhi_drv->probe(mhi_dev, mhi_dev->id);
>> +}
>> +
>> +static int mhi_driver_remove(struct device *dev)
>> +{
>> +	struct mhi_device *mhi_dev = to_mhi_device(dev);
>> +	struct mhi_driver *mhi_drv = to_mhi_driver(dev->driver);
>> +	struct mhi_chan *mhi_chan;
>> +	enum mhi_ch_state ch_state[] = {
>> +		MHI_CH_STATE_DISABLED,
>> +		MHI_CH_STATE_DISABLED
>> +	};
>> +	int dir;
>> +
>> +	/* Skip if it is a controller device */
>> +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
>> +		return 0;
>> +
>> +	/* Reset both channels */
>> +	for (dir = 0; dir < 2; dir++) {
>> +		mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
>> +
>> +		if (!mhi_chan)
>> +			continue;
>> +
>> +		/* Wake all threads waiting for completion */
>> +		write_lock_irq(&mhi_chan->lock);
>> +		mhi_chan->ccs = MHI_EV_CC_INVALID;
>> +		complete_all(&mhi_chan->completion);
>> +		write_unlock_irq(&mhi_chan->lock);
>> +
>> +		/* Set the channel state to disabled */
>> +		mutex_lock(&mhi_chan->mutex);
>> +		write_lock_irq(&mhi_chan->lock);
>> +		ch_state[dir] = mhi_chan->ch_state;
>> +		mhi_chan->ch_state = MHI_CH_STATE_SUSPENDED;
>> +		write_unlock_irq(&mhi_chan->lock);
>> +
>> +		mutex_unlock(&mhi_chan->mutex);
>> +	}
>> +
>> +	mhi_drv->remove(mhi_dev);
>> +
>> +	/* De-init channel if it was enabled */
>> +	for (dir = 0; dir < 2; dir++) {
>> +		mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
>> +
>> +		if (!mhi_chan)
>> +			continue;
>> +
>> +		mutex_lock(&mhi_chan->mutex);
>> +
>> +		mhi_chan->ch_state = MHI_CH_STATE_DISABLED;
>> +
>> +		mutex_unlock(&mhi_chan->mutex);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int mhi_driver_register(struct mhi_driver *mhi_drv)
>> +{
>> +	struct device_driver *driver = &mhi_drv->driver;
>> +
>> +	if (!mhi_drv->probe || !mhi_drv->remove)
>> +		return -EINVAL;
>> +
>> +	driver->bus = &mhi_bus_type;
>> +	driver->probe = mhi_driver_probe;
>> +	driver->remove = mhi_driver_remove;
>> +
>> +	return driver_register(driver);
>> +}
>> +EXPORT_SYMBOL_GPL(mhi_driver_register);
> 
> You don't care about module owners of the driver?  Odd :(
> 
> (hint, you probably should...)
> 
> greg k-h
> 

For my own education, can you please clarify your comment?  I'm not sure 
that I understand the context of what you are saying (ie why is this 
export a possible problem?).
Manivannan Sadhasivam March 18, 2020, 2:23 p.m. UTC | #3 
Hi Greg,

On Wed, Mar 18, 2020 at 03:00:34PM +0100, Greg KH wrote:
> On Wed, Mar 18, 2020 at 07:54:30AM -0600, Jeffrey Hugo wrote:
> > On 3/18/2020 7:36 AM, Greg KH wrote:
> > > On Thu, Feb 20, 2020 at 03:28:41PM +0530, Manivannan Sadhasivam wrote:
> > > > This commit adds support for registering MHI client drivers with the
> > > > MHI stack. MHI client drivers binds to one or more MHI devices inorder
> > > > to sends and receive the upper-layer protocol packets like IP packets,
> > > > modem control messages, and diagnostics messages over MHI bus.
> > > > 
> > > > This is based on the patch submitted by Sujeev Dias:
> > > > https://lkml.org/lkml/2018/7/9/987
> > > > 
> > > > Signed-off-by: Sujeev Dias <sdias@codeaurora.org>
> > > > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > > [mani: splitted and cleaned up for upstream]
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
> > > > Tested-by: Jeffrey Hugo <jhugo@codeaurora.org>
> > > > ---
> > > >   drivers/bus/mhi/core/init.c | 149 ++++++++++++++++++++++++++++++++++++
> > > >   include/linux/mhi.h         |  39 ++++++++++
> > > >   2 files changed, 188 insertions(+)
> > > > 
> > > > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > > > index 6f24c21284ec..12e386862b3f 100644
> > > > --- a/drivers/bus/mhi/core/init.c
> > > > +++ b/drivers/bus/mhi/core/init.c
> > > > @@ -374,8 +374,157 @@ struct mhi_device *mhi_alloc_device(struct mhi_controller *mhi_cntrl)
> > > >   	return mhi_dev;
> > > >   }
> > > > +static int mhi_driver_probe(struct device *dev)
> > > > +{
> > > > +	struct mhi_device *mhi_dev = to_mhi_device(dev);
> > > > +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> > > > +	struct device_driver *drv = dev->driver;
> > > > +	struct mhi_driver *mhi_drv = to_mhi_driver(drv);
> > > > +	struct mhi_event *mhi_event;
> > > > +	struct mhi_chan *ul_chan = mhi_dev->ul_chan;
> > > > +	struct mhi_chan *dl_chan = mhi_dev->dl_chan;
> > > > +
> > > > +	if (ul_chan) {
> > > > +		/*
> > > > +		 * If channel supports LPM notifications then status_cb should
> > > > +		 * be provided
> > > > +		 */
> > > > +		if (ul_chan->lpm_notify && !mhi_drv->status_cb)
> > > > +			return -EINVAL;
> > > > +
> > > > +		/* For non-offload channels then xfer_cb should be provided */
> > > > +		if (!ul_chan->offload_ch && !mhi_drv->ul_xfer_cb)
> > > > +			return -EINVAL;
> > > > +
> > > > +		ul_chan->xfer_cb = mhi_drv->ul_xfer_cb;
> > > > +	}
> > > > +
> > > > +	if (dl_chan) {
> > > > +		/*
> > > > +		 * If channel supports LPM notifications then status_cb should
> > > > +		 * be provided
> > > > +		 */
> > > > +		if (dl_chan->lpm_notify && !mhi_drv->status_cb)
> > > > +			return -EINVAL;
> > > > +
> > > > +		/* For non-offload channels then xfer_cb should be provided */
> > > > +		if (!dl_chan->offload_ch && !mhi_drv->dl_xfer_cb)
> > > > +			return -EINVAL;
> > > > +
> > > > +		mhi_event = &mhi_cntrl->mhi_event[dl_chan->er_index];
> > > > +
> > > > +		/*
> > > > +		 * If the channel event ring is managed by client, then
> > > > +		 * status_cb must be provided so that the framework can
> > > > +		 * notify pending data
> > > > +		 */
> > > > +		if (mhi_event->cl_manage && !mhi_drv->status_cb)
> > > > +			return -EINVAL;
> > > > +
> > > > +		dl_chan->xfer_cb = mhi_drv->dl_xfer_cb;
> > > > +	}
> > > > +
> > > > +	/* Call the user provided probe function */
> > > > +	return mhi_drv->probe(mhi_dev, mhi_dev->id);
> > > > +}
> > > > +
> > > > +static int mhi_driver_remove(struct device *dev)
> > > > +{
> > > > +	struct mhi_device *mhi_dev = to_mhi_device(dev);
> > > > +	struct mhi_driver *mhi_drv = to_mhi_driver(dev->driver);
> > > > +	struct mhi_chan *mhi_chan;
> > > > +	enum mhi_ch_state ch_state[] = {
> > > > +		MHI_CH_STATE_DISABLED,
> > > > +		MHI_CH_STATE_DISABLED
> > > > +	};
> > > > +	int dir;
> > > > +
> > > > +	/* Skip if it is a controller device */
> > > > +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> > > > +		return 0;
> > > > +
> > > > +	/* Reset both channels */
> > > > +	for (dir = 0; dir < 2; dir++) {
> > > > +		mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
> > > > +
> > > > +		if (!mhi_chan)
> > > > +			continue;
> > > > +
> > > > +		/* Wake all threads waiting for completion */
> > > > +		write_lock_irq(&mhi_chan->lock);
> > > > +		mhi_chan->ccs = MHI_EV_CC_INVALID;
> > > > +		complete_all(&mhi_chan->completion);
> > > > +		write_unlock_irq(&mhi_chan->lock);
> > > > +
> > > > +		/* Set the channel state to disabled */
> > > > +		mutex_lock(&mhi_chan->mutex);
> > > > +		write_lock_irq(&mhi_chan->lock);
> > > > +		ch_state[dir] = mhi_chan->ch_state;
> > > > +		mhi_chan->ch_state = MHI_CH_STATE_SUSPENDED;
> > > > +		write_unlock_irq(&mhi_chan->lock);
> > > > +
> > > > +		mutex_unlock(&mhi_chan->mutex);
> > > > +	}
> > > > +
> > > > +	mhi_drv->remove(mhi_dev);
> > > > +
> > > > +	/* De-init channel if it was enabled */
> > > > +	for (dir = 0; dir < 2; dir++) {
> > > > +		mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
> > > > +
> > > > +		if (!mhi_chan)
> > > > +			continue;
> > > > +
> > > > +		mutex_lock(&mhi_chan->mutex);
> > > > +
> > > > +		mhi_chan->ch_state = MHI_CH_STATE_DISABLED;
> > > > +
> > > > +		mutex_unlock(&mhi_chan->mutex);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int mhi_driver_register(struct mhi_driver *mhi_drv)
> > > > +{
> > > > +	struct device_driver *driver = &mhi_drv->driver;
> > > > +
> > > > +	if (!mhi_drv->probe || !mhi_drv->remove)
> > > > +		return -EINVAL;
> > > > +
> > > > +	driver->bus = &mhi_bus_type;
> > > > +	driver->probe = mhi_driver_probe;
> > > > +	driver->remove = mhi_driver_remove;
> > > > +
> > > > +	return driver_register(driver);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(mhi_driver_register);
> > > 
> > > You don't care about module owners of the driver?  Odd :(
> > > 
> > > (hint, you probably should...)
> > > 
> > > greg k-h
> > > 
> > 
> > For my own education, can you please clarify your comment?  I'm not sure
> > that I understand the context of what you are saying (ie why is this export
> > a possible problem?).
> 
> Sorry, it didn't have to do with the export, it had to do with the fact
> that your driver_register() function does not pass in the owner of the
> module of the driver, like almost all other subsystems do.  That way you
> can try to protect the module from being unloaded if it has files open
> assigned to it.
> 
> If you don't have any userspace accesses like that, to the driver, then
> nevermind, all is fine :)
> 

This is not needed right now but I'll fix this anyway to avoid issues in
future :)

Btw, may I know the status of this series? Do you have any more comments
or do you happen to wait for more reviews?

Thanks,
Mani

> thanks,
> 
> greg k-h
Manivannan Sadhasivam March 18, 2020, 2:31 p.m. UTC | #4 
On Wed, Mar 18, 2020 at 03:27:30PM +0100, Greg KH wrote:
> On Wed, Mar 18, 2020 at 07:53:12PM +0530, Manivannan Sadhasivam wrote:
> > Hi Greg,
> > 
> > On Wed, Mar 18, 2020 at 03:00:34PM +0100, Greg KH wrote:
> > > On Wed, Mar 18, 2020 at 07:54:30AM -0600, Jeffrey Hugo wrote:
> > > > On 3/18/2020 7:36 AM, Greg KH wrote:
> > > > > On Thu, Feb 20, 2020 at 03:28:41PM +0530, Manivannan Sadhasivam wrote:
> > > > > > This commit adds support for registering MHI client drivers with the
> > > > > > MHI stack. MHI client drivers binds to one or more MHI devices inorder
> > > > > > to sends and receive the upper-layer protocol packets like IP packets,
> > > > > > modem control messages, and diagnostics messages over MHI bus.
> > > > > > 
> > > > > > This is based on the patch submitted by Sujeev Dias:
> > > > > > https://lkml.org/lkml/2018/7/9/987
> > > > > > 
> > > > > > Signed-off-by: Sujeev Dias <sdias@codeaurora.org>
> > > > > > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > > > > > [mani: splitted and cleaned up for upstream]
> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > > Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
> > > > > > Tested-by: Jeffrey Hugo <jhugo@codeaurora.org>
> > > > > > ---
> > > > > >   drivers/bus/mhi/core/init.c | 149 ++++++++++++++++++++++++++++++++++++
> > > > > >   include/linux/mhi.h         |  39 ++++++++++
> > > > > >   2 files changed, 188 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > > > > > index 6f24c21284ec..12e386862b3f 100644
> > > > > > --- a/drivers/bus/mhi/core/init.c
> > > > > > +++ b/drivers/bus/mhi/core/init.c
> > > > > > @@ -374,8 +374,157 @@ struct mhi_device *mhi_alloc_device(struct mhi_controller *mhi_cntrl)
> > > > > >   	return mhi_dev;
> > > > > >   }
> > > > > > +static int mhi_driver_probe(struct device *dev)
> > > > > > +{
> > > > > > +	struct mhi_device *mhi_dev = to_mhi_device(dev);
> > > > > > +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> > > > > > +	struct device_driver *drv = dev->driver;
> > > > > > +	struct mhi_driver *mhi_drv = to_mhi_driver(drv);
> > > > > > +	struct mhi_event *mhi_event;
> > > > > > +	struct mhi_chan *ul_chan = mhi_dev->ul_chan;
> > > > > > +	struct mhi_chan *dl_chan = mhi_dev->dl_chan;
> > > > > > +
> > > > > > +	if (ul_chan) {
> > > > > > +		/*
> > > > > > +		 * If channel supports LPM notifications then status_cb should
> > > > > > +		 * be provided
> > > > > > +		 */
> > > > > > +		if (ul_chan->lpm_notify && !mhi_drv->status_cb)
> > > > > > +			return -EINVAL;
> > > > > > +
> > > > > > +		/* For non-offload channels then xfer_cb should be provided */
> > > > > > +		if (!ul_chan->offload_ch && !mhi_drv->ul_xfer_cb)
> > > > > > +			return -EINVAL;
> > > > > > +
> > > > > > +		ul_chan->xfer_cb = mhi_drv->ul_xfer_cb;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (dl_chan) {
> > > > > > +		/*
> > > > > > +		 * If channel supports LPM notifications then status_cb should
> > > > > > +		 * be provided
> > > > > > +		 */
> > > > > > +		if (dl_chan->lpm_notify && !mhi_drv->status_cb)
> > > > > > +			return -EINVAL;
> > > > > > +
> > > > > > +		/* For non-offload channels then xfer_cb should be provided */
> > > > > > +		if (!dl_chan->offload_ch && !mhi_drv->dl_xfer_cb)
> > > > > > +			return -EINVAL;
> > > > > > +
> > > > > > +		mhi_event = &mhi_cntrl->mhi_event[dl_chan->er_index];
> > > > > > +
> > > > > > +		/*
> > > > > > +		 * If the channel event ring is managed by client, then
> > > > > > +		 * status_cb must be provided so that the framework can
> > > > > > +		 * notify pending data
> > > > > > +		 */
> > > > > > +		if (mhi_event->cl_manage && !mhi_drv->status_cb)
> > > > > > +			return -EINVAL;
> > > > > > +
> > > > > > +		dl_chan->xfer_cb = mhi_drv->dl_xfer_cb;
> > > > > > +	}
> > > > > > +
> > > > > > +	/* Call the user provided probe function */
> > > > > > +	return mhi_drv->probe(mhi_dev, mhi_dev->id);
> > > > > > +}
> > > > > > +
> > > > > > +static int mhi_driver_remove(struct device *dev)
> > > > > > +{
> > > > > > +	struct mhi_device *mhi_dev = to_mhi_device(dev);
> > > > > > +	struct mhi_driver *mhi_drv = to_mhi_driver(dev->driver);
> > > > > > +	struct mhi_chan *mhi_chan;
> > > > > > +	enum mhi_ch_state ch_state[] = {
> > > > > > +		MHI_CH_STATE_DISABLED,
> > > > > > +		MHI_CH_STATE_DISABLED
> > > > > > +	};
> > > > > > +	int dir;
> > > > > > +
> > > > > > +	/* Skip if it is a controller device */
> > > > > > +	if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	/* Reset both channels */
> > > > > > +	for (dir = 0; dir < 2; dir++) {
> > > > > > +		mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
> > > > > > +
> > > > > > +		if (!mhi_chan)
> > > > > > +			continue;
> > > > > > +
> > > > > > +		/* Wake all threads waiting for completion */
> > > > > > +		write_lock_irq(&mhi_chan->lock);
> > > > > > +		mhi_chan->ccs = MHI_EV_CC_INVALID;
> > > > > > +		complete_all(&mhi_chan->completion);
> > > > > > +		write_unlock_irq(&mhi_chan->lock);
> > > > > > +
> > > > > > +		/* Set the channel state to disabled */
> > > > > > +		mutex_lock(&mhi_chan->mutex);
> > > > > > +		write_lock_irq(&mhi_chan->lock);
> > > > > > +		ch_state[dir] = mhi_chan->ch_state;
> > > > > > +		mhi_chan->ch_state = MHI_CH_STATE_SUSPENDED;
> > > > > > +		write_unlock_irq(&mhi_chan->lock);
> > > > > > +
> > > > > > +		mutex_unlock(&mhi_chan->mutex);
> > > > > > +	}
> > > > > > +
> > > > > > +	mhi_drv->remove(mhi_dev);
> > > > > > +
> > > > > > +	/* De-init channel if it was enabled */
> > > > > > +	for (dir = 0; dir < 2; dir++) {
> > > > > > +		mhi_chan = dir ? mhi_dev->ul_chan : mhi_dev->dl_chan;
> > > > > > +
> > > > > > +		if (!mhi_chan)
> > > > > > +			continue;
> > > > > > +
> > > > > > +		mutex_lock(&mhi_chan->mutex);
> > > > > > +
> > > > > > +		mhi_chan->ch_state = MHI_CH_STATE_DISABLED;
> > > > > > +
> > > > > > +		mutex_unlock(&mhi_chan->mutex);
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +int mhi_driver_register(struct mhi_driver *mhi_drv)
> > > > > > +{
> > > > > > +	struct device_driver *driver = &mhi_drv->driver;
> > > > > > +
> > > > > > +	if (!mhi_drv->probe || !mhi_drv->remove)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	driver->bus = &mhi_bus_type;
> > > > > > +	driver->probe = mhi_driver_probe;
> > > > > > +	driver->remove = mhi_driver_remove;
> > > > > > +
> > > > > > +	return driver_register(driver);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(mhi_driver_register);
> > > > > 
> > > > > You don't care about module owners of the driver?  Odd :(
> > > > > 
> > > > > (hint, you probably should...)
> > > > > 
> > > > > greg k-h
> > > > > 
> > > > 
> > > > For my own education, can you please clarify your comment?  I'm not sure
> > > > that I understand the context of what you are saying (ie why is this export
> > > > a possible problem?).
> > > 
> > > Sorry, it didn't have to do with the export, it had to do with the fact
> > > that your driver_register() function does not pass in the owner of the
> > > module of the driver, like almost all other subsystems do.  That way you
> > > can try to protect the module from being unloaded if it has files open
> > > assigned to it.
> > > 
> > > If you don't have any userspace accesses like that, to the driver, then
> > > nevermind, all is fine :)
> > > 
> > 
> > This is not needed right now but I'll fix this anyway to avoid issues in
> > future :)
> > 
> > Btw, may I know the status of this series? Do you have any more comments
> > or do you happen to wait for more reviews?
> 
> It would be nice to get other reviews, but other than what I noticed
> above, it looks sane to me.  Am I the one supposed to take these
> patches?
> 

I guess so ;) Do you think I need to sent another revision incorporating
the module owner fix or it can come as an incremental patch? Btw, I do
have few more in pipeline :)

Thanks,
Mani

> thanks,
> 
> greg k-h
Greg Kroah-Hartman March 18, 2020, 2:42 p.m. UTC | #5 
On Thu, Feb 20, 2020 at 03:28:52PM +0530, Manivannan Sadhasivam wrote:
> MHI is the transport layer used for communicating to the external modems.
> Hence, this commit adds MHI transport layer support to QRTR for
> transferring the QMI messages over IPC Router.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  net/qrtr/Kconfig  |   7 ++
>  net/qrtr/Makefile |   2 +
>  net/qrtr/mhi.c    | 209 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 218 insertions(+)
>  create mode 100644 net/qrtr/mhi.c

I stopped here in this series, as I do not feel comfortable merging
stuff under net/.

Can you get some review by the networking developers and then I will be
ok with taking it through my tree.

thanks,

greg k-h