[PATCH V5 0/3] Implement Shutdown callback for geni-i2c

[PATCH V5 0/3] Implement Shutdown callback for geni-i2c

[Roja Rani Yarubandi]

 - As per Stephen's comments prepared separate patches for rx/tx
   transfer cleanup and shutdown callback

Roja Rani Yarubandi (3):
  soc: qcom: geni: Remove "iova" check
  i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct
  i2c: i2c-qcom-geni: Add shutdown callback for i2c

 drivers/i2c/busses/i2c-qcom-geni.c | 101 ++++++++++++++++++++++++-----
 drivers/soc/qcom/qcom-geni-se.c    |   4 +-
 2 files changed, 87 insertions(+), 18 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH V5 1/3] soc: qcom: geni: Remove "iova" check

[Roja Rani Yarubandi]

Remove "iova" check from geni_se_tx_dma_unprep and geni_se_rx_dma_unprep
fucntions as invalidating with dma_mapping_error() is enough.

Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
---
Changes in V5:
 - This is newly added patch in V5. As per Stephen's comments separted
   this patch from shutdown callback patch.

 drivers/soc/qcom/qcom-geni-se.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index d0e4f520cff8..0216b38c1e9a 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -705,7 +705,7 @@ void geni_se_tx_dma_unprep(struct geni_se *se, dma_addr_t iova, size_t len)
 {
 	struct geni_wrapper *wrapper = se->wrapper;
 
-	if (iova && !dma_mapping_error(wrapper->dev, iova))
+	if (!dma_mapping_error(wrapper->dev, iova))
 		dma_unmap_single(wrapper->dev, iova, len, DMA_TO_DEVICE);
 }
 EXPORT_SYMBOL(geni_se_tx_dma_unprep);
@@ -722,7 +722,7 @@ void geni_se_rx_dma_unprep(struct geni_se *se, dma_addr_t iova, size_t len)
 {
 	struct geni_wrapper *wrapper = se->wrapper;
 
-	if (iova && !dma_mapping_error(wrapper->dev, iova))
+	if (!dma_mapping_error(wrapper->dev, iova))
 		dma_unmap_single(wrapper->dev, iova, len, DMA_FROM_DEVICE);
 }
 EXPORT_SYMBOL(geni_se_rx_dma_unprep);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH V5 2/3] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct

[Roja Rani Yarubandi]

Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping
data scope. For example during shutdown callback to unmap DMA mapping,
this stored DMA mapping data can be used to call geni_se_tx_dma_unprep
and geni_se_rx_dma_unprep functions.

Add two helper functions geni_i2c_rx_msg_cleanup and
geni_i2c_tx_msg_cleanup to unwrap the things after rx/tx FIFO/DMA
transfers, so that the same can be used in geni_i2c_stop_xfer()
function during shutdown callback.

Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
---
Changes in V5:
 - As per Stephen's comments separated this patch from shutdown
   callback patch, gi2c->cur = NULL is not removed from 
   geni_i2c_abort_xfer(), and made a copy of gi2c->cur and passed
   to cleanup functions.

 drivers/i2c/busses/i2c-qcom-geni.c | 63 ++++++++++++++++++++++--------
 1 file changed, 47 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index dead5db3315a..aee2a1dd2c62 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -86,6 +86,9 @@ struct geni_i2c_dev {
 	u32 clk_freq_out;
 	const struct geni_i2c_clk_fld *clk_fld;
 	int suspended;
+	void *dma_buf;
+	size_t xfer_len;
+	dma_addr_t dma_addr;
 };
 
 struct geni_i2c_err_log {
@@ -349,14 +352,43 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)
 		dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n");
 }
 
