Re: [PATCH v3 03/16] bus: mhi: core: Add support for registering MHI client drivers

[Greg KH <gregkh@linuxfoundation.org>]

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 :)

thanks,

greg k-h