Hi Greg,
Here is the patchset for improving the MHI bus support. One of the patch
is suggested by you for adding the driver owner field and rest are additional
improvements and some fixes.
I've also included the remaining networking patches from previous patch series
which needs review from Dave. Dave could you please look into those 2 patches
which falls under net subsystem? Greg can take those 2 if an Ack is provided.
Thanks,
Mani
Changes in v3:
* Added Bjorn's Reviewed-by tag
* Fixed commit message for QCA6390
* Added extra comment for MHI revision fields in mhi.h
Changes in v2:
* Fixed some minor comments in mhi.h
Manivannan Sadhasivam (7):
bus: mhi: core: Pass module owner during client driver registration
bus: mhi: core: Add support for reading MHI info from device
bus: mhi: core: Initialize bhie field in mhi_cntrl for RDDM capture
bus: mhi: core: Drop the references to mhi_dev in mhi_destroy_device()
bus: mhi: core: Add support for MHI suspend and resume
net: qrtr: Add MHI transport layer
net: qrtr: Do not depend on ARCH_QCOM
drivers/bus/mhi/core/init.c | 39 +++++-
drivers/bus/mhi/core/internal.h | 10 ++
drivers/bus/mhi/core/main.c | 16 ++-
drivers/bus/mhi/core/pm.c | 143 ++++++++++++++++++++++
include/linux/mhi.h | 57 ++++++++-
net/qrtr/Kconfig | 8 +-
net/qrtr/Makefile | 2 +
net/qrtr/mhi.c | 208 ++++++++++++++++++++++++++++++++
8 files changed, 474 insertions(+), 9 deletions(-)
create mode 100644 net/qrtr/mhi.c
--
2.17.1
The MHI register base has several registers used for getting the MHI
specific information such as version, family, major, and minor numbers
from the device. This information can be used by the controller drivers
for usecases such as applying quirks for a specific revision etc...
While at it, let's also rearrange the local variables
in mhi_register_controller().
Suggested-by: Hemant Kumar <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/bus/mhi/core/init.c | 19 +++++++++++++++++--
drivers/bus/mhi/core/internal.h | 10 ++++++++++
include/linux/mhi.h | 17 +++++++++++++++++
3 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index eb7f556a8531..d136f6c6ca78 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -802,12 +802,12 @@ static int parse_config(struct mhi_controller *mhi_cntrl,
int mhi_register_controller(struct mhi_controller *mhi_cntrl,
struct mhi_controller_config *config)
{
- int ret;
- int i;
struct mhi_event *mhi_event;
struct mhi_chan *mhi_chan;
struct mhi_cmd *mhi_cmd;
struct mhi_device *mhi_dev;
+ u32 soc_info;
+ int ret, i;
if (!mhi_cntrl)
return -EINVAL;
@@ -874,6 +874,21 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
mhi_cntrl->unmap_single = mhi_unmap_single_no_bb;
}
+ /* Read the MHI device info */
+ ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs,
+ SOC_HW_VERSION_OFFS, &soc_info);
+ if (ret)
+ goto error_alloc_dev;
+
+ mhi_cntrl->family_number = (soc_info & SOC_HW_VERSION_FAM_NUM_BMSK) >>
+ SOC_HW_VERSION_FAM_NUM_SHFT;
+ mhi_cntrl->device_number = (soc_info & SOC_HW_VERSION_DEV_NUM_BMSK) >>
+ SOC_HW_VERSION_DEV_NUM_SHFT;
+ mhi_cntrl->major_version = (soc_info & SOC_HW_VERSION_MAJOR_VER_BMSK) >>
+ SOC_HW_VERSION_MAJOR_VER_SHFT;
+ mhi_cntrl->minor_version = (soc_info & SOC_HW_VERSION_MINOR_VER_BMSK) >>
+ SOC_HW_VERSION_MINOR_VER_SHFT;
+
/* Register controller with MHI bus */
mhi_dev = mhi_alloc_device(mhi_cntrl);
if (IS_ERR(mhi_dev)) {
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 18066302e6e2..5deadfaa053a 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -196,6 +196,16 @@ extern struct bus_type mhi_bus_type;
#define BHIE_RXVECSTATUS_STATUS_XFER_COMPL (0x02)
#define BHIE_RXVECSTATUS_STATUS_ERROR (0x03)
+#define SOC_HW_VERSION_OFFS (0x224)
+#define SOC_HW_VERSION_FAM_NUM_BMSK (0xF0000000)
+#define SOC_HW_VERSION_FAM_NUM_SHFT (28)
+#define SOC_HW_VERSION_DEV_NUM_BMSK (0x0FFF0000)
+#define SOC_HW_VERSION_DEV_NUM_SHFT (16)
+#define SOC_HW_VERSION_MAJOR_VER_BMSK (0x0000FF00)
+#define SOC_HW_VERSION_MAJOR_VER_SHFT (8)
+#define SOC_HW_VERSION_MINOR_VER_BMSK (0x000000FF)
+#define SOC_HW_VERSION_MINOR_VER_SHFT (0)
+
#define EV_CTX_RESERVED_MASK GENMASK(7, 0)
#define EV_CTX_INTMODC_MASK GENMASK(15, 8)
#define EV_CTX_INTMODC_SHIFT 8
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index d83e7772681b..ad1996001965 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -310,6 +310,10 @@ struct mhi_controller_config {
* @sw_ev_rings: Number of software event rings
* @nr_irqs_req: Number of IRQs required to operate (optional)
* @nr_irqs: Number of IRQ allocated by bus master (required)
+ * @family_number: MHI controller family number
+ * @device_number: MHI controller device number
+ * @major_version: MHI controller major revision number
+ * @minor_version: MHI controller minor revision number
* @mhi_event: MHI event ring configurations table
* @mhi_cmd: MHI command ring configurations table
* @mhi_ctxt: MHI device context, shared memory between host and device
@@ -348,6 +352,15 @@ struct mhi_controller_config {
* Fields marked as (required) need to be populated by the controller driver
* before calling mhi_register_controller(). For the fields marked as (optional)
* they can be populated depending on the usecase.
+ *
+ * The following fields are present for the purpose of implementing any device
+ * specific quirks or customizations for specific MHI revisions used in device
+ * by the controller drivers. The MHI stack will just populate these fields
+ * during mhi_register_controller():
+ * family_number
+ * device_number
+ * major_version
+ * minor_version
*/
struct mhi_controller {
struct device *cntrl_dev;
@@ -375,6 +388,10 @@ struct mhi_controller {
u32 sw_ev_rings;
u32 nr_irqs_req;
u32 nr_irqs;
+ u32 family_number;
+ u32 device_number;
+ u32 major_version;
+ u32 minor_version;
struct mhi_event *mhi_event;
struct mhi_cmd *mhi_cmd;
--
2.17.1
The bhie field in mhi_cntrl needs to be initialized to proper register
base in order to make mhi_rddm_prepare() to work. Otherwise,
mhi_rddm_prepare() will cause NULL pointer dereference.
Fixes: 6fdfdd27328c ("bus: mhi: core: Add support for downloading RDDM image during panic")
Reported-by: Hemant Kumar <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/bus/mhi/core/init.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index d136f6c6ca78..f6e3c16225a7 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -979,7 +979,8 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl)
goto bhie_error;
}
- memset_io(mhi_cntrl->regs + bhie_off + BHIE_RXVECADDR_LOW_OFFS,
+ mhi_cntrl->bhie = mhi_cntrl->regs + bhie_off;
+ memset_io(mhi_cntrl->bhie + BHIE_RXVECADDR_LOW_OFFS,
0, BHIE_RXVECSTATUS_OFFS - BHIE_RXVECADDR_LOW_OFFS +
4);
--
2.17.1
The module owner field can be used to prevent the removal of kernel
modules when there are any device files associated with it opened in
userspace. Hence, modify the API to pass module owner field. For
convenience, module_mhi_driver() macro is used which takes care of
passing the module owner through THIS_MODULE of the module of the
driver and also avoiding the use of specifying the default MHI client
driver register/unregister routines.
Suggested-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
drivers/bus/mhi/core/init.c | 5 +++--
include/linux/mhi.h | 21 +++++++++++++++++++--
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 5fb756ca335e..eb7f556a8531 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -1189,7 +1189,7 @@ static int mhi_driver_remove(struct device *dev)
return 0;
}
-int mhi_driver_register(struct mhi_driver *mhi_drv)
+int __mhi_driver_register(struct mhi_driver *mhi_drv, struct module *owner)
{
struct device_driver *driver = &mhi_drv->driver;
@@ -1197,12 +1197,13 @@ int mhi_driver_register(struct mhi_driver *mhi_drv)
return -EINVAL;
driver->bus = &mhi_bus_type;
+ driver->owner = owner;
driver->probe = mhi_driver_probe;
driver->remove = mhi_driver_remove;
return driver_register(driver);
}
-EXPORT_SYMBOL_GPL(mhi_driver_register);
+EXPORT_SYMBOL_GPL(__mhi_driver_register);
void mhi_driver_unregister(struct mhi_driver *mhi_drv)
{
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 79cb9f898544..d83e7772681b 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -514,11 +514,28 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
*/
void mhi_unregister_controller(struct mhi_controller *mhi_cntrl);
+/*
+ * module_mhi_driver() - Helper macro for drivers that don't do
+ * anything special other than using default mhi_driver_register() and
+ * mhi_driver_unregister(). This eliminates a lot of boilerplate.
+ * Each module may only use this macro once.
+ */
+#define module_mhi_driver(mhi_drv) \
+ module_driver(mhi_drv, mhi_driver_register, \
+ mhi_driver_unregister)
+
+/*
+ * Macro to avoid include chaining to get THIS_MODULE
+ */
+#define mhi_driver_register(mhi_drv) \
+ __mhi_driver_register(mhi_drv, THIS_MODULE)
+
/**
- * mhi_driver_register - Register driver with MHI framework
+ * __mhi_driver_register - Register driver with MHI framework
* @mhi_drv: Driver associated with the device
+ * @owner: The module owner
*/
-int mhi_driver_register(struct mhi_driver *mhi_drv);
+int __mhi_driver_register(struct mhi_driver *mhi_drv, struct module *owner);
/**
* mhi_driver_unregister - Unregister a driver for mhi_devices
--
2.17.1
For some scenarios like controller suspend and resume, mhi_destroy_device()
will get called without mhi_unregister_controller(). In that case, the
references to the mhi_dev created for the channels will not be dropped
but the channels will be destroyed as per the spec. This will cause issue
during resume as the channels will not be created due to the fact that
mhi_dev is not NULL.
Hence, this change decrements the refcount for mhi_dev in
mhi_destroy_device() for concerned channels and also sets mhi_dev to NULL
in release_device().
Reported-by: Carl Huang <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/bus/mhi/core/init.c | 12 ++++++++++++
drivers/bus/mhi/core/main.c | 13 +++++++++++++
2 files changed, 25 insertions(+)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index f6e3c16225a7..b38359c480ea 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -1028,6 +1028,18 @@ static void mhi_release_device(struct device *dev)
{
struct mhi_device *mhi_dev = to_mhi_device(dev);
+ /*
+ * We need to set the mhi_chan->mhi_dev to NULL here since the MHI
+ * devices for the channels will only get created if the mhi_dev
+ * associated with it is NULL. This scenario will happen during the
+ * controller suspend and resume.
+ */
+ if (mhi_dev->ul_chan)
+ mhi_dev->ul_chan->mhi_dev = NULL;
+
+ if (mhi_dev->dl_chan)
+ mhi_dev->dl_chan->mhi_dev = NULL;
+
kfree(mhi_dev);
}
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index fa1c9000fc6c..eb4256b81406 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -244,6 +244,19 @@ int mhi_destroy_device(struct device *dev, void *data)
if (mhi_dev->dev_type == MHI_DEVICE_CONTROLLER)
return 0;
+ /*
+ * For the suspend and resume case, this function will get called
+ * without mhi_unregister_controller(). Hence, we need to drop the
+ * references to mhi_dev created for ul and dl channels. We can
+ * be sure that there will be no instances of mhi_dev left after
+ * this.
+ */
+ if (mhi_dev->ul_chan)
+ put_device(&mhi_dev->ul_chan->mhi_dev->dev);
+
+ if (mhi_dev->dl_chan)
+ put_device(&mhi_dev->dl_chan->mhi_dev->dev);
+
dev_dbg(&mhi_cntrl->mhi_dev->dev, "destroy device for chan:%s\n",
mhi_dev->chan_name);
--
2.17.1
Add support for MHI suspend and resume states. While at it, the
mhi_notify() function needs to be exported as well.
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/bus/mhi/core/main.c | 3 +-
drivers/bus/mhi/core/pm.c | 143 ++++++++++++++++++++++++++++++++++++
include/linux/mhi.h | 19 +++++
3 files changed, 164 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index eb4256b81406..3e9aa3b2da77 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -267,7 +267,7 @@ int mhi_destroy_device(struct device *dev, void *data)
return 0;
}
-static void mhi_notify(struct mhi_device *mhi_dev, enum mhi_callback cb_reason)
+void mhi_notify(struct mhi_device *mhi_dev, enum mhi_callback cb_reason)
{
struct mhi_driver *mhi_drv;
@@ -279,6 +279,7 @@ static void mhi_notify(struct mhi_device *mhi_dev, enum mhi_callback cb_reason)
if (mhi_drv->status_cb)
mhi_drv->status_cb(mhi_dev, cb_reason);
}
+EXPORT_SYMBOL_GPL(mhi_notify);
/* Bind MHI channels to MHI devices */
void mhi_create_devices(struct mhi_controller *mhi_cntrl)
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 52690cb5c89c..3529419d076b 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -669,6 +669,149 @@ void mhi_pm_st_worker(struct work_struct *work)
}
}
+int mhi_pm_suspend(struct mhi_controller *mhi_cntrl)
+{
+ struct mhi_chan *itr, *tmp;
+ struct device *dev = &mhi_cntrl->mhi_dev->dev;
+ enum mhi_pm_state new_state;
+ int ret;
+
+ if (mhi_cntrl->pm_state == MHI_PM_DISABLE)
+ return -EINVAL;
+
+ if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state))
+ return -EIO;
+
+ /* Return busy if there are any pending resources */
+ if (atomic_read(&mhi_cntrl->dev_wake))
+ return -EBUSY;
+
+ /* Take MHI out of M2 state */
+ read_lock_bh(&mhi_cntrl->pm_lock);
+ mhi_cntrl->wake_get(mhi_cntrl, false);
+ read_unlock_bh(&mhi_cntrl->pm_lock);
+
+ ret = wait_event_timeout(mhi_cntrl->state_event,
+ mhi_cntrl->dev_state == MHI_STATE_M0 ||
+ mhi_cntrl->dev_state == MHI_STATE_M1 ||
+ MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
+ msecs_to_jiffies(mhi_cntrl->timeout_ms));
+
+ read_lock_bh(&mhi_cntrl->pm_lock);
+ mhi_cntrl->wake_put(mhi_cntrl, false);
+ read_unlock_bh(&mhi_cntrl->pm_lock);
+
+ if (!ret || MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
+ dev_err(dev,
+ "Could not enter M0/M1 state");
+ return -EIO;
+ }
+
+ write_lock_irq(&mhi_cntrl->pm_lock);
+
+ if (atomic_read(&mhi_cntrl->dev_wake)) {
+ write_unlock_irq(&mhi_cntrl->pm_lock);
+ return -EBUSY;
+ }
+
+ dev_info(dev, "Allowing M3 transition\n");
+ new_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_M3_ENTER);
+ if (new_state != MHI_PM_M3_ENTER) {
+ write_unlock_irq(&mhi_cntrl->pm_lock);
+ dev_err(dev,
+ "Error setting to PM state: %s from: %s\n",
+ to_mhi_pm_state_str(MHI_PM_M3_ENTER),
+ to_mhi_pm_state_str(mhi_cntrl->pm_state));
+ return -EIO;
+ }
+
+ /* Set MHI to M3 and wait for completion */
+ mhi_set_mhi_state(mhi_cntrl, MHI_STATE_M3);
+ write_unlock_irq(&mhi_cntrl->pm_lock);
+ dev_info(dev, "Wait for M3 completion\n");
+
+ ret = wait_event_timeout(mhi_cntrl->state_event,
+ mhi_cntrl->dev_state == MHI_STATE_M3 ||
+ MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
+ msecs_to_jiffies(mhi_cntrl->timeout_ms));
+
+ if (!ret || MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
+ dev_err(dev,
+ "Did not enter M3 state, MHI state: %s, PM state: %s\n",
+ TO_MHI_STATE_STR(mhi_cntrl->dev_state),
+ to_mhi_pm_state_str(mhi_cntrl->pm_state));
+ return -EIO;
+ }
+
+ /* Notify clients about entering LPM */
+ list_for_each_entry_safe(itr, tmp, &mhi_cntrl->lpm_chans, node) {
+ mutex_lock(&itr->mutex);
+ if (itr->mhi_dev)
+ mhi_notify(itr->mhi_dev, MHI_CB_LPM_ENTER);
+ mutex_unlock(&itr->mutex);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mhi_pm_suspend);
+
+int mhi_pm_resume(struct mhi_controller *mhi_cntrl)
+{
+ struct mhi_chan *itr, *tmp;
+ struct device *dev = &mhi_cntrl->mhi_dev->dev;
+ enum mhi_pm_state cur_state;
+ int ret;
+
+ dev_info(dev, "Entered with PM state: %s, MHI state: %s\n",
+ to_mhi_pm_state_str(mhi_cntrl->pm_state),
+ TO_MHI_STATE_STR(mhi_cntrl->dev_state));
+
+ if (mhi_cntrl->pm_state == MHI_PM_DISABLE)
+ return 0;
+
+ if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state))
+ return -EIO;
+
+ /* Notify clients about exiting LPM */
+ list_for_each_entry_safe(itr, tmp, &mhi_cntrl->lpm_chans, node) {
+ mutex_lock(&itr->mutex);
+ if (itr->mhi_dev)
+ mhi_notify(itr->mhi_dev, MHI_CB_LPM_EXIT);
+ mutex_unlock(&itr->mutex);
+ }
+
+ write_lock_irq(&mhi_cntrl->pm_lock);
+ cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_M3_EXIT);
+ if (cur_state != MHI_PM_M3_EXIT) {
+ write_unlock_irq(&mhi_cntrl->pm_lock);
+ dev_info(dev,
+ "Error setting to PM state: %s from: %s\n",
+ to_mhi_pm_state_str(MHI_PM_M3_EXIT),
+ to_mhi_pm_state_str(mhi_cntrl->pm_state));
+ return -EIO;
+ }
+
+ /* Set MHI to M0 and wait for completion */
+ mhi_set_mhi_state(mhi_cntrl, MHI_STATE_M0);
+ write_unlock_irq(&mhi_cntrl->pm_lock);
+
+ ret = wait_event_timeout(mhi_cntrl->state_event,
+ mhi_cntrl->dev_state == MHI_STATE_M0 ||
+ MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
+ msecs_to_jiffies(mhi_cntrl->timeout_ms));
+
+ if (!ret || MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
+ dev_err(dev,
+ "Did not enter M0 state, MHI state: %s, PM state: %s\n",
+ TO_MHI_STATE_STR(mhi_cntrl->dev_state),
+ to_mhi_pm_state_str(mhi_cntrl->pm_state));
+ return -EIO;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(mhi_pm_resume);
+
int __mhi_device_get_sync(struct mhi_controller *mhi_cntrl)
{
int ret;
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index ad1996001965..a4288f4d656f 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -568,6 +568,13 @@ void mhi_driver_unregister(struct mhi_driver *mhi_drv);
void mhi_set_mhi_state(struct mhi_controller *mhi_cntrl,
enum mhi_state state);
+/**
+ * mhi_notify - Notify the MHI client driver about client device status
+ * @mhi_dev: MHI device instance
+ * @cb_reason: MHI callback reason
+ */
+void mhi_notify(struct mhi_device *mhi_dev, enum mhi_callback cb_reason);
+
/**
* mhi_prepare_for_power_up - Do pre-initialization before power up.
* This is optional, call this before power up if
@@ -604,6 +611,18 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful);
*/
void mhi_unprepare_after_power_down(struct mhi_controller *mhi_cntrl);
+/**
+ * mhi_pm_suspend - Move MHI into a suspended state
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_pm_suspend(struct mhi_controller *mhi_cntrl);
+
+/**
+ * mhi_pm_resume - Resume MHI from suspended state
+ * @mhi_cntrl: MHI controller
+ */
+int mhi_pm_resume(struct mhi_controller *mhi_cntrl);
+
/**
* mhi_download_rddm_img - Download ramdump image from device for
* debugging purpose.
--
2.17.1
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" <[email protected]>
Cc: [email protected]
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
net/qrtr/Kconfig | 7 ++
net/qrtr/Makefile | 2 +
net/qrtr/mhi.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 217 insertions(+)
create mode 100644 net/qrtr/mhi.c
diff --git a/net/qrtr/Kconfig b/net/qrtr/Kconfig
index 63f89cc6e82c..8eb876471564 100644
--- a/net/qrtr/Kconfig
+++ b/net/qrtr/Kconfig
@@ -29,4 +29,11 @@ config QRTR_TUN
implement endpoints of QRTR, for purpose of tunneling data to other
hosts or testing purposes.
+config QRTR_MHI
+ tristate "MHI IPC Router channels"
+ depends on MHI_BUS
+ help
+ Say Y here to support MHI based ipcrouter channels. MHI is the
+ transport used for communicating to external modems.
+
endif # QRTR
diff --git a/net/qrtr/Makefile b/net/qrtr/Makefile
index 1c6d6c120fb7..3dc0a7c9d455 100644
--- a/net/qrtr/Makefile
+++ b/net/qrtr/Makefile
@@ -5,3 +5,5 @@ obj-$(CONFIG_QRTR_SMD) += qrtr-smd.o
qrtr-smd-y := smd.o
obj-$(CONFIG_QRTR_TUN) += qrtr-tun.o
qrtr-tun-y := tun.o
+obj-$(CONFIG_QRTR_MHI) += qrtr-mhi.o
+qrtr-mhi-y := mhi.o
diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
new file mode 100644
index 000000000000..90af208f34c1
--- /dev/null
+++ b/net/qrtr/mhi.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/mhi.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/skbuff.h>
+#include <net/sock.h>
+
+#include "qrtr.h"
+
+struct qrtr_mhi_dev {
+ struct qrtr_endpoint ep;
+ struct mhi_device *mhi_dev;
+ struct device *dev;
+ spinlock_t ul_lock; /* lock to protect ul_pkts */
+ struct list_head ul_pkts;
+ atomic_t in_reset;
+};
+
+struct qrtr_mhi_pkt {
+ struct list_head node;
+ struct sk_buff *skb;
+ struct kref refcount;
+ struct completion done;
+};
+
+static void qrtr_mhi_pkt_release(struct kref *ref)
+{
+ struct qrtr_mhi_pkt *pkt = container_of(ref, struct qrtr_mhi_pkt,
+ refcount);
+ struct sock *sk = pkt->skb->sk;
+
+ consume_skb(pkt->skb);
+ if (sk)
+ sock_put(sk);
+
+ kfree(pkt);
+}
+
+/* From MHI to QRTR */
+static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev,
+ struct mhi_result *mhi_res)
+{
+ struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
+ int rc;
+
+ if (!qdev || mhi_res->transaction_status)
+ return;
+
+ rc = qrtr_endpoint_post(&qdev->ep, mhi_res->buf_addr,
+ mhi_res->bytes_xferd);
+ if (rc == -EINVAL)
+ dev_err(qdev->dev, "invalid ipcrouter packet\n");
+}
+
+/* From QRTR to MHI */
+static void qcom_mhi_qrtr_ul_callback(struct mhi_device *mhi_dev,
+ struct mhi_result *mhi_res)
+{
+ struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
+ struct qrtr_mhi_pkt *pkt;
+ unsigned long flags;
+
+ spin_lock_irqsave(&qdev->ul_lock, flags);
+ pkt = list_first_entry(&qdev->ul_pkts, struct qrtr_mhi_pkt, node);
+ list_del(&pkt->node);
+ complete_all(&pkt->done);
+
+ kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
+ spin_unlock_irqrestore(&qdev->ul_lock, flags);
+}
+
+static void qcom_mhi_qrtr_status_callback(struct mhi_device *mhi_dev,
+ enum mhi_callback mhi_cb)
+{
+ struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
+ struct qrtr_mhi_pkt *pkt;
+ unsigned long flags;
+
+ if (mhi_cb != MHI_CB_FATAL_ERROR)
+ return;
+
+ atomic_inc(&qdev->in_reset);
+ spin_lock_irqsave(&qdev->ul_lock, flags);
+ list_for_each_entry(pkt, &qdev->ul_pkts, node)
+ complete_all(&pkt->done);
+ spin_unlock_irqrestore(&qdev->ul_lock, flags);
+}
+
+/* Send data over MHI */
+static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
+{
+ struct qrtr_mhi_dev *qdev = container_of(ep, struct qrtr_mhi_dev, ep);
+ struct qrtr_mhi_pkt *pkt;
+ int rc;
+
+ rc = skb_linearize(skb);
+ if (rc) {
+ kfree_skb(skb);
+ return rc;
+ }
+
+ pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
+ if (!pkt) {
+ kfree_skb(skb);
+ return -ENOMEM;
+ }
+
+ init_completion(&pkt->done);
+ kref_init(&pkt->refcount);
+ kref_get(&pkt->refcount);
+ pkt->skb = skb;
+
+ spin_lock_bh(&qdev->ul_lock);
+ list_add_tail(&pkt->node, &qdev->ul_pkts);
+ rc = mhi_queue_skb(qdev->mhi_dev, DMA_TO_DEVICE, skb, skb->len,
+ MHI_EOT);
+ if (rc) {
+ list_del(&pkt->node);
+ /* Reference count needs to be dropped 2 times */
+ kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
+ kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
+ kfree_skb(skb);
+ spin_unlock_bh(&qdev->ul_lock);
+ return rc;
+ }
+
+ spin_unlock_bh(&qdev->ul_lock);
+ if (skb->sk)
+ sock_hold(skb->sk);
+
+ rc = wait_for_completion_interruptible_timeout(&pkt->done, HZ * 5);
+ if (atomic_read(&qdev->in_reset))
+ rc = -ECONNRESET;
+ else if (rc == 0)
+ rc = -ETIMEDOUT;
+ else if (rc > 0)
+ rc = 0;
+
+ kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
+
+ return rc;
+}
+
+static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
+ const struct mhi_device_id *id)
+{
+ struct qrtr_mhi_dev *qdev;
+ u32 net_id;
+ int rc;
+
+ qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL);
+ if (!qdev)
+ return -ENOMEM;
+
+ qdev->mhi_dev = mhi_dev;
+ qdev->dev = &mhi_dev->dev;
+ qdev->ep.xmit = qcom_mhi_qrtr_send;
+ atomic_set(&qdev->in_reset, 0);
+
+ net_id = QRTR_EP_NID_AUTO;
+
+ INIT_LIST_HEAD(&qdev->ul_pkts);
+ spin_lock_init(&qdev->ul_lock);
+
+ dev_set_drvdata(&mhi_dev->dev, qdev);
+ rc = qrtr_endpoint_register(&qdev->ep, net_id);
+ if (rc)
+ return rc;
+
+ dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n");
+
+ return 0;
+}
+
+static void qcom_mhi_qrtr_remove(struct mhi_device *mhi_dev)
+{
+ struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
+
+ qrtr_endpoint_unregister(&qdev->ep);
+ dev_set_drvdata(&mhi_dev->dev, NULL);
+}
+
+static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = {
+ { .chan = "IPCR" },
+ {}
+};
+MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table);
+
+static struct mhi_driver qcom_mhi_qrtr_driver = {
+ .probe = qcom_mhi_qrtr_probe,
+ .remove = qcom_mhi_qrtr_remove,
+ .dl_xfer_cb = qcom_mhi_qrtr_dl_callback,
+ .ul_xfer_cb = qcom_mhi_qrtr_ul_callback,
+ .status_cb = qcom_mhi_qrtr_status_callback,
+ .id_table = qcom_mhi_qrtr_id_table,
+ .driver = {
+ .name = "qcom_mhi_qrtr",
+ },
+};
+
+module_mhi_driver(qcom_mhi_qrtr_driver);
+
+MODULE_DESCRIPTION("Qualcomm IPC-Router MHI interface driver");
+MODULE_LICENSE("GPL v2");
--
2.17.1
IPC Router protocol is also used by external modems for exchanging the QMI
messages. Hence, it doesn't always depend on Qualcomm platforms. One such
instance is the QCA6390 WLAN device connected to x86 machine.
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
net/qrtr/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/qrtr/Kconfig b/net/qrtr/Kconfig
index 8eb876471564..f362ca316015 100644
--- a/net/qrtr/Kconfig
+++ b/net/qrtr/Kconfig
@@ -4,7 +4,6 @@
config QRTR
tristate "Qualcomm IPC Router support"
- depends on ARCH_QCOM || COMPILE_TEST
---help---
Say Y if you intend to use Qualcomm IPC router protocol. The
protocol is used to communicate with services provided by other
--
2.17.1
On 3/24/2020 12:10 AM, Manivannan Sadhasivam wrote:
> The MHI register base has several registers used for getting the MHI
> specific information such as version, family, major, and minor numbers
> from the device. This information can be used by the controller drivers
> for usecases such as applying quirks for a specific revision etc...
>
> While at it, let's also rearrange the local variables
> in mhi_register_controller().
>
> Suggested-by: Hemant Kumar <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> drivers/bus/mhi/core/init.c | 19 +++++++++++++++++--
> drivers/bus/mhi/core/internal.h | 10 ++++++++++
> include/linux/mhi.h | 17 +++++++++++++++++
> 3 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index eb7f556a8531..d136f6c6ca78 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -802,12 +802,12 @@ static int parse_config(struct mhi_controller *mhi_cntrl,
> int mhi_register_controller(struct mhi_controller *mhi_cntrl,
> struct mhi_controller_config *config)
> {
> - int ret;
> - int i;
> struct mhi_event *mhi_event;
> struct mhi_chan *mhi_chan;
> struct mhi_cmd *mhi_cmd;
> struct mhi_device *mhi_dev;
> + u32 soc_info;
> + int ret, i;
>
> if (!mhi_cntrl)
> return -EINVAL;
> @@ -874,6 +874,21 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
> mhi_cntrl->unmap_single = mhi_unmap_single_no_bb;
> }
>
> + /* Read the MHI device info */
> + ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs,
> + SOC_HW_VERSION_OFFS, &soc_info);
> + if (ret)
> + goto error_alloc_dev;
> +
> + mhi_cntrl->family_number = (soc_info & SOC_HW_VERSION_FAM_NUM_BMSK) >>
> + SOC_HW_VERSION_FAM_NUM_SHFT;
> + mhi_cntrl->device_number = (soc_info & SOC_HW_VERSION_DEV_NUM_BMSK) >>
> + SOC_HW_VERSION_DEV_NUM_SHFT;
> + mhi_cntrl->major_version = (soc_info & SOC_HW_VERSION_MAJOR_VER_BMSK) >>
> + SOC_HW_VERSION_MAJOR_VER_SHFT;
> + mhi_cntrl->minor_version = (soc_info & SOC_HW_VERSION_MINOR_VER_BMSK) >>
> + SOC_HW_VERSION_MINOR_VER_SHFT;
> +
> /* Register controller with MHI bus */
> mhi_dev = mhi_alloc_device(mhi_cntrl);
> if (IS_ERR(mhi_dev)) {
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 18066302e6e2..5deadfaa053a 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -196,6 +196,16 @@ extern struct bus_type mhi_bus_type;
> #define BHIE_RXVECSTATUS_STATUS_XFER_COMPL (0x02)
> #define BHIE_RXVECSTATUS_STATUS_ERROR (0x03)
>
> +#define SOC_HW_VERSION_OFFS (0x224)
> +#define SOC_HW_VERSION_FAM_NUM_BMSK (0xF0000000)
> +#define SOC_HW_VERSION_FAM_NUM_SHFT (28)
> +#define SOC_HW_VERSION_DEV_NUM_BMSK (0x0FFF0000)
> +#define SOC_HW_VERSION_DEV_NUM_SHFT (16)
> +#define SOC_HW_VERSION_MAJOR_VER_BMSK (0x0000FF00)
> +#define SOC_HW_VERSION_MAJOR_VER_SHFT (8)
> +#define SOC_HW_VERSION_MINOR_VER_BMSK (0x000000FF)
> +#define SOC_HW_VERSION_MINOR_VER_SHFT (0)
I'm tempted to give reviewed-by, however it occurs to me that I don't
see this in the MHI spec. I'm looking at Rev E, which as far as I am
aware is the latest.
Hemant, is this in the spec, and if so, what Rev?
I'm concerned that if its not in the spec, we may have an issue with
some device not implementing this as expected.
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On Mon 23 Mar 23:10 PDT 2020, 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" <[email protected]>
> Cc: [email protected]
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> net/qrtr/Kconfig | 7 ++
> net/qrtr/Makefile | 2 +
> net/qrtr/mhi.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 217 insertions(+)
> create mode 100644 net/qrtr/mhi.c
>
> diff --git a/net/qrtr/Kconfig b/net/qrtr/Kconfig
> index 63f89cc6e82c..8eb876471564 100644
> --- a/net/qrtr/Kconfig
> +++ b/net/qrtr/Kconfig
> @@ -29,4 +29,11 @@ config QRTR_TUN
> implement endpoints of QRTR, for purpose of tunneling data to other
> hosts or testing purposes.
>
> +config QRTR_MHI
> + tristate "MHI IPC Router channels"
> + depends on MHI_BUS
> + help
> + Say Y here to support MHI based ipcrouter channels. MHI is the
> + transport used for communicating to external modems.
> +
> endif # QRTR
> diff --git a/net/qrtr/Makefile b/net/qrtr/Makefile
> index 1c6d6c120fb7..3dc0a7c9d455 100644
> --- a/net/qrtr/Makefile
> +++ b/net/qrtr/Makefile
> @@ -5,3 +5,5 @@ obj-$(CONFIG_QRTR_SMD) += qrtr-smd.o
> qrtr-smd-y := smd.o
> obj-$(CONFIG_QRTR_TUN) += qrtr-tun.o
> qrtr-tun-y := tun.o
> +obj-$(CONFIG_QRTR_MHI) += qrtr-mhi.o
> +qrtr-mhi-y := mhi.o
> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
> new file mode 100644
> index 000000000000..90af208f34c1
> --- /dev/null
> +++ b/net/qrtr/mhi.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/mhi.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/skbuff.h>
> +#include <net/sock.h>
> +
> +#include "qrtr.h"
> +
> +struct qrtr_mhi_dev {
> + struct qrtr_endpoint ep;
> + struct mhi_device *mhi_dev;
> + struct device *dev;
> + spinlock_t ul_lock; /* lock to protect ul_pkts */
> + struct list_head ul_pkts;
> + atomic_t in_reset;
> +};
> +
> +struct qrtr_mhi_pkt {
> + struct list_head node;
> + struct sk_buff *skb;
> + struct kref refcount;
> + struct completion done;
> +};
> +
> +static void qrtr_mhi_pkt_release(struct kref *ref)
> +{
> + struct qrtr_mhi_pkt *pkt = container_of(ref, struct qrtr_mhi_pkt,
> + refcount);
> + struct sock *sk = pkt->skb->sk;
> +
> + consume_skb(pkt->skb);
> + if (sk)
> + sock_put(sk);
> +
> + kfree(pkt);
> +}
> +
> +/* From MHI to QRTR */
> +static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev,
> + struct mhi_result *mhi_res)
> +{
> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> + int rc;
> +
> + if (!qdev || mhi_res->transaction_status)
> + return;
> +
> + rc = qrtr_endpoint_post(&qdev->ep, mhi_res->buf_addr,
> + mhi_res->bytes_xferd);
> + if (rc == -EINVAL)
> + dev_err(qdev->dev, "invalid ipcrouter packet\n");
Perhaps this should be a debug print, perhaps rate limited. But either
way it's relevant for any transport, so I think you should skip it here
- and potentially move it into qrtr_endpoint_post() in some form.
> +}
> +
> +/* From QRTR to MHI */
> +static void qcom_mhi_qrtr_ul_callback(struct mhi_device *mhi_dev,
> + struct mhi_result *mhi_res)
> +{
> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> + struct qrtr_mhi_pkt *pkt;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&qdev->ul_lock, flags);
> + pkt = list_first_entry(&qdev->ul_pkts, struct qrtr_mhi_pkt, node);
> + list_del(&pkt->node);
> + complete_all(&pkt->done);
You should be able to release the lock after popping the item off the
list, then complete and refcount it.
> +
> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> + spin_unlock_irqrestore(&qdev->ul_lock, flags);
> +}
> +
> +static void qcom_mhi_qrtr_status_callback(struct mhi_device *mhi_dev,
> + enum mhi_callback mhi_cb)
> +{
> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> + struct qrtr_mhi_pkt *pkt;
> + unsigned long flags;
> +
> + if (mhi_cb != MHI_CB_FATAL_ERROR)
> + return;
> +
> + atomic_inc(&qdev->in_reset);
You have ul_lock close at hand in both places where you access in_reset,
so I think it would be better to just use that, instead of an atomic.
> + spin_lock_irqsave(&qdev->ul_lock, flags);
> + list_for_each_entry(pkt, &qdev->ul_pkts, node)
> + complete_all(&pkt->done);
> + spin_unlock_irqrestore(&qdev->ul_lock, flags);
> +}
> +
> +/* Send data over MHI */
> +static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
> +{
> + struct qrtr_mhi_dev *qdev = container_of(ep, struct qrtr_mhi_dev, ep);
> + struct qrtr_mhi_pkt *pkt;
> + int rc;
> +
> + rc = skb_linearize(skb);
> + if (rc) {
> + kfree_skb(skb);
> + return rc;
> + }
> +
> + pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
> + if (!pkt) {
> + kfree_skb(skb);
> + return -ENOMEM;
> + }
> +
> + init_completion(&pkt->done);
> + kref_init(&pkt->refcount);
> + kref_get(&pkt->refcount);
> + pkt->skb = skb;
> +
> + spin_lock_bh(&qdev->ul_lock);
> + list_add_tail(&pkt->node, &qdev->ul_pkts);
> + rc = mhi_queue_skb(qdev->mhi_dev, DMA_TO_DEVICE, skb, skb->len,
> + MHI_EOT);
Do you want to continue doing this when qdev->in_reset? Wouldn't it be
better to bail early if the remote end is dying?
> + if (rc) {
> + list_del(&pkt->node);
> + /* Reference count needs to be dropped 2 times */
> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> + kfree_skb(skb);
> + spin_unlock_bh(&qdev->ul_lock);
> + return rc;
> + }
> +
> + spin_unlock_bh(&qdev->ul_lock);
> + if (skb->sk)
> + sock_hold(skb->sk);
> +
> + rc = wait_for_completion_interruptible_timeout(&pkt->done, HZ * 5);
> + if (atomic_read(&qdev->in_reset))
> + rc = -ECONNRESET;
> + else if (rc == 0)
> + rc = -ETIMEDOUT;
Is this recoverable? The message will remain on the list and may be
delivered at a later point(?), but qrtr and the app will learn that the
message was lost - which is presumably considered fatal.
Is it guaranteed that qcom_mhi_qrtr_ul_callback() will happen later and
find the head of the list?
The reason for my question is that without this you have one of two
scenarios;
1) the message is put on the list, decremented in
qcom_mhi_qrtr_ul_callback() then we get back here and decrement it
again.
2) the message is put on the list, then qcom_mhi_qrtr_status_callback()
happens and all messages are released - presumably then
qcom_mhi_qrtr_ul_callback() won't happen.
So if the third case (where we return here and then later
qcom_mhi_qrtr_ul_callback() must find this particular packet at the
front of the queue) can't happen, then you can just skip the entire
refcounting.
Further more, you could carry qrtr_mhi_pkt on the stack.
...or to flip this around, is there a reason to wait here at all? What
would happen if you just return immediately after calling
mhi_queue_skb()? Wouldn't that provide you better throughput?
> + else if (rc > 0)
> + rc = 0;
> +
> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> +
> + return rc;
> +}
> +
> +static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
> + const struct mhi_device_id *id)
> +{
> + struct qrtr_mhi_dev *qdev;
> + u32 net_id;
> + int rc;
> +
> + qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL);
> + if (!qdev)
> + return -ENOMEM;
> +
> + qdev->mhi_dev = mhi_dev;
> + qdev->dev = &mhi_dev->dev;
> + qdev->ep.xmit = qcom_mhi_qrtr_send;
> + atomic_set(&qdev->in_reset, 0);
> +
> + net_id = QRTR_EP_NID_AUTO;
Just pass QRTR_EP_NID_AUTO directly in the function call below.
Regards,
Bjorn
> +
> + INIT_LIST_HEAD(&qdev->ul_pkts);
> + spin_lock_init(&qdev->ul_lock);
> +
> + dev_set_drvdata(&mhi_dev->dev, qdev);
> + rc = qrtr_endpoint_register(&qdev->ep, net_id);
> + if (rc)
> + return rc;
> +
> + dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n");
> +
> + return 0;
> +}
> +
> +static void qcom_mhi_qrtr_remove(struct mhi_device *mhi_dev)
> +{
> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> +
> + qrtr_endpoint_unregister(&qdev->ep);
> + dev_set_drvdata(&mhi_dev->dev, NULL);
> +}
> +
> +static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = {
> + { .chan = "IPCR" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table);
> +
> +static struct mhi_driver qcom_mhi_qrtr_driver = {
> + .probe = qcom_mhi_qrtr_probe,
> + .remove = qcom_mhi_qrtr_remove,
> + .dl_xfer_cb = qcom_mhi_qrtr_dl_callback,
> + .ul_xfer_cb = qcom_mhi_qrtr_ul_callback,
> + .status_cb = qcom_mhi_qrtr_status_callback,
> + .id_table = qcom_mhi_qrtr_id_table,
> + .driver = {
> + .name = "qcom_mhi_qrtr",
> + },
> +};
> +
> +module_mhi_driver(qcom_mhi_qrtr_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm IPC-Router MHI interface driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>
On Mon 23 Mar 23:10 PDT 2020, Manivannan Sadhasivam wrote:
> IPC Router protocol is also used by external modems for exchanging the QMI
> messages. Hence, it doesn't always depend on Qualcomm platforms. One such
> instance is the QCA6390 WLAN device connected to x86 machine.
>
> Cc: "David S. Miller" <[email protected]>
> Cc: [email protected]
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
> ---
> net/qrtr/Kconfig | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/net/qrtr/Kconfig b/net/qrtr/Kconfig
> index 8eb876471564..f362ca316015 100644
> --- a/net/qrtr/Kconfig
> +++ b/net/qrtr/Kconfig
> @@ -4,7 +4,6 @@
>
> config QRTR
> tristate "Qualcomm IPC Router support"
> - depends on ARCH_QCOM || COMPILE_TEST
> ---help---
> Say Y if you intend to use Qualcomm IPC router protocol. The
> protocol is used to communicate with services provided by other
> --
> 2.17.1
>
Hi Bjorn,
+ Chris Lew
On Tue, Mar 24, 2020 at 01:39:52PM -0700, Bjorn Andersson wrote:
> On Mon 23 Mar 23:10 PDT 2020, 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" <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > ---
> > net/qrtr/Kconfig | 7 ++
> > net/qrtr/Makefile | 2 +
> > net/qrtr/mhi.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 217 insertions(+)
> > create mode 100644 net/qrtr/mhi.c
> >
> > diff --git a/net/qrtr/Kconfig b/net/qrtr/Kconfig
> > index 63f89cc6e82c..8eb876471564 100644
> > --- a/net/qrtr/Kconfig
> > +++ b/net/qrtr/Kconfig
> > @@ -29,4 +29,11 @@ config QRTR_TUN
> > implement endpoints of QRTR, for purpose of tunneling data to other
> > hosts or testing purposes.
> >
> > +config QRTR_MHI
> > + tristate "MHI IPC Router channels"
> > + depends on MHI_BUS
> > + help
> > + Say Y here to support MHI based ipcrouter channels. MHI is the
> > + transport used for communicating to external modems.
> > +
> > endif # QRTR
> > diff --git a/net/qrtr/Makefile b/net/qrtr/Makefile
> > index 1c6d6c120fb7..3dc0a7c9d455 100644
> > --- a/net/qrtr/Makefile
> > +++ b/net/qrtr/Makefile
> > @@ -5,3 +5,5 @@ obj-$(CONFIG_QRTR_SMD) += qrtr-smd.o
> > qrtr-smd-y := smd.o
> > obj-$(CONFIG_QRTR_TUN) += qrtr-tun.o
> > qrtr-tun-y := tun.o
> > +obj-$(CONFIG_QRTR_MHI) += qrtr-mhi.o
> > +qrtr-mhi-y := mhi.o
> > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
> > new file mode 100644
> > index 000000000000..90af208f34c1
> > --- /dev/null
> > +++ b/net/qrtr/mhi.c
> > @@ -0,0 +1,208 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include <linux/mhi.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/skbuff.h>
> > +#include <net/sock.h>
> > +
> > +#include "qrtr.h"
> > +
> > +struct qrtr_mhi_dev {
> > + struct qrtr_endpoint ep;
> > + struct mhi_device *mhi_dev;
> > + struct device *dev;
> > + spinlock_t ul_lock; /* lock to protect ul_pkts */
> > + struct list_head ul_pkts;
> > + atomic_t in_reset;
> > +};
> > +
> > +struct qrtr_mhi_pkt {
> > + struct list_head node;
> > + struct sk_buff *skb;
> > + struct kref refcount;
> > + struct completion done;
> > +};
> > +
> > +static void qrtr_mhi_pkt_release(struct kref *ref)
> > +{
> > + struct qrtr_mhi_pkt *pkt = container_of(ref, struct qrtr_mhi_pkt,
> > + refcount);
> > + struct sock *sk = pkt->skb->sk;
> > +
> > + consume_skb(pkt->skb);
> > + if (sk)
> > + sock_put(sk);
> > +
> > + kfree(pkt);
> > +}
> > +
> > +/* From MHI to QRTR */
> > +static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev,
> > + struct mhi_result *mhi_res)
> > +{
> > + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> > + int rc;
> > +
> > + if (!qdev || mhi_res->transaction_status)
> > + return;
> > +
> > + rc = qrtr_endpoint_post(&qdev->ep, mhi_res->buf_addr,
> > + mhi_res->bytes_xferd);
> > + if (rc == -EINVAL)
> > + dev_err(qdev->dev, "invalid ipcrouter packet\n");
>
> Perhaps this should be a debug print, perhaps rate limited. But either
> way it's relevant for any transport, so I think you should skip it here
> - and potentially move it into qrtr_endpoint_post() in some form.
>
I agree with moving this to qrtr_endpoint_post() but I don't think it is a
good idea to make it as a debug print. It is an error log and should stay
as it is. Only in this MHI transport driver, the return value is not getting
used but in others it is.
> > +}
> > +
> > +/* From QRTR to MHI */
> > +static void qcom_mhi_qrtr_ul_callback(struct mhi_device *mhi_dev,
> > + struct mhi_result *mhi_res)
> > +{
> > + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> > + struct qrtr_mhi_pkt *pkt;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&qdev->ul_lock, flags);
> > + pkt = list_first_entry(&qdev->ul_pkts, struct qrtr_mhi_pkt, node);
> > + list_del(&pkt->node);
> > + complete_all(&pkt->done);
>
> You should be able to release the lock after popping the item off the
> list, then complete and refcount it.
>
Okay.
> > +
> > + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> > + spin_unlock_irqrestore(&qdev->ul_lock, flags);
> > +}
> > +
> > +static void qcom_mhi_qrtr_status_callback(struct mhi_device *mhi_dev,
> > + enum mhi_callback mhi_cb)
> > +{
> > + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> > + struct qrtr_mhi_pkt *pkt;
> > + unsigned long flags;
> > +
> > + if (mhi_cb != MHI_CB_FATAL_ERROR)
> > + return;
> > +
> > + atomic_inc(&qdev->in_reset);
>
> You have ul_lock close at hand in both places where you access in_reset,
> so I think it would be better to just use that, instead of an atomic.
>
Okay.
> > + spin_lock_irqsave(&qdev->ul_lock, flags);
> > + list_for_each_entry(pkt, &qdev->ul_pkts, node)
> > + complete_all(&pkt->done);
Chris, shouldn't we require list_del(&pkt->node) here?
> > + spin_unlock_irqrestore(&qdev->ul_lock, flags);
> > +}
> > +
> > +/* Send data over MHI */
> > +static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
> > +{
> > + struct qrtr_mhi_dev *qdev = container_of(ep, struct qrtr_mhi_dev, ep);
> > + struct qrtr_mhi_pkt *pkt;
> > + int rc;
> > +
> > + rc = skb_linearize(skb);
> > + if (rc) {
> > + kfree_skb(skb);
> > + return rc;
> > + }
> > +
> > + pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
> > + if (!pkt) {
> > + kfree_skb(skb);
> > + return -ENOMEM;
> > + }
> > +
> > + init_completion(&pkt->done);
> > + kref_init(&pkt->refcount);
> > + kref_get(&pkt->refcount);
> > + pkt->skb = skb;
> > +
> > + spin_lock_bh(&qdev->ul_lock);
> > + list_add_tail(&pkt->node, &qdev->ul_pkts);
> > + rc = mhi_queue_skb(qdev->mhi_dev, DMA_TO_DEVICE, skb, skb->len,
> > + MHI_EOT);
>
> Do you want to continue doing this when qdev->in_reset? Wouldn't it be
> better to bail early if the remote end is dying?
>
Now I'm thinking why we are not decrementing in_reset anywhere! Incase of
SYS_ERR, the status_cb will get processed and in_reset will be set but
it will stay so even when after MHI gets reset.
Chris, can you please clarify?
> > + if (rc) {
> > + list_del(&pkt->node);
> > + /* Reference count needs to be dropped 2 times */
> > + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> > + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> > + kfree_skb(skb);
> > + spin_unlock_bh(&qdev->ul_lock);
> > + return rc;
> > + }
> > +
> > + spin_unlock_bh(&qdev->ul_lock);
> > + if (skb->sk)
> > + sock_hold(skb->sk);
> > +
> > + rc = wait_for_completion_interruptible_timeout(&pkt->done, HZ * 5);
> > + if (atomic_read(&qdev->in_reset))
> > + rc = -ECONNRESET;
> > + else if (rc == 0)
> > + rc = -ETIMEDOUT;
>
> Is this recoverable? The message will remain on the list and may be
> delivered at a later point(?), but qrtr and the app will learn that the
> message was lost - which is presumably considered fatal.
>
> Is it guaranteed that qcom_mhi_qrtr_ul_callback() will happen later and
> find the head of the list?
>
There are 2 scenarios:
1. If the completion interrupt happens after timeout, ul_callback()
will be called. But it will only try to fetch the current head of ul_pkts.
In most cases, we can hope that the completion interrupt will happen before
next queue_skb().
2. If we don't get completion interrupt, timeout will happen and at the final
stage (during mhi_driver_remove()), MHI stack will go over the pending TREs
for all channels in queue and call ul_callback() with -ENOTCONN. But in the
callback, we don't have any idea of the pkt which was not successfully
transferred to the device and currently just fetching first entry.
Now I'm seeing some issue here which I missed earlier. If the completion
interrupt never happens then the corresponding pkt will never get freed and
therefore we have a leak. Eventhough the ul_callback() will get called during
mhi_driver_remove() for pending TREs, we don't exactly fetch the right pkt.
Chris, our assumption of the ul_callback() gets called irrespective of
transfer status is wrong. I think this code needs a bit of rework.
>
> The reason for my question is that without this you have one of two
> scenarios;
> 1) the message is put on the list, decremented in
> qcom_mhi_qrtr_ul_callback() then we get back here and decrement it
> again.
> 2) the message is put on the list, then qcom_mhi_qrtr_status_callback()
> happens and all messages are released - presumably then
> qcom_mhi_qrtr_ul_callback() won't happen.
>
>
> So if the third case (where we return here and then later
> qcom_mhi_qrtr_ul_callback() must find this particular packet at the
> front of the queue) can't happen, then you can just skip the entire
> refcounting.
>
> Further more, you could carry qrtr_mhi_pkt on the stack.
>
>
> ...or to flip this around, is there a reason to wait here at all? What
> would happen if you just return immediately after calling
> mhi_queue_skb()? Wouldn't that provide you better throughput?
>
Chris would be best person to answer this question.
> > + else if (rc > 0)
> > + rc = 0;
> > +
> > + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> > +
> > + return rc;
> > +}
> > +
> > +static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
> > + const struct mhi_device_id *id)
> > +{
> > + struct qrtr_mhi_dev *qdev;
> > + u32 net_id;
> > + int rc;
> > +
> > + qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL);
> > + if (!qdev)
> > + return -ENOMEM;
> > +
> > + qdev->mhi_dev = mhi_dev;
> > + qdev->dev = &mhi_dev->dev;
> > + qdev->ep.xmit = qcom_mhi_qrtr_send;
> > + atomic_set(&qdev->in_reset, 0);
> > +
> > + net_id = QRTR_EP_NID_AUTO;
>
> Just pass QRTR_EP_NID_AUTO directly in the function call below.
>
Okay.
Thanks,
Mani
> Regards,
> Bjorn
>
> > +
> > + INIT_LIST_HEAD(&qdev->ul_pkts);
> > + spin_lock_init(&qdev->ul_lock);
> > +
> > + dev_set_drvdata(&mhi_dev->dev, qdev);
> > + rc = qrtr_endpoint_register(&qdev->ep, net_id);
> > + if (rc)
> > + return rc;
> > +
> > + dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n");
> > +
> > + return 0;
> > +}
> > +
> > +static void qcom_mhi_qrtr_remove(struct mhi_device *mhi_dev)
> > +{
> > + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> > +
> > + qrtr_endpoint_unregister(&qdev->ep);
> > + dev_set_drvdata(&mhi_dev->dev, NULL);
> > +}
> > +
> > +static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = {
> > + { .chan = "IPCR" },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table);
> > +
> > +static struct mhi_driver qcom_mhi_qrtr_driver = {
> > + .probe = qcom_mhi_qrtr_probe,
> > + .remove = qcom_mhi_qrtr_remove,
> > + .dl_xfer_cb = qcom_mhi_qrtr_dl_callback,
> > + .ul_xfer_cb = qcom_mhi_qrtr_ul_callback,
> > + .status_cb = qcom_mhi_qrtr_status_callback,
> > + .id_table = qcom_mhi_qrtr_id_table,
> > + .driver = {
> > + .name = "qcom_mhi_qrtr",
> > + },
> > +};
> > +
> > +module_mhi_driver(qcom_mhi_qrtr_driver);
> > +
> > +MODULE_DESCRIPTION("Qualcomm IPC-Router MHI interface driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.1
> >
On 3/24/2020 12:10 AM, Manivannan Sadhasivam wrote:
> The bhie field in mhi_cntrl needs to be initialized to proper register
> base in order to make mhi_rddm_prepare() to work. Otherwise,
> mhi_rddm_prepare() will cause NULL pointer dereference.
>
> Fixes: 6fdfdd27328c ("bus: mhi: core: Add support for downloading RDDM image during panic")
> Reported-by: Hemant Kumar <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Jeffrey Hugo <[email protected]>
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On 3/24/2020 12:10 AM, Manivannan Sadhasivam wrote:
> For some scenarios like controller suspend and resume, mhi_destroy_device()
> will get called without mhi_unregister_controller(). In that case, the
> references to the mhi_dev created for the channels will not be dropped
> but the channels will be destroyed as per the spec. This will cause issue
> during resume as the channels will not be created due to the fact that
> mhi_dev is not NULL.
>
> Hence, this change decrements the refcount for mhi_dev in
> mhi_destroy_device() for concerned channels and also sets mhi_dev to NULL
> in release_device().
>
> Reported-by: Carl Huang <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Jeffrey Hugo <[email protected]>
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On 3/24/2020 8:38 AM, Jeffrey Hugo wrote:
> On 3/24/2020 12:10 AM, Manivannan Sadhasivam wrote:
>> The MHI register base has several registers used for getting the MHI
>> specific information such as version, family, major, and minor numbers
>> from the device. This information can be used by the controller drivers
>> for usecases such as applying quirks for a specific revision etc...
>>
>> While at it, let's also rearrange the local variables
>> in mhi_register_controller().
>>
>> Suggested-by: Hemant Kumar <[email protected]>
>> Signed-off-by: Manivannan Sadhasivam <[email protected]>
>> ---
>> drivers/bus/mhi/core/init.c | 19 +++++++++++++++++--
>> drivers/bus/mhi/core/internal.h | 10 ++++++++++
>> include/linux/mhi.h | 17 +++++++++++++++++
>> 3 files changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> index eb7f556a8531..d136f6c6ca78 100644
>> --- a/drivers/bus/mhi/core/init.c
>> +++ b/drivers/bus/mhi/core/init.c
>> @@ -802,12 +802,12 @@ static int parse_config(struct mhi_controller
>> *mhi_cntrl,
>> int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>> struct mhi_controller_config *config)
>> {
>> - int ret;
>> - int i;
>> struct mhi_event *mhi_event;
>> struct mhi_chan *mhi_chan;
>> struct mhi_cmd *mhi_cmd;
>> struct mhi_device *mhi_dev;
>> + u32 soc_info;
>> + int ret, i;
>> if (!mhi_cntrl)
>> return -EINVAL;
>> @@ -874,6 +874,21 @@ int mhi_register_controller(struct mhi_controller
>> *mhi_cntrl,
>> mhi_cntrl->unmap_single = mhi_unmap_single_no_bb;
>> }
>> + /* Read the MHI device info */
>> + ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs,
>> + SOC_HW_VERSION_OFFS, &soc_info);
>> + if (ret)
>> + goto error_alloc_dev;
>> +
>> + mhi_cntrl->family_number = (soc_info &
>> SOC_HW_VERSION_FAM_NUM_BMSK) >>
>> + SOC_HW_VERSION_FAM_NUM_SHFT;
>> + mhi_cntrl->device_number = (soc_info &
>> SOC_HW_VERSION_DEV_NUM_BMSK) >>
>> + SOC_HW_VERSION_DEV_NUM_SHFT;
>> + mhi_cntrl->major_version = (soc_info &
>> SOC_HW_VERSION_MAJOR_VER_BMSK) >>
>> + SOC_HW_VERSION_MAJOR_VER_SHFT;
>> + mhi_cntrl->minor_version = (soc_info &
>> SOC_HW_VERSION_MINOR_VER_BMSK) >>
>> + SOC_HW_VERSION_MINOR_VER_SHFT;
>> +
>> /* Register controller with MHI bus */
>> mhi_dev = mhi_alloc_device(mhi_cntrl);
>> if (IS_ERR(mhi_dev)) {
>> diff --git a/drivers/bus/mhi/core/internal.h
>> b/drivers/bus/mhi/core/internal.h
>> index 18066302e6e2..5deadfaa053a 100644
>> --- a/drivers/bus/mhi/core/internal.h
>> +++ b/drivers/bus/mhi/core/internal.h
>> @@ -196,6 +196,16 @@ extern struct bus_type mhi_bus_type;
>> #define BHIE_RXVECSTATUS_STATUS_XFER_COMPL (0x02)
>> #define BHIE_RXVECSTATUS_STATUS_ERROR (0x03)
>> +#define SOC_HW_VERSION_OFFS (0x224)
>> +#define SOC_HW_VERSION_FAM_NUM_BMSK (0xF0000000)
>> +#define SOC_HW_VERSION_FAM_NUM_SHFT (28)
>> +#define SOC_HW_VERSION_DEV_NUM_BMSK (0x0FFF0000)
>> +#define SOC_HW_VERSION_DEV_NUM_SHFT (16)
>> +#define SOC_HW_VERSION_MAJOR_VER_BMSK (0x0000FF00)
>> +#define SOC_HW_VERSION_MAJOR_VER_SHFT (8)
>> +#define SOC_HW_VERSION_MINOR_VER_BMSK (0x000000FF)
>> +#define SOC_HW_VERSION_MINOR_VER_SHFT (0)
>
> I'm tempted to give reviewed-by, however it occurs to me that I don't
> see this in the MHI spec. I'm looking at Rev E, which as far as I am
> aware is the latest.
>
> Hemant, is this in the spec, and if so, what Rev?
>
> I'm concerned that if its not in the spec, we may have an issue with
> some device not implementing this as expected.
>
Reviewed-by: Jeffrey Hugo <[email protected]>
I talked with Hemant offline, and this is queued up for the next spec
rev. I'm satisfied with that.
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On Tue, Mar 24, 2020 at 11:40:43AM +0530, Manivannan Sadhasivam wrote:
> Hi Greg,
>
> Here is the patchset for improving the MHI bus support. One of the patch
> is suggested by you for adding the driver owner field and rest are additional
> improvements and some fixes.
I've taken the first 4 of these now, thanks.
greg k-h
On Thu, Mar 26, 2020 at 03:51:44PM +0100, Greg KH wrote:
> On Tue, Mar 24, 2020 at 11:40:43AM +0530, Manivannan Sadhasivam wrote:
> > Hi Greg,
> >
> > Here is the patchset for improving the MHI bus support. One of the patch
> > is suggested by you for adding the driver owner field and rest are additional
> > improvements and some fixes.
>
> I've taken the first 4 of these now, thanks.
>
Thanks Greg! For the future patches after v5.7, how do you want to pick them?
I assume that you'll be the person picking all "bus" related patches, then
do you want me to CC you for all patches or just send them as a pull request
finally?
Thanks,
Mani
> greg k-h
On Thu, Mar 26, 2020 at 10:55:14PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Mar 26, 2020 at 03:51:44PM +0100, Greg KH wrote:
> > On Tue, Mar 24, 2020 at 11:40:43AM +0530, Manivannan Sadhasivam wrote:
> > > Hi Greg,
> > >
> > > Here is the patchset for improving the MHI bus support. One of the patch
> > > is suggested by you for adding the driver owner field and rest are additional
> > > improvements and some fixes.
> >
> > I've taken the first 4 of these now, thanks.
> >
>
> Thanks Greg! For the future patches after v5.7, how do you want to pick them?
> I assume that you'll be the person picking all "bus" related patches, then
> do you want me to CC you for all patches or just send them as a pull request
> finally?
Sending me patch series like this is good to start with for now. If it
gets too complex and too big, then we can worry about pull requests.
thanks,
greg k-h
On 3/25/2020 3:37 AM, Manivannan Sadhasivam wrote:
> Hi Bjorn,
>
> + Chris Lew
>
> On Tue, Mar 24, 2020 at 01:39:52PM -0700, Bjorn Andersson wrote:
>> On Mon 23 Mar 23:10 PDT 2020, 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" <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: Manivannan Sadhasivam <[email protected]>
>>> ---
>>> net/qrtr/Kconfig | 7 ++
>>> net/qrtr/Makefile | 2 +
>>> net/qrtr/mhi.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 217 insertions(+)
>>> create mode 100644 net/qrtr/mhi.c
>>>
>>> diff --git a/net/qrtr/Kconfig b/net/qrtr/Kconfig
>>> index 63f89cc6e82c..8eb876471564 100644
>>> --- a/net/qrtr/Kconfig
>>> +++ b/net/qrtr/Kconfig
>>> @@ -29,4 +29,11 @@ config QRTR_TUN
>>> implement endpoints of QRTR, for purpose of tunneling data to other
>>> hosts or testing purposes.
>>>
>>> +config QRTR_MHI
>>> + tristate "MHI IPC Router channels"
>>> + depends on MHI_BUS
>>> + help
>>> + Say Y here to support MHI based ipcrouter channels. MHI is the
>>> + transport used for communicating to external modems.
>>> +
>>> endif # QRTR
>>> diff --git a/net/qrtr/Makefile b/net/qrtr/Makefile
>>> index 1c6d6c120fb7..3dc0a7c9d455 100644
>>> --- a/net/qrtr/Makefile
>>> +++ b/net/qrtr/Makefile
>>> @@ -5,3 +5,5 @@ obj-$(CONFIG_QRTR_SMD) += qrtr-smd.o
>>> qrtr-smd-y := smd.o
>>> obj-$(CONFIG_QRTR_TUN) += qrtr-tun.o
>>> qrtr-tun-y := tun.o
>>> +obj-$(CONFIG_QRTR_MHI) += qrtr-mhi.o
>>> +qrtr-mhi-y := mhi.o
>>> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
>>> new file mode 100644
>>> index 000000000000..90af208f34c1
>>> --- /dev/null
>>> +++ b/net/qrtr/mhi.c
>>> @@ -0,0 +1,208 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
>>> + */
>>> +
>>> +#include <linux/mhi.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/module.h>
>>> +#include <linux/skbuff.h>
>>> +#include <net/sock.h>
>>> +
>>> +#include "qrtr.h"
>>> +
>>> +struct qrtr_mhi_dev {
>>> + struct qrtr_endpoint ep;
>>> + struct mhi_device *mhi_dev;
>>> + struct device *dev;
>>> + spinlock_t ul_lock; /* lock to protect ul_pkts */
>>> + struct list_head ul_pkts;
>>> + atomic_t in_reset;
>>> +};
>>> +
>>> +struct qrtr_mhi_pkt {
>>> + struct list_head node;
>>> + struct sk_buff *skb;
>>> + struct kref refcount;
>>> + struct completion done;
>>> +};
>>> +
>>> +static void qrtr_mhi_pkt_release(struct kref *ref)
>>> +{
>>> + struct qrtr_mhi_pkt *pkt = container_of(ref, struct qrtr_mhi_pkt,
>>> + refcount);
>>> + struct sock *sk = pkt->skb->sk;
>>> +
>>> + consume_skb(pkt->skb);
>>> + if (sk)
>>> + sock_put(sk);
>>> +
>>> + kfree(pkt);
>>> +}
>>> +
>>> +/* From MHI to QRTR */
>>> +static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev,
>>> + struct mhi_result *mhi_res)
>>> +{
>>> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
>>> + int rc;
>>> +
>>> + if (!qdev || mhi_res->transaction_status)
>>> + return;
>>> +
>>> + rc = qrtr_endpoint_post(&qdev->ep, mhi_res->buf_addr,
>>> + mhi_res->bytes_xferd);
>>> + if (rc == -EINVAL)
>>> + dev_err(qdev->dev, "invalid ipcrouter packet\n");
>>
>> Perhaps this should be a debug print, perhaps rate limited. But either
>> way it's relevant for any transport, so I think you should skip it here
>> - and potentially move it into qrtr_endpoint_post() in some form.
>>
>
> I agree with moving this to qrtr_endpoint_post() but I don't think it is a
> good idea to make it as a debug print. It is an error log and should stay
> as it is. Only in this MHI transport driver, the return value is not getting
> used but in others it is.
>
This print has been useful for catching transport errors. Only issue I
see with moving this to qrtr_endpoint_post() is that there are multiple
points of failure in qrtr_endpoint_post() and logging it here makes it
easier.
>>> +}
>>> +
>>> +/* From QRTR to MHI */
>>> +static void qcom_mhi_qrtr_ul_callback(struct mhi_device *mhi_dev,
>>> + struct mhi_result *mhi_res)
>>> +{
>>> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
>>> + struct qrtr_mhi_pkt *pkt;
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&qdev->ul_lock, flags);
>>> + pkt = list_first_entry(&qdev->ul_pkts, struct qrtr_mhi_pkt, node);
>>> + list_del(&pkt->node);
>>> + complete_all(&pkt->done);
>>
>> You should be able to release the lock after popping the item off the
>> list, then complete and refcount it.
>>
>
> Okay.
>
>>> +
>>> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
>>> + spin_unlock_irqrestore(&qdev->ul_lock, flags);
>>> +}
>>> +
>>> +static void qcom_mhi_qrtr_status_callback(struct mhi_device *mhi_dev,
>>> + enum mhi_callback mhi_cb)
>>> +{
>>> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
>>> + struct qrtr_mhi_pkt *pkt;
>>> + unsigned long flags;
>>> +
>>> + if (mhi_cb != MHI_CB_FATAL_ERROR)
>>> + return;
>>> +
>>> + atomic_inc(&qdev->in_reset);
>>
>> You have ul_lock close at hand in both places where you access in_reset,
>> so I think it would be better to just use that, instead of an atomic.
>>
>
> Okay.
>
Does this version of MHI give the MHI_CB_FATAL_ERROR as a status
callback? We added this as an early notifier workaround. The in_reset
code probably isn't required with the current feature set.
>>> + spin_lock_irqsave(&qdev->ul_lock, flags);
>>> + list_for_each_entry(pkt, &qdev->ul_pkts, node)
>>> + complete_all(&pkt->done);
>
> Chris, shouldn't we require list_del(&pkt->node) here?
>
No this isn't a full cleanup, with the "early notifier" we just
unblocked any threads waiting for the ul_callback. Those threads will
wake, check in_reset, return an error back to the caller. Any list
cleanup will be done in the ul_callbacks that the mhi bus will do for
each queued packet right before device remove.
Again to simplify the code, we can probable remove the in_reset handling
since it's not required with the current feature set.
>>> + spin_unlock_irqrestore(&qdev->ul_lock, flags);
>>> +}
>>> +
>>> +/* Send data over MHI */
>>> +static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
>>> +{
>>> + struct qrtr_mhi_dev *qdev = container_of(ep, struct qrtr_mhi_dev, ep);
>>> + struct qrtr_mhi_pkt *pkt;
>>> + int rc;
>>> +
>>> + rc = skb_linearize(skb);
>>> + if (rc) {
>>> + kfree_skb(skb);
>>> + return rc;
>>> + }
>>> +
>>> + pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
>>> + if (!pkt) {
>>> + kfree_skb(skb);
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + init_completion(&pkt->done);
>>> + kref_init(&pkt->refcount);
>>> + kref_get(&pkt->refcount);
>>> + pkt->skb = skb;
>>> +
>>> + spin_lock_bh(&qdev->ul_lock);
>>> + list_add_tail(&pkt->node, &qdev->ul_pkts);
>>> + rc = mhi_queue_skb(qdev->mhi_dev, DMA_TO_DEVICE, skb, skb->len,
>>> + MHI_EOT);
>>
>> Do you want to continue doing this when qdev->in_reset? Wouldn't it be
>> better to bail early if the remote end is dying?
>>
>
> Now I'm thinking why we are not decrementing in_reset anywhere! Incase of
> SYS_ERR, the status_cb will get processed and in_reset will be set but
> it will stay so even when after MHI gets reset.
>
> Chris, can you please clarify?
>
Early notify of reset, if we do get SYS_ERR, we expect remove to be
called *soon*.
>>> + if (rc) {
>>> + list_del(&pkt->node);
>>> + /* Reference count needs to be dropped 2 times */
>>> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
>>> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
>>> + kfree_skb(skb);
>>> + spin_unlock_bh(&qdev->ul_lock);
>>> + return rc;
>>> + }
>>> +
>>> + spin_unlock_bh(&qdev->ul_lock);
>>> + if (skb->sk)
>>> + sock_hold(skb->sk);
>>> +
>>> + rc = wait_for_completion_interruptible_timeout(&pkt->done, HZ * 5);
>>> + if (atomic_read(&qdev->in_reset))
>>> + rc = -ECONNRESET;
>>> + else if (rc == 0)
>>> + rc = -ETIMEDOUT;
>>
>> Is this recoverable? The message will remain on the list and may be
>> delivered at a later point(?), but qrtr and the app will learn that the
>> message was lost - which is presumably considered fatal.
>>
>> Is it guaranteed that qcom_mhi_qrtr_ul_callback() will happen later and
>> find the head of the list?
>>
>
> There are 2 scenarios:
>
> 1. If the completion interrupt happens after timeout, ul_callback()
> will be called. But it will only try to fetch the current head of ul_pkts.
> In most cases, we can hope that the completion interrupt will happen before
> next queue_skb().
>
> 2. If we don't get completion interrupt, timeout will happen and at the final
> stage (during mhi_driver_remove()), MHI stack will go over the pending TREs
> for all channels in queue and call ul_callback() with -ENOTCONN. But in the
> callback, we don't have any idea of the pkt which was not successfully
> transferred to the device and currently just fetching first entry.
>
> Now I'm seeing some issue here which I missed earlier. If the completion
> interrupt never happens then the corresponding pkt will never get freed and
> therefore we have a leak. Eventhough the ul_callback() will get called during
> mhi_driver_remove() for pending TREs, we don't exactly fetch the right pkt.
>
> Chris, our assumption of the ul_callback() gets called irrespective of
> transfer status is wrong. I think this code needs a bit of rework.
>
>>
>> The reason for my question is that without this you have one of two
>> scenarios;
>> 1) the message is put on the list, decremented in
>> qcom_mhi_qrtr_ul_callback() then we get back here and decrement it
>> again.
>> 2) the message is put on the list, then qcom_mhi_qrtr_status_callback()
>> happens and all messages are released - presumably then
>> qcom_mhi_qrtr_ul_callback() won't happen.
>>
>>
>> So if the third case (where we return here and then later
>> qcom_mhi_qrtr_ul_callback() must find this particular packet at the
>> front of the queue) can't happen, then you can just skip the entire
>> refcounting.
>>
>> Further more, you could carry qrtr_mhi_pkt on the stack.
>>
>>
>> ...or to flip this around, is there a reason to wait here at all? What
>> would happen if you just return immediately after calling
>> mhi_queue_skb()? Wouldn't that provide you better throughput?
>>
>
> Chris would be best person to answer this question.
>
Yea - after working with MHI for a bit now, I think we can definitely
return after queueing the skb.
The IPC-Router Protocol, wasn't really designed with the ability to
recover from dropped packets since SMD/GLINK are "Lossless". I figured
it would be better for the client to definitely know that the packet
reached the other side which is why it blocks until ul callback.
I thought having the client get an error on timeout and resend the
packet would be better than silently dropping it. In practice, we've
really only seen the timeout or ul_callback errors on unrecoverable
errors so I think the timeout handling can definitely be redone.
Thanks,
Chris
>>> + else if (rc > 0)
>>> + rc = 0;
>>> +
>>> + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
>>> +
>>> + return rc;
>>> +}
>>> +
>>> +static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
>>> + const struct mhi_device_id *id)
>>> +{
>>> + struct qrtr_mhi_dev *qdev;
>>> + u32 net_id;
>>> + int rc;
>>> +
>>> + qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL);
>>> + if (!qdev)
>>> + return -ENOMEM;
>>> +
>>> + qdev->mhi_dev = mhi_dev;
>>> + qdev->dev = &mhi_dev->dev;
>>> + qdev->ep.xmit = qcom_mhi_qrtr_send;
>>> + atomic_set(&qdev->in_reset, 0);
>>> +
>>> + net_id = QRTR_EP_NID_AUTO;
>>
>> Just pass QRTR_EP_NID_AUTO directly in the function call below.
>>
>
> Okay.
>
> Thanks,
> Mani
>
>> Regards,
>> Bjorn
>>
>>> +
>>> + INIT_LIST_HEAD(&qdev->ul_pkts);
>>> + spin_lock_init(&qdev->ul_lock);
>>> +
>>> + dev_set_drvdata(&mhi_dev->dev, qdev);
>>> + rc = qrtr_endpoint_register(&qdev->ep, net_id);
>>> + if (rc)
>>> + return rc;
>>> +
>>> + dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n");
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void qcom_mhi_qrtr_remove(struct mhi_device *mhi_dev)
>>> +{
>>> + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
>>> +
>>> + qrtr_endpoint_unregister(&qdev->ep);
>>> + dev_set_drvdata(&mhi_dev->dev, NULL);
>>> +}
>>> +
>>> +static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = {
>>> + { .chan = "IPCR" },
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table);
>>> +
>>> +static struct mhi_driver qcom_mhi_qrtr_driver = {
>>> + .probe = qcom_mhi_qrtr_probe,
>>> + .remove = qcom_mhi_qrtr_remove,
>>> + .dl_xfer_cb = qcom_mhi_qrtr_dl_callback,
>>> + .ul_xfer_cb = qcom_mhi_qrtr_ul_callback,
>>> + .status_cb = qcom_mhi_qrtr_status_callback,
>>> + .id_table = qcom_mhi_qrtr_id_table,
>>> + .driver = {
>>> + .name = "qcom_mhi_qrtr",
>>> + },
>>> +};
>>> +
>>> +module_mhi_driver(qcom_mhi_qrtr_driver);
>>> +
>>> +MODULE_DESCRIPTION("Qualcomm IPC-Router MHI interface driver");
>>> +MODULE_LICENSE("GPL v2");
>>> --
>>> 2.17.1
>>>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
Hi Chris,
On Thu, Mar 26, 2020 at 03:54:42PM -0700, Chris Lew wrote:
>
>
> On 3/25/2020 3:37 AM, Manivannan Sadhasivam wrote:
> > Hi Bjorn,
> >
> > + Chris Lew
> >
> > On Tue, Mar 24, 2020 at 01:39:52PM -0700, Bjorn Andersson wrote:
> > > On Mon 23 Mar 23:10 PDT 2020, 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" <[email protected]>
> > > > Cc: [email protected]
> > > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > > > ---
> > > > net/qrtr/Kconfig | 7 ++
> > > > net/qrtr/Makefile | 2 +
> > > > net/qrtr/mhi.c | 208 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 217 insertions(+)
> > > > create mode 100644 net/qrtr/mhi.c
> > > >
> > > > diff --git a/net/qrtr/Kconfig b/net/qrtr/Kconfig
> > > > index 63f89cc6e82c..8eb876471564 100644
> > > > --- a/net/qrtr/Kconfig
> > > > +++ b/net/qrtr/Kconfig
> > > > @@ -29,4 +29,11 @@ config QRTR_TUN
> > > > implement endpoints of QRTR, for purpose of tunneling data to other
> > > > hosts or testing purposes.
> > > > +config QRTR_MHI
> > > > + tristate "MHI IPC Router channels"
> > > > + depends on MHI_BUS
> > > > + help
> > > > + Say Y here to support MHI based ipcrouter channels. MHI is the
> > > > + transport used for communicating to external modems.
> > > > +
> > > > endif # QRTR
> > > > diff --git a/net/qrtr/Makefile b/net/qrtr/Makefile
> > > > index 1c6d6c120fb7..3dc0a7c9d455 100644
> > > > --- a/net/qrtr/Makefile
> > > > +++ b/net/qrtr/Makefile
> > > > @@ -5,3 +5,5 @@ obj-$(CONFIG_QRTR_SMD) += qrtr-smd.o
> > > > qrtr-smd-y := smd.o
> > > > obj-$(CONFIG_QRTR_TUN) += qrtr-tun.o
> > > > qrtr-tun-y := tun.o
> > > > +obj-$(CONFIG_QRTR_MHI) += qrtr-mhi.o
> > > > +qrtr-mhi-y := mhi.o
> > > > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
> > > > new file mode 100644
> > > > index 000000000000..90af208f34c1
> > > > --- /dev/null
> > > > +++ b/net/qrtr/mhi.c
> > > > @@ -0,0 +1,208 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.
> > > > + */
> > > > +
> > > > +#include <linux/mhi.h>
> > > > +#include <linux/mod_devicetable.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/skbuff.h>
> > > > +#include <net/sock.h>
> > > > +
> > > > +#include "qrtr.h"
> > > > +
> > > > +struct qrtr_mhi_dev {
> > > > + struct qrtr_endpoint ep;
> > > > + struct mhi_device *mhi_dev;
> > > > + struct device *dev;
> > > > + spinlock_t ul_lock; /* lock to protect ul_pkts */
> > > > + struct list_head ul_pkts;
> > > > + atomic_t in_reset;
> > > > +};
> > > > +
> > > > +struct qrtr_mhi_pkt {
> > > > + struct list_head node;
> > > > + struct sk_buff *skb;
> > > > + struct kref refcount;
> > > > + struct completion done;
> > > > +};
> > > > +
> > > > +static void qrtr_mhi_pkt_release(struct kref *ref)
> > > > +{
> > > > + struct qrtr_mhi_pkt *pkt = container_of(ref, struct qrtr_mhi_pkt,
> > > > + refcount);
> > > > + struct sock *sk = pkt->skb->sk;
> > > > +
> > > > + consume_skb(pkt->skb);
> > > > + if (sk)
> > > > + sock_put(sk);
> > > > +
> > > > + kfree(pkt);
> > > > +}
> > > > +
> > > > +/* From MHI to QRTR */
> > > > +static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev,
> > > > + struct mhi_result *mhi_res)
> > > > +{
> > > > + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> > > > + int rc;
> > > > +
> > > > + if (!qdev || mhi_res->transaction_status)
> > > > + return;
> > > > +
> > > > + rc = qrtr_endpoint_post(&qdev->ep, mhi_res->buf_addr,
> > > > + mhi_res->bytes_xferd);
> > > > + if (rc == -EINVAL)
> > > > + dev_err(qdev->dev, "invalid ipcrouter packet\n");
> > >
> > > Perhaps this should be a debug print, perhaps rate limited. But either
> > > way it's relevant for any transport, so I think you should skip it here
> > > - and potentially move it into qrtr_endpoint_post() in some form.
> > >
> >
> > I agree with moving this to qrtr_endpoint_post() but I don't think it is a
> > good idea to make it as a debug print. It is an error log and should stay
> > as it is. Only in this MHI transport driver, the return value is not getting
> > used but in others it is.
> >
>
> This print has been useful for catching transport errors. Only issue I see
> with moving this to qrtr_endpoint_post() is that there are multiple points
> of failure in qrtr_endpoint_post() and logging it here makes it easier.
>
> > > > +}
> > > > +
> > > > +/* From QRTR to MHI */
> > > > +static void qcom_mhi_qrtr_ul_callback(struct mhi_device *mhi_dev,
> > > > + struct mhi_result *mhi_res)
> > > > +{
> > > > + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> > > > + struct qrtr_mhi_pkt *pkt;
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&qdev->ul_lock, flags);
> > > > + pkt = list_first_entry(&qdev->ul_pkts, struct qrtr_mhi_pkt, node);
> > > > + list_del(&pkt->node);
> > > > + complete_all(&pkt->done);
> > >
> > > You should be able to release the lock after popping the item off the
> > > list, then complete and refcount it.
> > >
> >
> > Okay.
> >
> > > > +
> > > > + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> > > > + spin_unlock_irqrestore(&qdev->ul_lock, flags);
> > > > +}
> > > > +
> > > > +static void qcom_mhi_qrtr_status_callback(struct mhi_device *mhi_dev,
> > > > + enum mhi_callback mhi_cb)
> > > > +{
> > > > + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> > > > + struct qrtr_mhi_pkt *pkt;
> > > > + unsigned long flags;
> > > > +
> > > > + if (mhi_cb != MHI_CB_FATAL_ERROR)
> > > > + return;
> > > > +
> > > > + atomic_inc(&qdev->in_reset);
> > >
> > > You have ul_lock close at hand in both places where you access in_reset,
> > > so I think it would be better to just use that, instead of an atomic.
> > >
> >
> > Okay.
> >
>
> Does this version of MHI give the MHI_CB_FATAL_ERROR as a status callback?
> We added this as an early notifier workaround. The in_reset code probably
> isn't required with the current feature set.
>
Err, NO. I got confused with mhi_cntrl->status_cb which gives MHI_CB_FATAL_ERROR
to the controller drivers. But there is no MHI_CB_FATAL_ERROR provided for the
client drivers like this one.
Even in downstream (msm-4.14), I can't find where the early notifier is getting
called.
> > > > + spin_lock_irqsave(&qdev->ul_lock, flags);
> > > > + list_for_each_entry(pkt, &qdev->ul_pkts, node)
> > > > + complete_all(&pkt->done);
> >
> > Chris, shouldn't we require list_del(&pkt->node) here?
> >
>
> No this isn't a full cleanup, with the "early notifier" we just unblocked
> any threads waiting for the ul_callback. Those threads will wake, check
> in_reset, return an error back to the caller. Any list cleanup will be done
> in the ul_callbacks that the mhi bus will do for each queued packet right
> before device remove.
>
> Again to simplify the code, we can probable remove the in_reset handling
> since it's not required with the current feature set.
>
So since we are not getting status_cb for fatal errors, I think we should just
remove status_cb, in_reset and timeout code.
> > > > + spin_unlock_irqrestore(&qdev->ul_lock, flags);
> > > > +}
> > > > +
> > > > +/* Send data over MHI */
> > > > +static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
> > > > +{
> > > > + struct qrtr_mhi_dev *qdev = container_of(ep, struct qrtr_mhi_dev, ep);
> > > > + struct qrtr_mhi_pkt *pkt;
> > > > + int rc;
> > > > +
> > > > + rc = skb_linearize(skb);
> > > > + if (rc) {
> > > > + kfree_skb(skb);
> > > > + return rc;
> > > > + }
> > > > +
> > > > + pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
> > > > + if (!pkt) {
> > > > + kfree_skb(skb);
> > > > + return -ENOMEM;
> > > > + }
> > > > +
> > > > + init_completion(&pkt->done);
> > > > + kref_init(&pkt->refcount);
> > > > + kref_get(&pkt->refcount);
> > > > + pkt->skb = skb;
> > > > +
> > > > + spin_lock_bh(&qdev->ul_lock);
> > > > + list_add_tail(&pkt->node, &qdev->ul_pkts);
> > > > + rc = mhi_queue_skb(qdev->mhi_dev, DMA_TO_DEVICE, skb, skb->len,
> > > > + MHI_EOT);
> > >
> > > Do you want to continue doing this when qdev->in_reset? Wouldn't it be
> > > better to bail early if the remote end is dying?
> > >
> >
> > Now I'm thinking why we are not decrementing in_reset anywhere! Incase of
> > SYS_ERR, the status_cb will get processed and in_reset will be set but
> > it will stay so even when after MHI gets reset.
> >
> > Chris, can you please clarify?
> >
>
> Early notify of reset, if we do get SYS_ERR, we expect remove to be called
> *soon*.
>
> > > > + if (rc) {
> > > > + list_del(&pkt->node);
> > > > + /* Reference count needs to be dropped 2 times */
> > > > + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> > > > + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> > > > + kfree_skb(skb);
> > > > + spin_unlock_bh(&qdev->ul_lock);
> > > > + return rc;
> > > > + }
> > > > +
> > > > + spin_unlock_bh(&qdev->ul_lock);
> > > > + if (skb->sk)
> > > > + sock_hold(skb->sk);
> > > > +
> > > > + rc = wait_for_completion_interruptible_timeout(&pkt->done, HZ * 5);
> > > > + if (atomic_read(&qdev->in_reset))
> > > > + rc = -ECONNRESET;
> > > > + else if (rc == 0)
> > > > + rc = -ETIMEDOUT;
> > >
> > > Is this recoverable? The message will remain on the list and may be
> > > delivered at a later point(?), but qrtr and the app will learn that the
> > > message was lost - which is presumably considered fatal.
> > >
> > > Is it guaranteed that qcom_mhi_qrtr_ul_callback() will happen later and
> > > find the head of the list?
> > >
> >
> > There are 2 scenarios:
> >
> > 1. If the completion interrupt happens after timeout, ul_callback()
> > will be called. But it will only try to fetch the current head of ul_pkts.
> > In most cases, we can hope that the completion interrupt will happen before
> > next queue_skb().
> >
> > 2. If we don't get completion interrupt, timeout will happen and at the final
> > stage (during mhi_driver_remove()), MHI stack will go over the pending TREs
> > for all channels in queue and call ul_callback() with -ENOTCONN. But in the
> > callback, we don't have any idea of the pkt which was not successfully
> > transferred to the device and currently just fetching first entry.
> >
> > Now I'm seeing some issue here which I missed earlier. If the completion
> > interrupt never happens then the corresponding pkt will never get freed and
> > therefore we have a leak. Eventhough the ul_callback() will get called during
> > mhi_driver_remove() for pending TREs, we don't exactly fetch the right pkt.
> >
> > Chris, our assumption of the ul_callback() gets called irrespective of
> > transfer status is wrong. I think this code needs a bit of rework.
> >
All fine here. I overlooked the list_del() part where the sent packet
gets popped out of list and when the MHI stack calls ul_callback() during
mhi_driver_remove() the leftover packets will get handled correctly.
> > >
> > > The reason for my question is that without this you have one of two
> > > scenarios;
> > > 1) the message is put on the list, decremented in
> > > qcom_mhi_qrtr_ul_callback() then we get back here and decrement it
> > > again.
> > > 2) the message is put on the list, then qcom_mhi_qrtr_status_callback()
> > > happens and all messages are released - presumably then
> > > qcom_mhi_qrtr_ul_callback() won't happen.
> > >
> > >
> > > So if the third case (where we return here and then later
> > > qcom_mhi_qrtr_ul_callback() must find this particular packet at the
> > > front of the queue) can't happen, then you can just skip the entire
> > > refcounting.
> > >
> > > Further more, you could carry qrtr_mhi_pkt on the stack.
> > >
> > >
> > > ...or to flip this around, is there a reason to wait here at all? What
> > > would happen if you just return immediately after calling
> > > mhi_queue_skb()? Wouldn't that provide you better throughput?
> > >
> >
> > Chris would be best person to answer this question.
> >
>
> Yea - after working with MHI for a bit now, I think we can definitely return
> after queueing the skb.
>
> The IPC-Router Protocol, wasn't really designed with the ability to recover
> from dropped packets since SMD/GLINK are "Lossless". I figured it would be
> better for the client to definitely know that the packet reached the other
> side which is why it blocks until ul callback.
>
Agree.
> I thought having the client get an error on timeout and resend the packet
> would be better than silently dropping it. In practice, we've really only
> seen the timeout or ul_callback errors on unrecoverable errors so I think
> the timeout handling can definitely be redone.
>
You mean we can just remove the timeout handling part and return after
kref_put()?
Thanks,
Mani
> Thanks,
> Chris
>
> > > > + else if (rc > 0)
> > > > + rc = 0;
> > > > +
> > > > + kref_put(&pkt->refcount, qrtr_mhi_pkt_release);
> > > > +
> > > > + return rc;
> > > > +}
> > > > +
> > > > +static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
> > > > + const struct mhi_device_id *id)
> > > > +{
> > > > + struct qrtr_mhi_dev *qdev;
> > > > + u32 net_id;
> > > > + int rc;
> > > > +
> > > > + qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL);
> > > > + if (!qdev)
> > > > + return -ENOMEM;
> > > > +
> > > > + qdev->mhi_dev = mhi_dev;
> > > > + qdev->dev = &mhi_dev->dev;
> > > > + qdev->ep.xmit = qcom_mhi_qrtr_send;
> > > > + atomic_set(&qdev->in_reset, 0);
> > > > +
> > > > + net_id = QRTR_EP_NID_AUTO;
> > >
> > > Just pass QRTR_EP_NID_AUTO directly in the function call below.
> > >
> >
> > Okay.
> >
> > Thanks,
> > Mani
> >
> > > Regards,
> > > Bjorn
> > >
> > > > +
> > > > + INIT_LIST_HEAD(&qdev->ul_pkts);
> > > > + spin_lock_init(&qdev->ul_lock);
> > > > +
> > > > + dev_set_drvdata(&mhi_dev->dev, qdev);
> > > > + rc = qrtr_endpoint_register(&qdev->ep, net_id);
> > > > + if (rc)
> > > > + return rc;
> > > > +
> > > > + dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n");
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void qcom_mhi_qrtr_remove(struct mhi_device *mhi_dev)
> > > > +{
> > > > + struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> > > > +
> > > > + qrtr_endpoint_unregister(&qdev->ep);
> > > > + dev_set_drvdata(&mhi_dev->dev, NULL);
> > > > +}
> > > > +
> > > > +static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = {
> > > > + { .chan = "IPCR" },
> > > > + {}
> > > > +};
> > > > +MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table);
> > > > +
> > > > +static struct mhi_driver qcom_mhi_qrtr_driver = {
> > > > + .probe = qcom_mhi_qrtr_probe,
> > > > + .remove = qcom_mhi_qrtr_remove,
> > > > + .dl_xfer_cb = qcom_mhi_qrtr_dl_callback,
> > > > + .ul_xfer_cb = qcom_mhi_qrtr_ul_callback,
> > > > + .status_cb = qcom_mhi_qrtr_status_callback,
> > > > + .id_table = qcom_mhi_qrtr_id_table,
> > > > + .driver = {
> > > > + .name = "qcom_mhi_qrtr",
> > > > + },
> > > > +};
> > > > +
> > > > +module_mhi_driver(qcom_mhi_qrtr_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("Qualcomm IPC-Router MHI interface driver");
> > > > +MODULE_LICENSE("GPL v2");
> > > > --
> > > > 2.17.1
> > > >
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project
On Mon 30 Mar 02:49 PDT 2020, Manivannan Sadhasivam wrote:
> Hi Chris,
>
> On Thu, Mar 26, 2020 at 03:54:42PM -0700, Chris Lew wrote:
> >
> >
> > On 3/25/2020 3:37 AM, Manivannan Sadhasivam wrote:
> > > Hi Bjorn,
> > >
> > > + Chris Lew
> > >
> > > On Tue, Mar 24, 2020 at 01:39:52PM -0700, Bjorn Andersson wrote:
> > > > On Mon 23 Mar 23:10 PDT 2020, Manivannan Sadhasivam wrote:
[..]
> > > > > + spin_lock_irqsave(&qdev->ul_lock, flags);
> > > > > + list_for_each_entry(pkt, &qdev->ul_pkts, node)
> > > > > + complete_all(&pkt->done);
> > >
> > > Chris, shouldn't we require list_del(&pkt->node) here?
> > >
> >
> > No this isn't a full cleanup, with the "early notifier" we just unblocked
> > any threads waiting for the ul_callback. Those threads will wake, check
> > in_reset, return an error back to the caller. Any list cleanup will be done
> > in the ul_callbacks that the mhi bus will do for each queued packet right
> > before device remove.
> >
> > Again to simplify the code, we can probable remove the in_reset handling
> > since it's not required with the current feature set.
> >
>
> So since we are not getting status_cb for fatal errors, I think we should just
> remove status_cb, in_reset and timeout code.
>
Looks reasonable.
[..]
> > I thought having the client get an error on timeout and resend the packet
> > would be better than silently dropping it. In practice, we've really only
> > seen the timeout or ul_callback errors on unrecoverable errors so I think
> > the timeout handling can definitely be redone.
> >
>
> You mean we can just remove the timeout handling part and return after
> kref_put()?
>
If all messages are "generated" by qcom_mhi_qrtr_send() and "released"
in qcom_mhi_qrtr_ul_callback() I don't think you need the refcounting at
all.
Presumably though, it would have been nice to not have to carry a
separate list of packets (and hope that it's in sync with the mhi core)
and instead have the ul callback somehow allow us to derive the skb to
be freed.
Regards,
Bjorn
Hi Bjorn,
On Mon, Mar 30, 2020 at 03:19:32PM -0700, Bjorn Andersson wrote:
> On Mon 30 Mar 02:49 PDT 2020, Manivannan Sadhasivam wrote:
>
> > Hi Chris,
> >
> > On Thu, Mar 26, 2020 at 03:54:42PM -0700, Chris Lew wrote:
> > >
> > >
> > > On 3/25/2020 3:37 AM, Manivannan Sadhasivam wrote:
> > > > Hi Bjorn,
> > > >
> > > > + Chris Lew
> > > >
> > > > On Tue, Mar 24, 2020 at 01:39:52PM -0700, Bjorn Andersson wrote:
> > > > > On Mon 23 Mar 23:10 PDT 2020, Manivannan Sadhasivam wrote:
> [..]
> > > > > > + spin_lock_irqsave(&qdev->ul_lock, flags);
> > > > > > + list_for_each_entry(pkt, &qdev->ul_pkts, node)
> > > > > > + complete_all(&pkt->done);
> > > >
> > > > Chris, shouldn't we require list_del(&pkt->node) here?
> > > >
> > >
> > > No this isn't a full cleanup, with the "early notifier" we just unblocked
> > > any threads waiting for the ul_callback. Those threads will wake, check
> > > in_reset, return an error back to the caller. Any list cleanup will be done
> > > in the ul_callbacks that the mhi bus will do for each queued packet right
> > > before device remove.
> > >
> > > Again to simplify the code, we can probable remove the in_reset handling
> > > since it's not required with the current feature set.
> > >
> >
> > So since we are not getting status_cb for fatal errors, I think we should just
> > remove status_cb, in_reset and timeout code.
> >
>
> Looks reasonable.
>
> [..]
> > > I thought having the client get an error on timeout and resend the packet
> > > would be better than silently dropping it. In practice, we've really only
> > > seen the timeout or ul_callback errors on unrecoverable errors so I think
> > > the timeout handling can definitely be redone.
> > >
> >
> > You mean we can just remove the timeout handling part and return after
> > kref_put()?
> >
>
> If all messages are "generated" by qcom_mhi_qrtr_send() and "released"
> in qcom_mhi_qrtr_ul_callback() I don't think you need the refcounting at
> all.
>
Hmm, you're right. We can move the packet releasing part to ul_callback now.
>
> Presumably though, it would have been nice to not have to carry a
> separate list of packets (and hope that it's in sync with the mhi core)
> and instead have the ul callback somehow allow us to derive the skb to
> be freed.
>
Yep, MHI stack holds the skb in buf_addr member of mhi_result. So, we can just
use below to get the skb in ul_callback:
struct sk_buff *skb = (struct sk_buff *)mhi_res->buf_addr;
This will help us to avoid the use of pkt, ul_pkts list and use the skb directly
everywhere. At the same time I think we can also remove the ul_lock which
was added to protect the ul_pkts list.
Let me know your opinion, I'll just send a series with this modified QRTR MHI
client driver and MHI suspend/resume patches.
Thanks,
Mani
> Regards,
> Bjorn
On Tue 31 Mar 04:23 PDT 2020, Manivannan Sadhasivam wrote:
> Hi Bjorn,
>
> On Mon, Mar 30, 2020 at 03:19:32PM -0700, Bjorn Andersson wrote:
> > On Mon 30 Mar 02:49 PDT 2020, Manivannan Sadhasivam wrote:
> >
> > > Hi Chris,
> > >
> > > On Thu, Mar 26, 2020 at 03:54:42PM -0700, Chris Lew wrote:
> > > >
> > > >
> > > > On 3/25/2020 3:37 AM, Manivannan Sadhasivam wrote:
> > > > > Hi Bjorn,
> > > > >
> > > > > + Chris Lew
> > > > >
> > > > > On Tue, Mar 24, 2020 at 01:39:52PM -0700, Bjorn Andersson wrote:
> > > > > > On Mon 23 Mar 23:10 PDT 2020, Manivannan Sadhasivam wrote:
> > [..]
> > > > > > > + spin_lock_irqsave(&qdev->ul_lock, flags);
> > > > > > > + list_for_each_entry(pkt, &qdev->ul_pkts, node)
> > > > > > > + complete_all(&pkt->done);
> > > > >
> > > > > Chris, shouldn't we require list_del(&pkt->node) here?
> > > > >
> > > >
> > > > No this isn't a full cleanup, with the "early notifier" we just unblocked
> > > > any threads waiting for the ul_callback. Those threads will wake, check
> > > > in_reset, return an error back to the caller. Any list cleanup will be done
> > > > in the ul_callbacks that the mhi bus will do for each queued packet right
> > > > before device remove.
> > > >
> > > > Again to simplify the code, we can probable remove the in_reset handling
> > > > since it's not required with the current feature set.
> > > >
> > >
> > > So since we are not getting status_cb for fatal errors, I think we should just
> > > remove status_cb, in_reset and timeout code.
> > >
> >
> > Looks reasonable.
> >
> > [..]
> > > > I thought having the client get an error on timeout and resend the packet
> > > > would be better than silently dropping it. In practice, we've really only
> > > > seen the timeout or ul_callback errors on unrecoverable errors so I think
> > > > the timeout handling can definitely be redone.
> > > >
> > >
> > > You mean we can just remove the timeout handling part and return after
> > > kref_put()?
> > >
> >
> > If all messages are "generated" by qcom_mhi_qrtr_send() and "released"
> > in qcom_mhi_qrtr_ul_callback() I don't think you need the refcounting at
> > all.
> >
>
> Hmm, you're right. We can move the packet releasing part to ul_callback now.
>
> >
> > Presumably though, it would have been nice to not have to carry a
> > separate list of packets (and hope that it's in sync with the mhi core)
> > and instead have the ul callback somehow allow us to derive the skb to
> > be freed.
> >
>
> Yep, MHI stack holds the skb in buf_addr member of mhi_result. So, we can just
> use below to get the skb in ul_callback:
>
> struct sk_buff *skb = (struct sk_buff *)mhi_res->buf_addr;
>
> This will help us to avoid the use of pkt, ul_pkts list and use the skb directly
> everywhere. At the same time I think we can also remove the ul_lock which
> was added to protect the ul_pkts list.
>
> Let me know your opinion, I'll just send a series with this modified QRTR MHI
> client driver and MHI suspend/resume patches.
>
This looks more robust than having the separate list shadowing the
internal state of the MHI core.
+1
Thanks,
Bjorn