+static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c,
+				     struct i2c_msg *cur)
+{
+	struct geni_se *se = &gi2c->se;
+
+	gi2c->cur_rd = 0;
+	if (gi2c->dma_buf) {
+		if (gi2c->err)
+			geni_i2c_rx_fsm_rst(gi2c);
+		geni_se_rx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len);
+		i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err);
+	}
+}
+
+static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c,
+				     struct i2c_msg *cur)
+{
+	struct geni_se *se = &gi2c->se;
+
+	gi2c->cur_wr = 0;
+	if (gi2c->dma_buf) {
+		if (gi2c->err)
+			geni_i2c_tx_fsm_rst(gi2c);
+		geni_se_tx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len);
+		i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err);
+	}
+}
+
 static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 				u32 m_param)
 {
-	dma_addr_t rx_dma;
+	dma_addr_t rx_dma = 0;
 	unsigned long time_left;
 	void *dma_buf = NULL;
 	struct geni_se *se = &gi2c->se;
 	size_t len = msg->len;
+	struct i2c_msg *cur;
 
 	if (!of_machine_is_compatible("lenovo,yoga-c630"))
 		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
@@ -372,21 +404,20 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 		geni_se_select_mode(se, GENI_SE_FIFO);
 		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
 		dma_buf = NULL;
+	} else {
+		gi2c->xfer_len = len;
+		gi2c->dma_addr = rx_dma;
+		gi2c->dma_buf = dma_buf;
 	}
 
 	geni_se_setup_m_cmd(se, I2C_READ, m_param);
 
+	cur = gi2c->cur;
 	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
 	if (!time_left)
 		geni_i2c_abort_xfer(gi2c);
 
-	gi2c->cur_rd = 0;
-	if (dma_buf) {
-		if (gi2c->err)
-			geni_i2c_rx_fsm_rst(gi2c);
-		geni_se_rx_dma_unprep(se, rx_dma, len);
-		i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
-	}
+	geni_i2c_rx_msg_cleanup(gi2c, cur);
 
 	return gi2c->err;
 }
@@ -394,11 +425,12 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 				u32 m_param)
 {
-	dma_addr_t tx_dma;
+	dma_addr_t tx_dma = 0;
 	unsigned long time_left;
 	void *dma_buf = NULL;
 	struct geni_se *se = &gi2c->se;
 	size_t len = msg->len;
+	struct i2c_msg *cur;
 
 	if (!of_machine_is_compatible("lenovo,yoga-c630"))
 		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
@@ -414,6 +446,10 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 		geni_se_select_mode(se, GENI_SE_FIFO);
 		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
 		dma_buf = NULL;
+	} else {
+		gi2c->xfer_len = len;
+		gi2c->dma_addr = tx_dma;
+		gi2c->dma_buf = dma_buf;
 	}
 
 	geni_se_setup_m_cmd(se, I2C_WRITE, m_param);
@@ -421,17 +457,12 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	if (!dma_buf) /* Get FIFO IRQ */
 		writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG);
 
+	cur = gi2c->cur;
 	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
 	if (!time_left)
 		geni_i2c_abort_xfer(gi2c);
 
-	gi2c->cur_wr = 0;
-	if (dma_buf) {
-		if (gi2c->err)
-			geni_i2c_tx_fsm_rst(gi2c);
-		geni_se_tx_dma_unprep(se, tx_dma, len);
-		i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
-	}
+	geni_i2c_tx_msg_cleanup(gi2c, cur);
 
 	return gi2c->err;
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH V5 3/3] i2c: i2c-qcom-geni: Add shutdown callback for i2c

[Roja Rani Yarubandi]

If the hardware is still accessing memory after SMMU translation
is disabled (as part of smmu shutdown callback), then the
IOVAs (I/O virtual address) which it was using will go on the bus
as the physical addresses which will result in unknown crashes
like NoC/interconnect errors.

So, implement shutdown callback to i2c driver to stop on-going transfer
and unmap DMA mappings during system "reboot" or "shutdown".

Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller")
Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
---
Changes in V2:
 - As per Stephen's comments added seperate function for stop transfer,
   fixed minor nitpicks.
 - As per Stephen's comments, changed commit text.

