| Message ID | 1629330634-36465-2-git-send-email-bbhatt@codeaurora.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MHI MMIO register write updates | expand |
On 8/18/2021 4:50 PM, Bhaumik Bhatt wrote: > Helper API to write register fields relies on successful reads > of the register/address prior to the write. Bail out if a failure > is seen when reading the register before the actual write is > performed. > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> Reviewed-by: Hemant Kumar <hemantk@codeaurora.org> -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Bhaumik,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20210818]
[cannot apply to linus/master v5.14-rc6 v5.14-rc5 v5.14-rc4 v5.14-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Bhaumik-Bhatt/MHI-MMIO-register-write-updates/20210819-075312
base: f26c3abc432a2026ba9ee7767061a1f88aead6ec
config: arm64-randconfig-r025-20210818 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d2b574a4dea5b718e4386bf2e26af0126e5978ce)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/0day-ci/linux/commit/034c38ae851193cbeec4b4538d1a47e75198b92a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Bhaumik-Bhatt/MHI-MMIO-register-write-updates/20210819-075312
git checkout 034c38ae851193cbeec4b4538d1a47e75198b92a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/bus/mhi/core/pm.c:133:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
^~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/bus/mhi/core/pm.c:136:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
^~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
vim +/warn_unused_result +133 drivers/bus/mhi/core/pm.c
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 129
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 130 void mhi_set_mhi_state(struct mhi_controller *mhi_cntrl, enum mhi_state state)
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 131 {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 132 if (state == MHI_STATE_RESET) {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 @133 mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 134 MHICTRL_RESET_MASK, MHICTRL_RESET_SHIFT, 1);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 135 } else {
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 136 mhi_write_reg_field(mhi_cntrl, mhi_cntrl->regs, MHICTRL,
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 137 MHICTRL_MHISTATE_MASK,
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 138 MHICTRL_MHISTATE_SHIFT, state);
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 139 }
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 140 }
a6e2e3522f2914 Manivannan Sadhasivam 2020-02-20 141
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 8/18/2021 7:40 PM, Hemant Kumar wrote: > > > On 8/18/2021 4:50 PM, Bhaumik Bhatt wrote: >> Helper API to write register fields relies on successful reads >> of the register/address prior to the write. Bail out if a failure >> is seen when reading the register before the actual write is >> performed. >> >> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org> > Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
On Wed, Aug 18, 2021 at 04:50:33PM -0700, Bhaumik Bhatt wrote: > Helper API to write register fields relies on successful reads > of the register/address prior to the write. Bail out if a failure > is seen when reading the register before the actual write is > performed. > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> As spotted by the test bot, can you please update pm.c as well? Thanks, Mani > --- > drivers/bus/mhi/core/boot.c | 25 +++++++++++++++++-------- > drivers/bus/mhi/core/init.c | 24 ++++++++++++++++++------ > drivers/bus/mhi/core/internal.h | 7 ++++--- > drivers/bus/mhi/core/main.c | 9 ++++++--- > 4 files changed, 45 insertions(+), 20 deletions(-) > > diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c > index 0a97262..13eacda 100644 > --- a/drivers/bus/mhi/core/boot.c > +++ b/drivers/bus/mhi/core/boot.c > @@ -19,8 +19,8 @@ > #include "internal.h" > > /* Setup RDDM vector table for RDDM transfer and program RXVEC */ > -void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, > - struct image_info *img_info) > +int mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, > + struct image_info *img_info) > { > struct mhi_buf *mhi_buf = img_info->mhi_buf; > struct bhi_vec_entry *bhi_vec = img_info->bhi_vec; > @@ -28,6 +28,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, > struct device *dev = &mhi_cntrl->mhi_dev->dev; > u32 sequence_id; > unsigned int i; > + int ret; > > for (i = 0; i < img_info->entries - 1; i++, mhi_buf++, bhi_vec++) { > bhi_vec->dma_addr = mhi_buf->dma_addr; > @@ -45,12 +46,17 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, > mhi_write_reg(mhi_cntrl, base, BHIE_RXVECSIZE_OFFS, mhi_buf->len); > sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_RXVECSTATUS_SEQNUM_BMSK); > > - mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS, > - BHIE_RXVECDB_SEQNUM_BMSK, BHIE_RXVECDB_SEQNUM_SHFT, > - sequence_id); > + ret = mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS, > + BHIE_RXVECDB_SEQNUM_BMSK, > + BHIE_RXVECDB_SEQNUM_SHFT, sequence_id); > + if (ret) { > + dev_err(dev, "Failed to write sequence ID for BHIE_RXVECDB\n"); > + return ret; > + } > > dev_dbg(dev, "Address: %p and len: 0x%zx sequence: %u\n", > &mhi_buf->dma_addr, mhi_buf->len, sequence_id); > + return 0; > } > > /* Collect RDDM buffer during kernel panic */ > @@ -202,11 +208,14 @@ static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl, > > mhi_write_reg(mhi_cntrl, base, BHIE_TXVECSIZE_OFFS, mhi_buf->len); > > - mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS, > - BHIE_TXVECDB_SEQNUM_BMSK, BHIE_TXVECDB_SEQNUM_SHFT, > - sequence_id); > + ret = mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS, > + BHIE_TXVECDB_SEQNUM_BMSK, > + BHIE_TXVECDB_SEQNUM_SHFT, sequence_id); > read_unlock_bh(pm_lock); > > + if (ret) > + return ret; > + > /* Wait for the image download to complete */ > ret = wait_event_timeout(mhi_cntrl->state_event, > MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state) || > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c > index 5aaca6d..0917465 100644 > --- a/drivers/bus/mhi/core/init.c > +++ b/drivers/bus/mhi/core/init.c > @@ -544,10 +544,15 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl) > mhi_cntrl->mhi_cmd[PRIMARY_CMD_RING].ring.db_addr = base + CRDB_LOWER; > > /* Write to MMIO registers */ > - for (i = 0; reg_info[i].offset; i++) > - mhi_write_reg_field(mhi_cntrl, base, reg_info[i].offset, > - reg_info[i].mask, reg_info[i].shift, > - reg_info[i].val); > + for (i = 0; reg_info[i].offset; i++) { > + ret = mhi_write_reg_field(mhi_cntrl, base, reg_info[i].offset, > + reg_info[i].mask, reg_info[i].shift, > + reg_info[i].val); > + if (ret) { > + dev_err(dev, "Unable to write to MMIO registers"); > + return ret; > + } > + } > > return 0; > } > @@ -1118,8 +1123,15 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl) > */ > 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); > + if (mhi_cntrl->rddm_image) { > + ret = mhi_rddm_prepare(mhi_cntrl, > + mhi_cntrl->rddm_image); > + if (ret) { > + mhi_free_bhie_table(mhi_cntrl, > + mhi_cntrl->rddm_image); > + goto error_reg_offset; > + } > + } > } > > mutex_unlock(&mhi_cntrl->pm_mutex); > diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h > index 721739c..3d17ec3 100644 > --- a/drivers/bus/mhi/core/internal.h > +++ b/drivers/bus/mhi/core/internal.h > @@ -663,8 +663,9 @@ int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl, > u32 shift, u32 val, u32 delayus); > void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base, > u32 offset, u32 val); > -void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base, > - u32 offset, u32 mask, u32 shift, u32 val); > +int __must_check mhi_write_reg_field(struct mhi_controller *mhi_cntrl, > + void __iomem *base, u32 offset, > + u32 mask, u32 shift, u32 val); > void mhi_ring_er_db(struct mhi_event *mhi_event); > void mhi_write_db(struct mhi_controller *mhi_cntrl, void __iomem *db_addr, > dma_addr_t db_val); > @@ -678,7 +679,7 @@ int mhi_init_dev_ctxt(struct mhi_controller *mhi_cntrl); > void mhi_deinit_dev_ctxt(struct mhi_controller *mhi_cntrl); > int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl); > void mhi_deinit_free_irq(struct mhi_controller *mhi_cntrl); > -void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, > +int mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, > struct image_info *img_info); > void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl); > int mhi_prepare_channel(struct mhi_controller *mhi_cntrl, > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index c01ec2f..902d854 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -66,19 +66,22 @@ void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base, > mhi_cntrl->write_reg(mhi_cntrl, base + offset, val); > } > > -void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base, > - u32 offset, u32 mask, u32 shift, u32 val) > +int __must_check mhi_write_reg_field(struct mhi_controller *mhi_cntrl, > + void __iomem *base, u32 offset, > + u32 mask, u32 shift, u32 val) > { > int ret; > u32 tmp; > > ret = mhi_read_reg(mhi_cntrl, base, offset, &tmp); > if (ret) > - return; > + return ret; > > tmp &= ~mask; > tmp |= (val << shift); > mhi_write_reg(mhi_cntrl, base, offset, tmp); > + > + return 0; > } > > void mhi_write_db(struct mhi_controller *mhi_cntrl, void __iomem *db_addr, > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c index 0a97262..13eacda 100644 --- a/drivers/bus/mhi/core/boot.c +++ b/drivers/bus/mhi/core/boot.c @@ -19,8 +19,8 @@ #include "internal.h" /* Setup RDDM vector table for RDDM transfer and program RXVEC */ -void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, - struct image_info *img_info) +int mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, + struct image_info *img_info) { struct mhi_buf *mhi_buf = img_info->mhi_buf; struct bhi_vec_entry *bhi_vec = img_info->bhi_vec; @@ -28,6 +28,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, struct device *dev = &mhi_cntrl->mhi_dev->dev; u32 sequence_id; unsigned int i; + int ret; for (i = 0; i < img_info->entries - 1; i++, mhi_buf++, bhi_vec++) { bhi_vec->dma_addr = mhi_buf->dma_addr; @@ -45,12 +46,17 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, mhi_write_reg(mhi_cntrl, base, BHIE_RXVECSIZE_OFFS, mhi_buf->len); sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_RXVECSTATUS_SEQNUM_BMSK); - mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS, - BHIE_RXVECDB_SEQNUM_BMSK, BHIE_RXVECDB_SEQNUM_SHFT, - sequence_id); + ret = mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS, + BHIE_RXVECDB_SEQNUM_BMSK, + BHIE_RXVECDB_SEQNUM_SHFT, sequence_id); + if (ret) { + dev_err(dev, "Failed to write sequence ID for BHIE_RXVECDB\n"); + return ret; + } dev_dbg(dev, "Address: %p and len: 0x%zx sequence: %u\n", &mhi_buf->dma_addr, mhi_buf->len, sequence_id); + return 0; } /* Collect RDDM buffer during kernel panic */ @@ -202,11 +208,14 @@ static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl, mhi_write_reg(mhi_cntrl, base, BHIE_TXVECSIZE_OFFS, mhi_buf->len); - mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS, - BHIE_TXVECDB_SEQNUM_BMSK, BHIE_TXVECDB_SEQNUM_SHFT, - sequence_id); + ret = mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS, + BHIE_TXVECDB_SEQNUM_BMSK, + BHIE_TXVECDB_SEQNUM_SHFT, sequence_id); read_unlock_bh(pm_lock); + if (ret) + return ret; + /* Wait for the image download to complete */ ret = wait_event_timeout(mhi_cntrl->state_event, MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state) || diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c index 5aaca6d..0917465 100644 --- a/drivers/bus/mhi/core/init.c +++ b/drivers/bus/mhi/core/init.c @@ -544,10 +544,15 @@ int mhi_init_mmio(struct mhi_controller *mhi_cntrl) mhi_cntrl->mhi_cmd[PRIMARY_CMD_RING].ring.db_addr = base + CRDB_LOWER; /* Write to MMIO registers */ - for (i = 0; reg_info[i].offset; i++) - mhi_write_reg_field(mhi_cntrl, base, reg_info[i].offset, - reg_info[i].mask, reg_info[i].shift, - reg_info[i].val); + for (i = 0; reg_info[i].offset; i++) { + ret = mhi_write_reg_field(mhi_cntrl, base, reg_info[i].offset, + reg_info[i].mask, reg_info[i].shift, + reg_info[i].val); + if (ret) { + dev_err(dev, "Unable to write to MMIO registers"); + return ret; + } + } return 0; } @@ -1118,8 +1123,15 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl) */ 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); + if (mhi_cntrl->rddm_image) { + ret = mhi_rddm_prepare(mhi_cntrl, + mhi_cntrl->rddm_image); + if (ret) { + mhi_free_bhie_table(mhi_cntrl, + mhi_cntrl->rddm_image); + goto error_reg_offset; + } + } } mutex_unlock(&mhi_cntrl->pm_mutex); diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h index 721739c..3d17ec3 100644 --- a/drivers/bus/mhi/core/internal.h +++ b/drivers/bus/mhi/core/internal.h @@ -663,8 +663,9 @@ int __must_check mhi_poll_reg_field(struct mhi_controller *mhi_cntrl, u32 shift, u32 val, u32 delayus); void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base, u32 offset, u32 val); -void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base, - u32 offset, u32 mask, u32 shift, u32 val); +int __must_check mhi_write_reg_field(struct mhi_controller *mhi_cntrl, + void __iomem *base, u32 offset, + u32 mask, u32 shift, u32 val); void mhi_ring_er_db(struct mhi_event *mhi_event); void mhi_write_db(struct mhi_controller *mhi_cntrl, void __iomem *db_addr, dma_addr_t db_val); @@ -678,7 +679,7 @@ int mhi_init_dev_ctxt(struct mhi_controller *mhi_cntrl); void mhi_deinit_dev_ctxt(struct mhi_controller *mhi_cntrl); int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl); void mhi_deinit_free_irq(struct mhi_controller *mhi_cntrl); -void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, +int mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, struct image_info *img_info); void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl); int mhi_prepare_channel(struct mhi_controller *mhi_cntrl, diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index c01ec2f..902d854 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -66,19 +66,22 @@ void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base, mhi_cntrl->write_reg(mhi_cntrl, base + offset, val); } -void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base, - u32 offset, u32 mask, u32 shift, u32 val) +int __must_check mhi_write_reg_field(struct mhi_controller *mhi_cntrl, + void __iomem *base, u32 offset, + u32 mask, u32 shift, u32 val) { int ret; u32 tmp; ret = mhi_read_reg(mhi_cntrl, base, offset, &tmp); if (ret) - return; + return ret; tmp &= ~mask; tmp |= (val << shift); mhi_write_reg(mhi_cntrl, base, offset, tmp); + + return 0; } void mhi_write_db(struct mhi_controller *mhi_cntrl, void __iomem *db_addr,
Helper API to write register fields relies on successful reads of the register/address prior to the write. Bail out if a failure is seen when reading the register before the actual write is performed. Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> --- drivers/bus/mhi/core/boot.c | 25 +++++++++++++++++-------- drivers/bus/mhi/core/init.c | 24 ++++++++++++++++++------ drivers/bus/mhi/core/internal.h | 7 ++++--- drivers/bus/mhi/core/main.c | 9 ++++++--- 4 files changed, 45 insertions(+), 20 deletions(-)