Re: [PATCH] i2c: i2c-qcom-geni: Fix DMA transfer race

From: "Mukesh, Savaliya" <msavaliy@codeaurora.org>
To: Stephen Boyd <swboyd@chromium.org>,
	Douglas Anderson <dianders@chromium.org>,
	Wolfram Sang <wsa@the-dreams.de>
Cc: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	Akash Asthana <akashast@codeaurora.org>,
	Alok Chauhan <alokc@codeaurora.org>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Wolfram Sang <wsa@kernel.org>,
	linux-arm-msm@vger.kernel.org, linux-i2c@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] i2c: i2c-qcom-geni: Fix DMA transfer race
Date: Tue, 21 Jul 2020 16:28:11 +0530	[thread overview]
Message-ID: <6d90f383-54d7-cee1-d064-dca51ce39e14@codeaurora.org> (raw)
In-Reply-To: <159531527579.3847286.1254956818647049462@swboyd.mtv.corp.google.com>

On 7/21/2020 12:37 PM, Stephen Boyd wrote:
> Quoting Stephen Boyd (2020-07-20 22:59:14)
>> I worry that we also need a dmb() here to make sure the dma buffer is
>> properly mapped before this write to the device is attempted. But it may
>> only matter to be before the I2C_READ.
>>
> I'm suggesting this patch instead where we make geni_se_setup_m_cmd()
> use a writel() so that it has the proper barrier semantics to wait for
> the other memory writes that happened in program order before this point
> to complete before the device is kicked to do a read or a write.

Not sure if the issue was because of the barrier, but fundamentally for 
read operation, before FIFO data gets written by the DMA to memory,

buffer should be present. Hence the previous change from Doug seem to be 
fine as well.

> ----8<----
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 18d1e4fd4cf3..7f130829bf01 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -367,7 +367,6 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>   		geni_se_select_mode(se, GENI_SE_FIFO);
>   
>   	writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN);
> -	geni_se_setup_m_cmd(se, I2C_READ, m_param);
>   
>   	if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) {
>   		geni_se_select_mode(se, GENI_SE_FIFO);
> @@ -375,6 +374,8 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>   		dma_buf = NULL;
>   	}
>   
> +	geni_se_setup_m_cmd(se, I2C_READ, m_param);
> +
>   	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
>   	if (!time_left)
>   		geni_i2c_abort_xfer(gi2c);
> @@ -408,7 +409,6 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>   		geni_se_select_mode(se, GENI_SE_FIFO);
>   
>   	writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN);
> -	geni_se_setup_m_cmd(se, I2C_WRITE, m_param);
>   
>   	if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, &tx_dma)) {
>   		geni_se_select_mode(se, GENI_SE_FIFO);
> @@ -416,6 +416,8 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>   		dma_buf = NULL;
>   	}
>   
> +	geni_se_setup_m_cmd(se, I2C_WRITE, m_param);
> +
>   	if (!dma_buf) /* Get FIFO IRQ */
>   		writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG);
>   
> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> index dd464943f717..1dc134e9eb36 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -262,7 +262,7 @@ static inline void geni_se_setup_m_cmd(struct geni_se *se, u32 cmd, u32 params)
>   	u32 m_cmd;
>   
>   	m_cmd = (cmd << M_OPCODE_SHFT) | (params & M_PARAMS_MSK);
> -	writel_relaxed(m_cmd, se->base + SE_GENI_M_CMD0);
> +	writel(m_cmd, se->base + SE_GENI_M_CMD0);
>   }
>   
>   /**