Changes in V3:
 - As per Stephen's comments, squashed patch 1 into patch 2, added Fixes tag.
 - As per Akash's comments, included FIFO case in stop_xfer, fixed minor nitpicks.

Changes in V4:
 - As per Stephen's comments cleaned up geni_i2c_stop_xfer function,
   added dma_buf in geni_i2c_dev struct to call i2c_put_dma_safe_msg_buf()
   from other functions, removed "iova" check in geni_se_rx_dma_unprep()
   and geni_se_tx_dma_unprep() functions.
 - Added two helper functions geni_i2c_rx_one_msg_done() and
   geni_i2c_tx_one_msg_done() to unwrap the things after rx/tx FIFO/DMA
   transfers, so that the same can be used in geni_i2c_stop_xfer() function
   during shutdown callback. Updated commit text accordingly.
 - Checking whether it is tx/rx transfer using I2C_M_RD which is valid for both
   FIFO and DMA cases, so dropped DMA_RX_ACTIVE and DMA_TX_ACTIVE bit checking

Changes in V5:
 - As per Stephen's comments, added spin_lock_irqsave & spin_unlock_irqsave in 
   geni_i2c_stop_xfer() function.

 drivers/i2c/busses/i2c-qcom-geni.c | 38 ++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index aee2a1dd2c62..56d3fbfe7eb6 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -380,6 +380,36 @@ static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c,
 	}
 }
 
+static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
+{
+	int ret;
+	u32 geni_status;
+	unsigned long flags;
+	struct i2c_msg *cur;
+
+	/* Resume device, runtime suspend can happen anytime during transfer */
+	ret = pm_runtime_get_sync(gi2c->se.dev);
+	if (ret < 0) {
+		dev_err(gi2c->se.dev, "Failed to resume device: %d\n", ret);
+		return;
+	}
+
+	spin_lock_irqsave(&gi2c->lock, flags);
+	geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
+	if (!(geni_status & M_GENI_CMD_ACTIVE))
+		goto out;
+
+	cur = gi2c->cur;
+	geni_i2c_abort_xfer(gi2c);
+	if (cur->flags & I2C_M_RD)
+		geni_i2c_rx_msg_cleanup(gi2c, cur);
+	else
+		geni_i2c_tx_msg_cleanup(gi2c, cur);
+	spin_unlock_irqrestore(&gi2c->lock, flags);
+out:
+	pm_runtime_put_sync_suspend(gi2c->se.dev);
+}
+
 static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 				u32 m_param)
 {
@@ -661,6 +691,13 @@ static int geni_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void  geni_i2c_shutdown(struct platform_device *pdev)
+{
+	struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
+
+	geni_i2c_stop_xfer(gi2c);
+}
+
 static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
 {
 	int ret;
@@ -725,6 +762,7 @@ MODULE_DEVICE_TABLE(of, geni_i2c_dt_match);
 static struct platform_driver geni_i2c_driver = {
 	.probe  = geni_i2c_probe,
 	.remove = geni_i2c_remove,
+	.shutdown = geni_i2c_shutdown,
 	.driver = {
 		.name = "geni_i2c",
 		.pm = &geni_i2c_pm_ops,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH V5 1/3] soc: qcom: geni: Remove "iova" check

[Stephen Boyd]

Quoting Roja Rani Yarubandi (2020-10-01 01:44:23)
> Remove "iova" check from geni_se_tx_dma_unprep and geni_se_rx_dma_unprep
> fucntions as invalidating with dma_mapping_error() is enough.

s/fucntions/functions/

also 

s/invalidating/checking/

> 
> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH V5 3/3] i2c: i2c-qcom-geni: Add shutdown callback for i2c

[Stephen Boyd]

Quoting Roja Rani Yarubandi (2020-10-01 01:44:25)
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index aee2a1dd2c62..56d3fbfe7eb6 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -380,6 +380,36 @@ static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c,
>         }
>  }
>  
> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
> +{
> +       int ret;
> +       u32 geni_status;
> +       unsigned long flags;
> +       struct i2c_msg *cur;
> +
> +       /* Resume device, runtime suspend can happen anytime during transfer */
> +       ret = pm_runtime_get_sync(gi2c->se.dev);
> +       if (ret < 0) {
> +               dev_err(gi2c->se.dev, "Failed to resume device: %d\n", ret);
> +               return;
> +       }
> +
> +       spin_lock_irqsave(&gi2c->lock, flags);

We grab the lock here.

> +       geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
> +       if (!(geni_status & M_GENI_CMD_ACTIVE))
> +               goto out;
> +
> +       cur = gi2c->cur;
> +       geni_i2c_abort_xfer(gi2c);

But it looks like this function takes the lock again? Did you test this
with lockdep enabled? It should hang even without lockdep, so it seems
like this path of code has not been tested.

> +       if (cur->flags & I2C_M_RD)
> +               geni_i2c_rx_msg_cleanup(gi2c, cur);
> +       else
> +               geni_i2c_tx_msg_cleanup(gi2c, cur);
> +       spin_unlock_irqrestore(&gi2c->lock, flags);
> +out:
> +       pm_runtime_put_sync_suspend(gi2c->se.dev);
> +}
> +
>  static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>                                 u32 m_param)
>  {

Re: [PATCH V5 1/3] soc: qcom: geni: Remove "iova" check

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On Thu, Oct 01, 2020 at 02:14:23PM +0530, Roja Rani Yarubandi wrote:
> Remove "iova" check from geni_se_tx_dma_unprep and geni_se_rx_dma_unprep
> fucntions as invalidating with dma_mapping_error() is enough.
> 
> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>

Applied to for-next, thanks!

The other patches need updates, it seems.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V5 3/3] i2c: i2c-qcom-geni: Add shutdown callback for i2c

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 4216 bytes --]

