BHI/BHIe improvements for MHI power purposes
v2 cover BHI/BHIe improvements for MHI power purposes
This patch series improves the power up behavior by allowing MHI host driver to set BHI and/or BHIe offsets early on in the preparation phase and fail pre-power up if offsets are not found or not within a limited MMIO region. This also allows MHI host to clean up the offsets in the unprepare after power down phase.
Going forward, controllers will be required to specify a reg_len field which will be used to check whether the BHI/BHIe offsets are in range or not.
This series has been tested on X86_64 architecture with the PCI generic driver as controller and an SDX55 device.
v2:
-Added reviewed-by tags
-Moved reg_len entry in mhi_controller structure to allow for a packed struct
Bhaumik Bhatt (6):
bus: mhi: core: Set BHI/BHIe offsets on power up preparation
bus: mhi: core: Set BHI and BHIe pointers to NULL in clean-up
bus: mhi: Add MMIO region length to controller structure
ath11k: set register access length for MHI driver
bus: mhi: pci_generic: Set register access length for MHI driver
bus: mhi: core: Add range checks for BHI and BHIe
drivers/bus/mhi/core/init.c | 58 +++++++++++++++++++++++------------ drivers/bus/mhi/core/pm.c | 28 +++-------------- drivers/bus/mhi/pci_generic.c | 1 + drivers/net/wireless/ath/ath11k/mhi.c | 1 + include/linux/mhi.h | 2 ++ 5 files changed, 47 insertions(+), 43 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v2 1/6 bus: mhi: core: Set BHI/BHIe offsets on power up preparation
Set the BHI and/or BHIe offsets in mhi_prepare_for_power_up(), rearrange the function, and remove the equivalent from mhi_async_power_up(). This helps consolidate multiple checks in different parts of the driver and can help MHI fail early on before power up begins if the offsets are not read correctly.
--- drivers/bus/mhi/core/init.c | 42 +++++++++++++++++++++++------------------- drivers/bus/mhi/core/pm.c | 28 ++++------------------------ 2 files changed, 27 insertions(+), 43 deletions(-)
drivers/bus/mhi/core/init.c +23 -19
@@ -1063,7 +1063,7 @@ EXPORT_SYMBOL_GPL(mhi_free_controller);int mhi_prepare_for_power_up (struct mhi_controller *mhi_cntrl) {struct device *dev = &mhi_cntrl->mhi_dev->dev; -u32 bhie_off; +u32 bhi_off, bhie_off;int ret;mutex_lock (&mhi_cntrl->pm_mutex);
@@ -1072,29 +1072,36 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl)if (ret) goto error_dev_ctxt; -/* -* Allocate RDDM table if specified, this table is for debugging purpose -*/ -if (mhi_cntrl->rddm_size) { -mhi_alloc_bhie_table (mhi_cntrl, &mhi_cntrl->rddm_image, - mhi_cntrl->rddm_size); + ret =mhi_read_reg (mhi_cntrl, mhi_cntrl->regs,BHIOFF , &bhi_off); +if (ret) { +dev_err (dev,"Error getting BHI offset\n" ); + goto error_reg_offset; + } + mhi_cntrl->bhi = mhi_cntrl->regs + bhi_off; -/* -* This controller supports RDDM, so we need to manually clear -* BHIE RX registers since POR values are undefined. -*/ +if (mhi_cntrl->fbc_download || mhi_cntrl->rddm_size) { ret =mhi_read_reg (mhi_cntrl, mhi_cntrl->regs,BHIEOFF , &bhie_off);if (ret) {dev_err (dev,"Error getting BHIE offset\n" ); - goto bhie_error; + goto error_reg_offset; } - mhi_cntrl->bhie = mhi_cntrl->regs + bhie_off; + } + +if (mhi_cntrl->rddm_size) { +/* +* This controller supports RDDM, so we need to manually clear +* BHIE RX registers since POR values are undefined. +*/ memset_io (mhi_cntrl->bhie +BHIE_RXVECADDR_LOW_OFFS ,0 ,BHIE_RXVECSTATUS_OFFS -BHIE_RXVECADDR_LOW_OFFS +4 ); - +/* +* Allocate RDDM table for debugging purpose if specified +*/ +mhi_alloc_bhie_table (mhi_cntrl, &mhi_cntrl->rddm_image, + mhi_cntrl->rddm_size);if (mhi_cntrl->rddm_image)mhi_rddm_prepare (mhi_cntrl, mhi_cntrl->rddm_image); }
@@ -1103,11 +1110,8 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl)return 0 ; -bhie_error: -if (mhi_cntrl->rddm_image) { -mhi_free_bhie_table (mhi_cntrl, mhi_cntrl->rddm_image); - mhi_cntrl->rddm_image =NULL ; - } +error_reg_offset: +mhi_deinit_dev_ctxt (mhi_cntrl); error_dev_ctxt:mutex_unlock (&mhi_cntrl->pm_mutex);
drivers/bus/mhi/core/pm.c +4 -24
@@ -1066,28 +1066,8 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)if (ret) goto error_setup_irq; -/* Setup BHI offset & INTVEC */ +/* Setup BHI INTVEC */ write_lock_irq (&mhi_cntrl->pm_lock); - ret =mhi_read_reg (mhi_cntrl, mhi_cntrl->regs,BHIOFF , &val); -if (ret) { -write_unlock_irq (&mhi_cntrl->pm_lock); - goto error_bhi_offset; - } - - mhi_cntrl->bhi = mhi_cntrl->regs + val; - -/* Setup BHIE offset */ -if (mhi_cntrl->fbc_download) { - ret =mhi_read_reg (mhi_cntrl, mhi_cntrl->regs,BHIEOFF , &val); -if (ret) { -write_unlock_irq (&mhi_cntrl->pm_lock); -dev_err (dev,"Error reading BHIE offset\n" ); - goto error_bhi_offset; - } - - mhi_cntrl->bhie = mhi_cntrl->regs + val; - } -mhi_write_reg (mhi_cntrl, mhi_cntrl->bhi,BHI_INTVEC ,0 ); mhi_cntrl->pm_state =MHI_PM_POR ; mhi_cntrl->ee =MHI_EE_MAX ;
@@ -1098,7 +1078,7 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)if (!MHI_IN_PBL (current_ee) && current_ee !=MHI_EE_AMSS ) {dev_err (dev,"Not a valid EE for power on\n" ); ret = -EIO ; - goto error_bhi_offset; + goto error_async_power_up; } state =mhi_get_mhi_state (mhi_cntrl);
@@ -1117,7 +1097,7 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)if (!ret) { ret = -EIO ;dev_info (dev,"Failed to reset MHI due to syserr state\n" ); - goto error_bhi_offset; + goto error_async_power_up; }/*
@@ -1139,7 +1119,7 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)return 0 ; -error_bhi_offset: +error_async_power_up:mhi_deinit_free_irq (mhi_cntrl); error_setup_irq:
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v2 2/6 bus: mhi: core: Set BHI and BHIe pointers to NULL in clean-up
In reply to Bhaumik BhattSet the BHI and BHIe pointers to NULL as part of clean-up. This makes sure that stale pointers are not accessed after powering MHI down.
--- drivers/bus/mhi/core/init.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/bus/mhi/core/init.c +3 -0
@@ -1132,6 +1132,9 @@ void mhi_unprepare_after_power_down(struct mhi_controller *mhi_cntrl) mhi_cntrl->rddm_image =NULL ; } + mhi_cntrl->bhi =NULL ; + mhi_cntrl->bhie =NULL ; +mhi_deinit_dev_ctxt (mhi_cntrl); }EXPORT_SYMBOL_GPL (mhi_unprepare_after_power_down);
Signature · 8 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- ath11k mailing list [email protected] http://lists.infradead.org/mailman/listinfo/ath11k
PATCH v2 3/6 bus: mhi: Add MMIO region length to controller structure
In reply to Bhaumik BhattMake controller driver specify the MMIO register region length for range checking of BHI or BHIe space. This can help validate that offsets are in acceptable memory region or not and avoid any boot-up issues due to BHI or BHIe memory accesses.
--- include/linux/mhi.h | 2 ++ 1 file changed, 2 insertions(+)
include/linux/mhi.h +2 -0
@@ -293,6 +293,7 @@ struct mhi_controller_config {* @mhi_dev: MHI device instance for the controller * @debugfs_dentry: MHI controller debugfs directory * @regs: Base address of MHI MMIO register space (required) +* @reg_len: Length of the MHI MMIO region (required) * @bhi: Points to base of MHI BHI register space * @bhie: Points to base of MHI BHIe register space * @wake_db: MHI WAKE doorbell register address
@@ -386,6 +387,7 @@ struct mhi_controller {size_t rddm_size;size_t sbl_size;size_t seg_len; +size_t reg_len;struct image_info *fbc_image;struct image_info *rddm_image;struct mhi_chan *mhi_chan;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v2 3/6] bus: mhi: Add MMIO region length to controller structure
Quoted 15 lines · Bhaumik Bhatt
Make controller driver specify the MMIO register region length for range checking of BHI or BHIe space. This can help validate that offsets are in acceptable memory region or not and avoid any boot-up issues due to BHI or BHIe memory accesses.
Signed-off-by Bhaumik Bhatt <[email protected]>--- include/linux/mhi.h | 2 ++ 1 file changed, 2 insertions(+)include/linux/mhi.h +0
-0
include/linux/mhi.h struct mhi_controller_config { · line 296
Quoted 4 lines
include/linux/mhi.h +1
-0@@ -293,6 +293,7 @@ struct mhi_controller_config {* @mhi_dev: MHI device instance for the controller * @debugfs_dentry: MHI controller debugfs directory * @regs: Base address of MHI MMIO register space (required) +* @reg_len: Length of the MHI MMIO region (required)
I believe this should be moved so that it matches the ordering of the struct fields below.
With the adjustment, you can add my reviewed tag to this.
Quoted 12 lines
include/linux/mhi.h +1
-0* @bhi: Points to base of MHI BHI register space * @bhie: Points to base of MHI BHIe register space * @wake_db: MHI WAKE doorbell register address @@ -386,6 +387,7 @@ struct mhi_controller {size_t rddm_size;size_t sbl_size;size_t seg_len; +size_t reg_len;struct image_info *fbc_image;struct image_info *rddm_image;struct mhi_chan *mhi_chan;
PATCH v2 4/6 ath11k: set register access length for MHI driver
In reply to Bhaumik BhattMHI driver requires register space length to add range checks and prevent memory region accesses outside of that for MMIO space. Set it before registering the MHI controller.
--- drivers/net/wireless/ath/ath11k/mhi.c | 1 + 1 file changed, 1 insertion(+)
drivers/net/wireless/ath/ath11k/mhi.c +1 -0
@@ -235,6 +235,7 @@ int ath11k_mhi_register(struct ath11k_pci *ab_pci) mhi_ctrl->cntrl_dev = ab->dev; mhi_ctrl->fw_image = ab_pci->amss_path; mhi_ctrl->regs = ab->mem; + mhi_ctrl->reg_len = ab->mem_len; ret =ath11k_mhi_get_msi (ab_pci);if (ret) {
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v2 5/6 bus: mhi: pci_generic: Set register access length for MHI driver
In reply to Bhaumik BhattMHI driver requires register space length to add range checks and prevent memory region accesses outside of that for MMIO space. Set it from the PCI generic controller driver before registering the MHI controller.
--- drivers/bus/mhi/pci_generic.c | 1 + 1 file changed, 1 insertion(+)
drivers/bus/mhi/pci_generic.c +1 -0
@@ -463,6 +463,7 @@ static int mhi_pci_claim(struct mhi_controller *mhi_cntrl,return err; } mhi_cntrl->regs =pcim_iomap_table (pdev)[bar_num]; + mhi_cntrl->reg_len =pci_resource_len (pdev, bar_num); err =pci_set_dma_mask (pdev, dma_mask);if (err) {
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v2 6/6 bus: mhi: core: Add range checks for BHI and BHIe
In reply to Bhaumik BhattWhen obtaining the BHI or BHIe offsets during the power up preparation phase, range checks are missing. These can help controller drivers avoid accessing any address outside of the MMIO region. Ensure that mhi_cntrl->reg_len is set before MHI registration as it is a required field and range checks will fail without it.
--- drivers/bus/mhi/core/init.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
drivers/bus/mhi/core/init.c +14 -1
@@ -885,7 +885,8 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,if (!mhi_cntrl || !mhi_cntrl->cntrl_dev || !mhi_cntrl->regs || !mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put || !mhi_cntrl->status_cb || !mhi_cntrl->read_reg || - !mhi_cntrl->write_reg || !mhi_cntrl->nr_irqs || !mhi_cntrl->irq) + !mhi_cntrl->write_reg || !mhi_cntrl->nr_irqs || + !mhi_cntrl->irq || !mhi_cntrl->reg_len)return -EINVAL ; ret =parse_config (mhi_cntrl, config);
@@ -1077,6 +1078,12 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl)dev_err (dev,"Error getting BHI offset\n" ); goto error_reg_offset; } + +if (bhi_off >= mhi_cntrl->reg_len) { +dev_err (dev,"BHI offset is out of range\n" ); + ret = -EINVAL ; + goto error_reg_offset; + } mhi_cntrl->bhi = mhi_cntrl->regs + bhi_off;if (mhi_cntrl->fbc_download || mhi_cntrl->rddm_size) {
@@ -1086,6 +1093,12 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl)dev_err (dev,"Error getting BHIE offset\n" ); goto error_reg_offset; } + +if (bhie_off >= mhi_cntrl->reg_len) { +dev_err (dev,"BHIe offset is out of range\n" ); + ret = -EINVAL ; + goto error_reg_offset; + } mhi_cntrl->bhie = mhi_cntrl->regs + bhie_off; }
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project