| Message ID | 20220212182117.49438-10-manivannan.sadhasivam@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add initial support for MHI endpoint stack | expand |
On 2/12/22 12:21 PM, Manivannan Sadhasivam wrote: > This commit adds support for registering MHI endpoint client drivers > with the MHI endpoint stack. MHI endpoint client drivers binds to one s/binds/bind/ > or more MHI endpoint devices inorder to send and receive the upper-layer > protocol packets like IP packets, modem control messages, and diagnostics > messages over MHI bus. I have a few more comments here but generally this looks good. -Alex > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/bus/mhi/ep/main.c | 86 +++++++++++++++++++++++++++++++++++++++ > include/linux/mhi_ep.h | 53 ++++++++++++++++++++++++ > 2 files changed, 139 insertions(+) > > diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c > index b006011d025d..f66404181972 100644 > --- a/drivers/bus/mhi/ep/main.c > +++ b/drivers/bus/mhi/ep/main.c > @@ -196,9 +196,89 @@ void mhi_ep_unregister_controller(struct mhi_ep_cntrl *mhi_cntrl) > } > EXPORT_SYMBOL_GPL(mhi_ep_unregister_controller); > > +static int mhi_ep_driver_probe(struct device *dev) > +{ > + struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev); > + struct mhi_ep_driver *mhi_drv = to_mhi_ep_driver(dev->driver); > + struct mhi_ep_chan *ul_chan = mhi_dev->ul_chan; > + struct mhi_ep_chan *dl_chan = mhi_dev->dl_chan; > + > + /* Client drivers should have callbacks for both channels */ > + if (!mhi_drv->ul_xfer_cb || !mhi_drv->dl_xfer_cb) > + return -EINVAL; > + > + ul_chan->xfer_cb = mhi_drv->ul_xfer_cb; > + dl_chan->xfer_cb = mhi_drv->dl_xfer_cb; > + > + return mhi_drv->probe(mhi_dev, mhi_dev->id); > +} > + > +static int mhi_ep_driver_remove(struct device *dev) > +{ > + struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev); > + struct mhi_ep_driver *mhi_drv = to_mhi_ep_driver(dev->driver); > + struct mhi_result result = {}; > + struct mhi_ep_chan *mhi_chan; > + int dir; > + > + /* Skip if it is a controller device */ > + if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER) > + return 0; > + It would be my preference to encapsulate the body of the following loop into a called function, then call that once for the UL channel and once for the DL channel. > + /* Disconnect the channels associated with the driver */ > + 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->lock); > + /* Send channel disconnect status to the client driver */ > + if (mhi_chan->xfer_cb) { > + result.transaction_status = -ENOTCONN; > + result.bytes_xferd = 0; > + mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); It appears the result is ignored here. If so, can we define the xfer_cb() function so that a NULL pointer may be supplied by the caller in cases like this? > + } > + > + /* Set channel state to DISABLED */ That comment is a little tautological. Just omit it. > + mhi_chan->state = MHI_CH_STATE_DISABLED; > + mhi_chan->xfer_cb = NULL; > + mutex_unlock(&mhi_chan->lock); > + } > + > + /* Remove the client driver now */ > + mhi_drv->remove(mhi_dev); > + > + return 0; > +} > + > +int __mhi_ep_driver_register(struct mhi_ep_driver *mhi_drv, struct module *owner) > +{ > + struct device_driver *driver = &mhi_drv->driver; > + > + if (!mhi_drv->probe || !mhi_drv->remove) > + return -EINVAL; > + > + driver->bus = &mhi_ep_bus_type; > + driver->owner = owner; > + driver->probe = mhi_ep_driver_probe; > + driver->remove = mhi_ep_driver_remove; > + > + return driver_register(driver); > +} > +EXPORT_SYMBOL_GPL(__mhi_ep_driver_register); > + > +void mhi_ep_driver_unregister(struct mhi_ep_driver *mhi_drv) > +{ > + driver_unregister(&mhi_drv->driver); > +} > +EXPORT_SYMBOL_GPL(mhi_ep_driver_unregister); > + > static int mhi_ep_match(struct device *dev, struct device_driver *drv) > { > struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev); > + struct mhi_ep_driver *mhi_drv = to_mhi_ep_driver(drv); > + const struct mhi_device_id *id; > > /* > * If the device is a controller type then there is no client driver > @@ -207,6 +287,12 @@ static int mhi_ep_match(struct device *dev, struct device_driver *drv) > if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER) > return 0; > > + for (id = mhi_drv->id_table; id->chan[0]; id++) > + if (!strcmp(mhi_dev->name, id->chan)) { > + mhi_dev->id = id; > + return 1; > + } > + > return 0; > }; > > diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h > index 20238e9df1b3..da865f9d3646 100644 > --- a/include/linux/mhi_ep.h > +++ b/include/linux/mhi_ep.h > @@ -122,7 +122,60 @@ struct mhi_ep_device { > enum mhi_device_type dev_type; > }; > > +/** > + * struct mhi_ep_driver - Structure representing a MHI Endpoint client driver > + * @id_table: Pointer to MHI Endpoint device ID table > + * @driver: Device driver model driver > + * @probe: CB function for client driver probe function > + * @remove: CB function for client driver remove function > + * @ul_xfer_cb: CB function for UL data transfer > + * @dl_xfer_cb: CB function for DL data transfer > + */ > +struct mhi_ep_driver { > + const struct mhi_device_id *id_table; > + struct device_driver driver; > + int (*probe)(struct mhi_ep_device *mhi_ep, > + const struct mhi_device_id *id); > + void (*remove)(struct mhi_ep_device *mhi_ep); I get confused by the "ul" versus "dl" naming scheme here. Is "ul" from the perspective of the host, meaning upload is from the host toward the WWAN network (and therefore toward the SDX AP), and download is from the WWAN toward the host? Somewhere this should be stated clearly in comments; maybe I just missed it. > + void (*ul_xfer_cb)(struct mhi_ep_device *mhi_dev, > + struct mhi_result *result); > + void (*dl_xfer_cb)(struct mhi_ep_device *mhi_dev, > + struct mhi_result *result); > +}; > + > #define to_mhi_ep_device(dev) container_of(dev, struct mhi_ep_device, dev) > +#define to_mhi_ep_driver(drv) container_of(drv, struct mhi_ep_driver, driver) > + > +/* > + * module_mhi_ep_driver() - Helper macro for drivers that don't do > + * anything special other than using default mhi_ep_driver_register() and > + * mhi_ep_driver_unregister(). This eliminates a lot of boilerplate. > + * Each module may only use this macro once. > + */ > +#define module_mhi_ep_driver(mhi_drv) \ > + module_driver(mhi_drv, mhi_ep_driver_register, \ > + mhi_ep_driver_unregister) > + > +/* > + * Macro to avoid include chaining to get THIS_MODULE > + */ > +#define mhi_ep_driver_register(mhi_drv) \ > + __mhi_ep_driver_register(mhi_drv, THIS_MODULE) > + > +/** > + * __mhi_ep_driver_register - Register a driver with MHI Endpoint bus > + * @mhi_drv: Driver to be associated with the device > + * @owner: The module owner > + * > + * Return: 0 if driver registrations succeeds, a negative error code otherwise. > + */ > +int __mhi_ep_driver_register(struct mhi_ep_driver *mhi_drv, struct module *owner); > + > +/** > + * mhi_ep_driver_unregister - Unregister a driver from MHI Endpoint bus > + * @mhi_drv: Driver associated with the device > + */ > +void mhi_ep_driver_unregister(struct mhi_ep_driver *mhi_drv); > > /** > * mhi_ep_register_controller - Register MHI Endpoint controller
On Tue, Feb 15, 2022 at 02:02:50PM -0600, Alex Elder wrote: [...] > > +static int mhi_ep_driver_remove(struct device *dev) > > +{ > > + struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev); > > + struct mhi_ep_driver *mhi_drv = to_mhi_ep_driver(dev->driver); > > + struct mhi_result result = {}; > > + struct mhi_ep_chan *mhi_chan; > > + int dir; > > + > > + /* Skip if it is a controller device */ > > + if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER) > > + return 0; > > + > > It would be my preference to encapsulate the body of the > following loop into a called function, then call that once > for the UL channel and once for the DL channel. > This follows the host stack, so I'd like to keep it the same. > > + /* Disconnect the channels associated with the driver */ > > + 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->lock); > > + /* Send channel disconnect status to the client driver */ > > + if (mhi_chan->xfer_cb) { > > + result.transaction_status = -ENOTCONN; > > + result.bytes_xferd = 0; > > + mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); > > It appears the result is ignored here. If so, can we > define the xfer_cb() function so that a NULL pointer may > be supplied by the caller in cases like this? > result is not ignored, only the bytes_xfered. "transaction_status" will be used by the client drivers for error handling. > > + } > > + > > + /* Set channel state to DISABLED */ > > That comment is a little tautological. Just omit it. > > > + mhi_chan->state = MHI_CH_STATE_DISABLED; > > + mhi_chan->xfer_cb = NULL; > > + mutex_unlock(&mhi_chan->lock); > > + } > > + > > + /* Remove the client driver now */ > > + mhi_drv->remove(mhi_dev); > > + > > + return 0; > > +} [...] > > +struct mhi_ep_driver { > > + const struct mhi_device_id *id_table; > > + struct device_driver driver; > > + int (*probe)(struct mhi_ep_device *mhi_ep, > > + const struct mhi_device_id *id); > > + void (*remove)(struct mhi_ep_device *mhi_ep); > > I get confused by the "ul" versus "dl" naming scheme here. > Is "ul" from the perspective of the host, meaning upload > is from the host toward the WWAN network (and therefore > toward the SDX AP), and download is from the WWAN toward > the host? Somewhere this should be stated clearly in > comments; maybe I just missed it. > Yes UL and DL are as per host context. I didn't state this explicitly since this is the MHI host stack behaviour but I'll add a comment for clarity Thanks, Mani
On 2/17/22 4:20 AM, Manivannan Sadhasivam wrote: > On Tue, Feb 15, 2022 at 02:02:50PM -0600, Alex Elder wrote: > > [...] > >>> +static int mhi_ep_driver_remove(struct device *dev) >>> +{ >>> + struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev); >>> + struct mhi_ep_driver *mhi_drv = to_mhi_ep_driver(dev->driver); >>> + struct mhi_result result = {}; >>> + struct mhi_ep_chan *mhi_chan; >>> + int dir; >>> + >>> + /* Skip if it is a controller device */ >>> + if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER) >>> + return 0; >>> + >> >> It would be my preference to encapsulate the body of the >> following loop into a called function, then call that once >> for the UL channel and once for the DL channel. >> > > This follows the host stack, so I'd like to keep it the same. I think you should change both, but I'll leave that up to you. >>> + /* Disconnect the channels associated with the driver */ >>> + 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->lock); >>> + /* Send channel disconnect status to the client driver */ >>> + if (mhi_chan->xfer_cb) { >>> + result.transaction_status = -ENOTCONN; >>> + result.bytes_xferd = 0; >>> + mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); >> >> It appears the result is ignored here. If so, can we >> define the xfer_cb() function so that a NULL pointer may >> be supplied by the caller in cases like this? >> > > result is not ignored, only the bytes_xfered. "transaction_status" will > be used by the client drivers for error handling. Sorry, I was looking at the code *after* the call, and was ignoring that it was information being passed in... My mistake. >>> + } >>> + >>> + /* Set channel state to DISABLED */ >> >> That comment is a little tautological. Just omit it. >> >>> + mhi_chan->state = MHI_CH_STATE_DISABLED; >>> + mhi_chan->xfer_cb = NULL; >>> + mutex_unlock(&mhi_chan->lock); >>> + } >>> + >>> + /* Remove the client driver now */ >>> + mhi_drv->remove(mhi_dev); >>> + >>> + return 0; >>> +} > > [...] > >>> +struct mhi_ep_driver { >>> + const struct mhi_device_id *id_table; >>> + struct device_driver driver; >>> + int (*probe)(struct mhi_ep_device *mhi_ep, >>> + const struct mhi_device_id *id); >>> + void (*remove)(struct mhi_ep_device *mhi_ep); >> >> I get confused by the "ul" versus "dl" naming scheme here. >> Is "ul" from the perspective of the host, meaning upload >> is from the host toward the WWAN network (and therefore >> toward the SDX AP), and download is from the WWAN toward >> the host? Somewhere this should be stated clearly in >> comments; maybe I just missed it. >> > > Yes UL and DL are as per host context. I didn't state this explicitly > since this is the MHI host stack behaviour but I'll add a comment for > clarity Sounds good, thanks. -Alex > > Thanks, > Mani
diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c index b006011d025d..f66404181972 100644 --- a/drivers/bus/mhi/ep/main.c +++ b/drivers/bus/mhi/ep/main.c @@ -196,9 +196,89 @@ void mhi_ep_unregister_controller(struct mhi_ep_cntrl *mhi_cntrl) } EXPORT_SYMBOL_GPL(mhi_ep_unregister_controller); +static int mhi_ep_driver_probe(struct device *dev) +{ + struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev); + struct mhi_ep_driver *mhi_drv = to_mhi_ep_driver(dev->driver); + struct mhi_ep_chan *ul_chan = mhi_dev->ul_chan; + struct mhi_ep_chan *dl_chan = mhi_dev->dl_chan; + + /* Client drivers should have callbacks for both channels */ + if (!mhi_drv->ul_xfer_cb || !mhi_drv->dl_xfer_cb) + return -EINVAL; + + ul_chan->xfer_cb = mhi_drv->ul_xfer_cb; + dl_chan->xfer_cb = mhi_drv->dl_xfer_cb; + + return mhi_drv->probe(mhi_dev, mhi_dev->id); +} + +static int mhi_ep_driver_remove(struct device *dev) +{ + struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev); + struct mhi_ep_driver *mhi_drv = to_mhi_ep_driver(dev->driver); + struct mhi_result result = {}; + struct mhi_ep_chan *mhi_chan; + int dir; + + /* Skip if it is a controller device */ + if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER) + return 0; + + /* Disconnect the channels associated with the driver */ + 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->lock); + /* Send channel disconnect status to the client driver */ + if (mhi_chan->xfer_cb) { + result.transaction_status = -ENOTCONN; + result.bytes_xferd = 0; + mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); + } + + /* Set channel state to DISABLED */ + mhi_chan->state = MHI_CH_STATE_DISABLED; + mhi_chan->xfer_cb = NULL; + mutex_unlock(&mhi_chan->lock); + } + + /* Remove the client driver now */ + mhi_drv->remove(mhi_dev); + + return 0; +} + +int __mhi_ep_driver_register(struct mhi_ep_driver *mhi_drv, struct module *owner) +{ + struct device_driver *driver = &mhi_drv->driver; + + if (!mhi_drv->probe || !mhi_drv->remove) + return -EINVAL; + + driver->bus = &mhi_ep_bus_type; + driver->owner = owner; + driver->probe = mhi_ep_driver_probe; + driver->remove = mhi_ep_driver_remove; + + return driver_register(driver); +} +EXPORT_SYMBOL_GPL(__mhi_ep_driver_register); + +void mhi_ep_driver_unregister(struct mhi_ep_driver *mhi_drv) +{ + driver_unregister(&mhi_drv->driver); +} +EXPORT_SYMBOL_GPL(mhi_ep_driver_unregister); + static int mhi_ep_match(struct device *dev, struct device_driver *drv) { struct mhi_ep_device *mhi_dev = to_mhi_ep_device(dev); + struct mhi_ep_driver *mhi_drv = to_mhi_ep_driver(drv); + const struct mhi_device_id *id; /* * If the device is a controller type then there is no client driver @@ -207,6 +287,12 @@ static int mhi_ep_match(struct device *dev, struct device_driver *drv) if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER) return 0; + for (id = mhi_drv->id_table; id->chan[0]; id++) + if (!strcmp(mhi_dev->name, id->chan)) { + mhi_dev->id = id; + return 1; + } + return 0; }; diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h index 20238e9df1b3..da865f9d3646 100644 --- a/include/linux/mhi_ep.h +++ b/include/linux/mhi_ep.h @@ -122,7 +122,60 @@ struct mhi_ep_device { enum mhi_device_type dev_type; }; +/** + * struct mhi_ep_driver - Structure representing a MHI Endpoint client driver + * @id_table: Pointer to MHI Endpoint device ID table + * @driver: Device driver model driver + * @probe: CB function for client driver probe function + * @remove: CB function for client driver remove function + * @ul_xfer_cb: CB function for UL data transfer + * @dl_xfer_cb: CB function for DL data transfer + */ +struct mhi_ep_driver { + const struct mhi_device_id *id_table; + struct device_driver driver; + int (*probe)(struct mhi_ep_device *mhi_ep, + const struct mhi_device_id *id); + void (*remove)(struct mhi_ep_device *mhi_ep); + void (*ul_xfer_cb)(struct mhi_ep_device *mhi_dev, + struct mhi_result *result); + void (*dl_xfer_cb)(struct mhi_ep_device *mhi_dev, + struct mhi_result *result); +}; + #define to_mhi_ep_device(dev) container_of(dev, struct mhi_ep_device, dev) +#define to_mhi_ep_driver(drv) container_of(drv, struct mhi_ep_driver, driver) + +/* + * module_mhi_ep_driver() - Helper macro for drivers that don't do + * anything special other than using default mhi_ep_driver_register() and + * mhi_ep_driver_unregister(). This eliminates a lot of boilerplate. + * Each module may only use this macro once. + */ +#define module_mhi_ep_driver(mhi_drv) \ + module_driver(mhi_drv, mhi_ep_driver_register, \ + mhi_ep_driver_unregister) + +/* + * Macro to avoid include chaining to get THIS_MODULE + */ +#define mhi_ep_driver_register(mhi_drv) \ + __mhi_ep_driver_register(mhi_drv, THIS_MODULE) + +/** + * __mhi_ep_driver_register - Register a driver with MHI Endpoint bus + * @mhi_drv: Driver to be associated with the device + * @owner: The module owner + * + * Return: 0 if driver registrations succeeds, a negative error code otherwise. + */ +int __mhi_ep_driver_register(struct mhi_ep_driver *mhi_drv, struct module *owner); + +/** + * mhi_ep_driver_unregister - Unregister a driver from MHI Endpoint bus + * @mhi_drv: Driver associated with the device + */ +void mhi_ep_driver_unregister(struct mhi_ep_driver *mhi_drv); /** * mhi_ep_register_controller - Register MHI Endpoint controller
This commit adds support for registering MHI endpoint client drivers with the MHI endpoint stack. MHI endpoint client drivers binds to one or more MHI endpoint devices inorder to send and receive the upper-layer protocol packets like IP packets, modem control messages, and diagnostics messages over MHI bus. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/bus/mhi/ep/main.c | 86 +++++++++++++++++++++++++++++++++++++++ include/linux/mhi_ep.h | 53 ++++++++++++++++++++++++ 2 files changed, 139 insertions(+)