Hi Roja,

url:    https://github.com/0day-ci/linux/commits/Roja-Rani-Yarubandi/Implement-Shutdown-callback-for-geni-i2c/20201001-164636 
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git  i2c/for-next
config: parisc-randconfig-s032-20201008 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross  -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.2-218-gc0e96d6d-dirty
        # https://github.com/0day-ci/linux/commit/d8cd18adc21493e099c0d562dfb53a817ca21926 
        git remote add linux-review https://github.com/0day-ci/linux 
        git fetch --no-tags linux-review Roja-Rani-Yarubandi/Implement-Shutdown-callback-for-geni-i2c/20201001-164636
        git checkout d8cd18adc21493e099c0d562dfb53a817ca21926
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

echo
echo "sparse warnings: (new ones prefixed by >>)"
echo
>> drivers/i2c/busses/i2c-qcom-geni.c:383:13: sparse: sparse: context imbalance in 'geni_i2c_stop_xfer' - different lock contexts for basic block

vim +/geni_i2c_stop_xfer +383 drivers/i2c/busses/i2c-qcom-geni.c

d8cd18adc21493e Roja Rani Yarubandi 2020-10-01 @383  static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  384  {
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  385  	int ret;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  386  	u32 geni_status;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  387  	unsigned long flags;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  388  	struct i2c_msg *cur;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  389  
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  390  	/* Resume device, runtime suspend can happen anytime during transfer */
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  391  	ret = pm_runtime_get_sync(gi2c->se.dev);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  392  	if (ret < 0) {
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  393  		dev_err(gi2c->se.dev, "Failed to resume device: %d\n", ret);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  394  		return;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  395  	}
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  396  
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  397  	spin_lock_irqsave(&gi2c->lock, flags);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  398  	geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  399  	if (!(geni_status & M_GENI_CMD_ACTIVE))
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  400  		goto out;


goto unlock;

d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  401  
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  402  	cur = gi2c->cur;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  403  	geni_i2c_abort_xfer(gi2c);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  404  	if (cur->flags & I2C_M_RD)
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  405  		geni_i2c_rx_msg_cleanup(gi2c, cur);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  406  	else
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  407  		geni_i2c_tx_msg_cleanup(gi2c, cur);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  408  	spin_unlock_irqrestore(&gi2c->lock, flags);
                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  409  out:
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  410  	pm_runtime_put_sync_suspend(gi2c->se.dev);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  411  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org 

_______________________________________________
kbuild mailing list -- kbuild(a)lists.01.org
To unsubscribe send an email to kbuild-leave(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33727 bytes --]

[kbuild] Re: [PATCH V5 3/3] i2c: i2c-qcom-geni: Add shutdown callback for i2c

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 4216 bytes --]

