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.
v4:
-Added reviewed-by tags
-Updated range check patch to include BHI/e offsets in the error message
v3:
-Added reviewed-by tags
-Updated order of reg_len in mhi_controller structure documentation
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 | 61 ++++++++++++++++++++++++-----------
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, 50 insertions(+), 43 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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.
Signed-off-by: Bhaumik Bhatt <[email protected]>
Reviewed-by: Jeffrey Hugo <[email protected]>
Reviewed-by: Hemant Kumar <[email protected]>
---
drivers/bus/mhi/core/init.c | 42 +++++++++++++++++++++++-------------------
drivers/bus/mhi/core/pm.c | 28 ++++------------------------
2 files changed, 27 insertions(+), 43 deletions(-)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index c81b377..11c7a3d 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -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);
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index e2e59a3..adf426c 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -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:
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
When 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.
Signed-off-by: Bhaumik Bhatt <[email protected]>
Reviewed-by: Jeffrey Hugo <[email protected]>
---
drivers/bus/mhi/core/init.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 1cc2f22..aeb1e3c 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -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,13 @@ 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: 0x%x is out of range: 0x%zx\n",
+ bhi_off, mhi_cntrl->reg_len);
+ 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 +1094,14 @@ 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: 0x%x is out of range: 0x%zx\n",
+ bhie_off, mhi_cntrl->reg_len);
+ ret = -EINVAL;
+ goto error_reg_offset;
+ }
mhi_cntrl->bhie = mhi_cntrl->regs + bhie_off;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 5/6/21 12:51 PM, Bhaumik Bhatt wrote:
> When 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.
>
> Signed-off-by: Bhaumik Bhatt <[email protected]>
> Reviewed-by: Jeffrey Hugo <[email protected]>
Reviewed-by: Hemant Kumar <[email protected]>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Thu, May 06, 2021 at 12:51:40PM -0700, Bhaumik Bhatt wrote:
> 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.
>
> Signed-off-by: Bhaumik Bhatt <[email protected]>
> Reviewed-by: Jeffrey Hugo <[email protected]>
> Reviewed-by: Hemant Kumar <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Thanks,
Mani
> ---
> drivers/bus/mhi/core/init.c | 42 +++++++++++++++++++++++-------------------
> drivers/bus/mhi/core/pm.c | 28 ++++------------------------
> 2 files changed, 27 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index c81b377..11c7a3d 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -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);
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index e2e59a3..adf426c 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -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:
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On Thu, May 06, 2021 at 12:51:45PM -0700, Bhaumik Bhatt wrote:
> When 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.
>
> Signed-off-by: Bhaumik Bhatt <[email protected]>
> Reviewed-by: Jeffrey Hugo <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Thanks,
Mani
> ---
> drivers/bus/mhi/core/init.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 1cc2f22..aeb1e3c 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -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,13 @@ 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: 0x%x is out of range: 0x%zx\n",
> + bhi_off, mhi_cntrl->reg_len);
> + 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 +1094,14 @@ 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: 0x%x is out of range: 0x%zx\n",
> + bhie_off, mhi_cntrl->reg_len);
> + ret = -EINVAL;
> + goto error_reg_offset;
> + }
> mhi_cntrl->bhie = mhi_cntrl->regs + bhie_off;
> }
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On Thu, May 06, 2021 at 12:51:39PM -0700, Bhaumik Bhatt wrote:
> 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.
>
Since there is a chance to send this series for 5.14, I've applied it
now to mhi-next!
Thanks,
Mani
> v4:
> -Added reviewed-by tags
> -Updated range check patch to include BHI/e offsets in the error message
>
> v3:
> -Added reviewed-by tags
> -Updated order of reg_len in mhi_controller structure documentation
>
> 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 | 61 ++++++++++++++++++++++++-----------
> 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, 50 insertions(+), 43 deletions(-)
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>