| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200318143145.GA12341@Mani-XPS-13-9360>
Date: Wed, 18 Mar 2020 20:01:45 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: Jeffrey Hugo <jhugo@...eaurora.org>, arnd@...db.de,
smohanad@...eaurora.org, kvalo@...eaurora.org,
bjorn.andersson@...aro.org, hemantk@...eaurora.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 03/16] bus: mhi: core: Add support for registering MHI
client drivers
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@...eaurora.org>
> > > > > > Signed-off-by: Siddartha Mohanadoss <smohanad@...eaurora.org>
> > > > > > [mani: splitted and cleaned up for upstream]
> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > > > > > Reviewed-by: Jeffrey Hugo <jhugo@...eaurora.org>
> > > > > > Tested-by: Jeffrey Hugo <jhugo@...eaurora.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
Powered by blists - more mailing lists