Hi Roja,

url:    https://github.com/0day-ci/linux/commits/Roja-Rani-Yarubandi/Implement-Shutdown-callback-for-geni-i2c/20201001-164636 
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git  i2c/for-next
config: parisc-randconfig-s032-20201008 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross  -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.2-218-gc0e96d6d-dirty
        # https://github.com/0day-ci/linux/commit/d8cd18adc21493e099c0d562dfb53a817ca21926 
        git remote add linux-review https://github.com/0day-ci/linux 
        git fetch --no-tags linux-review Roja-Rani-Yarubandi/Implement-Shutdown-callback-for-geni-i2c/20201001-164636
        git checkout d8cd18adc21493e099c0d562dfb53a817ca21926
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

echo
echo "sparse warnings: (new ones prefixed by >>)"
echo
>> drivers/i2c/busses/i2c-qcom-geni.c:383:13: sparse: sparse: context imbalance in 'geni_i2c_stop_xfer' - different lock contexts for basic block

vim +/geni_i2c_stop_xfer +383 drivers/i2c/busses/i2c-qcom-geni.c

d8cd18adc21493e Roja Rani Yarubandi 2020-10-01 @383  static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  384  {
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  385  	int ret;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  386  	u32 geni_status;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  387  	unsigned long flags;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  388  	struct i2c_msg *cur;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  389  
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  390  	/* Resume device, runtime suspend can happen anytime during transfer */
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  391  	ret = pm_runtime_get_sync(gi2c->se.dev);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  392  	if (ret < 0) {
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  393  		dev_err(gi2c->se.dev, "Failed to resume device: %d\n", ret);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  394  		return;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  395  	}
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  396  
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  397  	spin_lock_irqsave(&gi2c->lock, flags);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  398  	geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  399  	if (!(geni_status & M_GENI_CMD_ACTIVE))
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  400  		goto out;


goto unlock;

d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  401  
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  402  	cur = gi2c->cur;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  403  	geni_i2c_abort_xfer(gi2c);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  404  	if (cur->flags & I2C_M_RD)
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  405  		geni_i2c_rx_msg_cleanup(gi2c, cur);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  406  	else
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  407  		geni_i2c_tx_msg_cleanup(gi2c, cur);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  408  	spin_unlock_irqrestore(&gi2c->lock, flags);
                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  409  out:
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  410  	pm_runtime_put_sync_suspend(gi2c->se.dev);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  411  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org 

_______________________________________________
kbuild mailing list -- kbuild(a)lists.01.org
To unsubscribe send an email to kbuild-leave(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33727 bytes --]

Re: [PATCH V5 3/3] i2c: i2c-qcom-geni: Add shutdown callback for i2c

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4498 bytes --]

Hi Roja,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on driver-core/driver-core-testing linus/master v5.9-rc8 next-20201008]
[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/Roja-Rani-Yarubandi/Implement-Shutdown-callback-for-geni-i2c/20201001-164636
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
:::::: branch date: 7 days ago
:::::: commit date: 7 days ago
config: parisc-randconfig-s032-20201008 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.2-218-gc0e96d6d-dirty
        # https://github.com/0day-ci/linux/commit/d8cd18adc21493e099c0d562dfb53a817ca21926
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Roja-Rani-Yarubandi/Implement-Shutdown-callback-for-geni-i2c/20201001-164636
        git checkout d8cd18adc21493e099c0d562dfb53a817ca21926
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

echo
echo "sparse warnings: (new ones prefixed by >>)"
echo
>> drivers/i2c/busses/i2c-qcom-geni.c:383:13: sparse: sparse: context imbalance in 'geni_i2c_stop_xfer' - different lock contexts for basic block

vim +/geni_i2c_stop_xfer +383 drivers/i2c/busses/i2c-qcom-geni.c

34b404a7f63f37e Roja Rani Yarubandi 2020-10-01  382  
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01 @383  static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  384  {
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  385  	int ret;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  386  	u32 geni_status;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  387  	unsigned long flags;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  388  	struct i2c_msg *cur;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  389  
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  390  	/* Resume device, runtime suspend can happen anytime during transfer */
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  391  	ret = pm_runtime_get_sync(gi2c->se.dev);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  392  	if (ret < 0) {
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  393  		dev_err(gi2c->se.dev, "Failed to resume device: %d\n", ret);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  394  		return;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  395  	}
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  396  
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  397  	spin_lock_irqsave(&gi2c->lock, flags);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  398  	geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  399  	if (!(geni_status & M_GENI_CMD_ACTIVE))
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  400  		goto out;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  401  
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  402  	cur = gi2c->cur;
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  403  	geni_i2c_abort_xfer(gi2c);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  404  	if (cur->flags & I2C_M_RD)
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  405  		geni_i2c_rx_msg_cleanup(gi2c, cur);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  406  	else
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  407  		geni_i2c_tx_msg_cleanup(gi2c, cur);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  408  	spin_unlock_irqrestore(&gi2c->lock, flags);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  409  out:
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  410  	pm_runtime_put_sync_suspend(gi2c->se.dev);
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  411  }
d8cd18adc21493e Roja Rani Yarubandi 2020-10-01  412  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33727 bytes --]

Re: [PATCH V5 3/3] i2c: i2c-qcom-geni: Add shutdown callback for i2c

[rojay]

Hi Stephen,

On 2020-10-03 07:09, Stephen Boyd wrote:
> Quoting Roja Rani Yarubandi (2020-10-01 01:44:25)
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index aee2a1dd2c62..56d3fbfe7eb6 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -380,6 +380,36 @@ static void geni_i2c_tx_msg_cleanup(struct 
>> geni_i2c_dev *gi2c,
>>         }
>>  }
>> 
>> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> +       int ret;
>> +       u32 geni_status;
>> +       unsigned long flags;
>> +       struct i2c_msg *cur;
>> +
>> +       /* Resume device, runtime suspend can happen anytime during 
>> transfer */
>> +       ret = pm_runtime_get_sync(gi2c->se.dev);
>> +       if (ret < 0) {
>> +               dev_err(gi2c->se.dev, "Failed to resume device: %d\n", 
>> ret);
>> +               return;
>> +       }
>> +
>> +       spin_lock_irqsave(&gi2c->lock, flags);
> 
> We grab the lock here.
> 
>> +       geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
>> +       if (!(geni_status & M_GENI_CMD_ACTIVE))
>> +               goto out;
>> +
>> +       cur = gi2c->cur;
>> +       geni_i2c_abort_xfer(gi2c);
> 
> But it looks like this function takes the lock again? Did you test this
> with lockdep enabled? It should hang even without lockdep, so it seems
> like this path of code has not been tested.
> 

Tested with lockdep enabled, and fixed the unsafe lock order issue here.
And to be more proper moved spin_lock/unlock to cleanup functions.

>> +       if (cur->flags & I2C_M_RD)
>> +               geni_i2c_rx_msg_cleanup(gi2c, cur);
>> +       else
>> +               geni_i2c_tx_msg_cleanup(gi2c, cur);
>> +       spin_unlock_irqrestore(&gi2c->lock, flags);
>> +out:
>> +       pm_runtime_put_sync_suspend(gi2c->se.dev);
>> +}
>> +
>>  static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct 
>> i2c_msg *msg,
>>                                 u32 m_param)
>>  {