[PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Mukesh Kumar Savaliya]

I2C driver currently reports "DMA txn failed" error even though it's
NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
on the bus instead of generic transfer failure which doesn't give any
specific clue.

Make Changes inside i2c driver callback handler function
i2c_gpi_cb_result() to parse these errors and make sure GSI driver
stores the error status during error interrupt.

Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
Co-developed-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
---
v2 -> v3:
- Modifed commit log reflecting an imperative mood.

v1 -> v2:
- Commit log changed we->We.
- Explained the problem that we are not detecing NACK error.
- Removed Heap based memory allocation and hence memory leakage issue.
- Used FIELD_GET and removed shiting and masking every time as suggested by Bjorn.
- Changed commit log to reflect the code changes done.
- Removed adding anything into struct gpi_i2c_config and created new structure
  for error status as suggested by Bjorn.
---

 drivers/dma/qcom/gpi.c             | 12 +++++++++++-
 drivers/i2c/busses/i2c-qcom-geni.c | 19 +++++++++++++++----
 include/linux/dma/qcom-gpi-dma.h   | 10 ++++++++++
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index 1c93864e0e4d..e3508d51fdc9 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -1076,7 +1076,17 @@ static void gpi_process_xfer_compl_event(struct gchan *gchan,
 	dev_dbg(gpii->gpi_dev->dev, "Residue %d\n", result.residue);
 
 	dma_cookie_complete(&vd->tx);
-	dmaengine_desc_get_callback_invoke(&vd->tx, &result);
+	if (gchan->protocol == QCOM_GPI_I2C) {
+		struct dmaengine_desc_callback cb;
+		struct gpi_i2c_result *i2c;
+
+		dmaengine_desc_get_callback(&vd->tx, &cb);
+		i2c = cb.callback_param;
+		i2c->status = compl_event->status;
+		dmaengine_desc_callback_invoke(&cb, &result);
+	} else {
+		dmaengine_desc_get_callback_invoke(&vd->tx, &result);
+	}
 
 gpi_free_desc:
 	spin_lock_irqsave(&gchan->vc.lock, flags);
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index da94df466e83..36a7c0c0ff54 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -66,6 +66,7 @@ enum geni_i2c_err_code {
 	GENI_TIMEOUT,
 };
 
+#define I2C_DMA_TX_IRQ_MASK	GENMASK(12, 5)
 #define DM_I2C_CB_ERR		((BIT(NACK) | BIT(BUS_PROTO) | BIT(ARB_LOST)) \
 									<< 5)
 
@@ -99,6 +100,7 @@ struct geni_i2c_dev {
 	struct dma_chan *rx_c;
 	bool gpi_mode;
 	bool abort_done;
+	struct gpi_i2c_result i2c_result;
 };
 
 struct geni_i2c_desc {
@@ -484,9 +486,18 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 
 static void i2c_gpi_cb_result(void *cb, const struct dmaengine_result *result)
 {
-	struct geni_i2c_dev *gi2c = cb;
-
-	if (result->result != DMA_TRANS_NOERROR) {
+	struct gpi_i2c_result *i2c_res = cb;
+	struct geni_i2c_dev *gi2c = container_of(i2c_res, struct geni_i2c_dev, i2c_result);
+	u32 status;
+
+	status = FIELD_GET(I2C_DMA_TX_IRQ_MASK, i2c_res->status);
+	if (status == BIT(NACK)) {
+		geni_i2c_err(gi2c, NACK);
+	} else if (status == BIT(BUS_PROTO)) {
+		geni_i2c_err(gi2c, BUS_PROTO);
+	} else if (status == BIT(ARB_LOST)) {
+		geni_i2c_err(gi2c, ARB_LOST);
+	} else if (result->result != DMA_TRANS_NOERROR) {
 		dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result);
 		gi2c->err = -EIO;
 	} else if (result->residue) {
@@ -568,7 +579,7 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	}
 
 	desc->callback_result = i2c_gpi_cb_result;
-	desc->callback_param = gi2c;
+	desc->callback_param = &gi2c->i2c_result;
 
 	dmaengine_submit(desc);
 	*buf = dma_buf;
diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
index 6680dd1a43c6..f585c6a35e51 100644
--- a/include/linux/dma/qcom-gpi-dma.h
+++ b/include/linux/dma/qcom-gpi-dma.h
@@ -80,4 +80,14 @@ struct gpi_i2c_config {
 	bool multi_msg;
 };
 
+/**
+ * struct gpi_i2c_result - i2c transfer status result in GSI mode
+ *
+ * @status: store txfer status value as part of callback
+ *
+ */
+struct gpi_i2c_result {
+	u32 status;
+};
+
 #endif /* QCOM_GPI_DMA_H */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Dmitry Baryshkov]

On Thu, 7 Mar 2024 at 22:56, Mukesh Kumar Savaliya
<quic_msavaliy@quicinc.com> wrote:
>
> I2C driver currently reports "DMA txn failed" error even though it's
> NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> on the bus instead of generic transfer failure which doesn't give any
> specific clue.
>
> Make Changes inside i2c driver callback handler function
> i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> stores the error status during error interrupt.
>
> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
> Co-developed-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
> ---
> v2 -> v3:
> - Modifed commit log reflecting an imperative mood.
>
> v1 -> v2:
> - Commit log changed we->We.
> - Explained the problem that we are not detecing NACK error.
> - Removed Heap based memory allocation and hence memory leakage issue.
> - Used FIELD_GET and removed shiting and masking every time as suggested by Bjorn.
> - Changed commit log to reflect the code changes done.
> - Removed adding anything into struct gpi_i2c_config and created new structure
>   for error status as suggested by Bjorn.
> ---
>
>  drivers/dma/qcom/gpi.c             | 12 +++++++++++-
>  drivers/i2c/busses/i2c-qcom-geni.c | 19 +++++++++++++++----
>  include/linux/dma/qcom-gpi-dma.h   | 10 ++++++++++
>  3 files changed, 36 insertions(+), 5 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>



-- 
With best wishes
Dmitry

Re: [PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Andi Shyti]

Hi Mukesh,

On Fri, Mar 08, 2024 at 02:25:39AM +0530, Mukesh Kumar Savaliya wrote:
> I2C driver currently reports "DMA txn failed" error even though it's
> NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> on the bus instead of generic transfer failure which doesn't give any
> specific clue.
> 
> Make Changes inside i2c driver callback handler function
> i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> stores the error status during error interrupt.

funny note: this is half "imperative mood" :-)

A real imperative would be "Pares the error inside
i2c_gpi_cb_result()... blah blah blah". No need to resend.

> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")

I still don't understand what's the fix here. You are making a
generic DMA error to be more specific... where is the bug? What
exactly is broken now?

Besides, keep in mind, that commits with fixes tags get
backported to older kernels (this one dates back to 5.18) and you
should also Cc the stable mailing list:

Cc: <stable@vger.kernel.org> # v5.18+

Thanks,
Andi

Re: [PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Mukesh Kumar Savaliya]

On 3/8/2024 4:28 AM, Andi Shyti wrote:
> Hi Mukesh,
> 
> On Fri, Mar 08, 2024 at 02:25:39AM +0530, Mukesh Kumar Savaliya wrote:
>> I2C driver currently reports "DMA txn failed" error even though it's
>> NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
>> on the bus instead of generic transfer failure which doesn't give any
>> specific clue.
>>
>> Make Changes inside i2c driver callback handler function
>> i2c_gpi_cb_result() to parse these errors and make sure GSI driver
>> stores the error status during error interrupt.
> 
> funny note: this is half "imperative mood" :-)
> 
> A real imperative would be "Pares the error inside
> i2c_gpi_cb_result()... blah blah blah". No need to resend.
> 

  :-) Thanks !

>> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
> 
> I still don't understand what's the fix here. You are making a
> generic DMA error to be more specific... where is the bug? What
> exactly is broken now?
> 
This is about being particular while reporting specific error.
Like i mentioned, instead of generic DMA transfer error, it should be 
particular error 1) NACK 2) BUT_PROTO 3)ARB_LOST.
Ofcourse when data transfer via DMA fails, it can be considered as
DMA Txfer fail.
In summary so far driver was considering all failure as txfer failure,
but i2c has errors which are kind of response/condition on the bus.

Sorry if it confusing still, but please let me know if anything required 
to be updated in  commit log which can bring clarity.

> Besides, keep in mind, that commits with fixes tags get
> backported to older kernels (this one dates back to 5.18) and you
> should also Cc the stable mailing list:
> 
> Cc: <stable@vger.kernel.org> # v5.18+

Sure, will add into CC. was waiting for reviewed-by tag.

> 
> Thanks,
> Andi

Re: [PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Andi Shyti]

Hi Mukesh,

...

> > > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
> > 
> > I still don't understand what's the fix here. You are making a
> > generic DMA error to be more specific... where is the bug? What
> > exactly is broken now?
> > 
> This is about being particular while reporting specific error.
> Like i mentioned, instead of generic DMA transfer error, it should be
> particular error 1) NACK 2) BUT_PROTO 3)ARB_LOST.
> Ofcourse when data transfer via DMA fails, it can be considered as
> DMA Txfer fail.
> In summary so far driver was considering all failure as txfer failure,
> but i2c has errors which are kind of response/condition on the bus.

I understand that, but what I need to know is: does the system
crash? does the system act in unexpected way?

Moving from "you received an error" to "you received a nack" is
not a fix, it's an improvement and it should not have the Fixes
tag.

Having the Fixes tag decides which path this patch will take to
to reach upstream. It's important because after it gets to
upstream other people will take your patch and backport it older
kernels.

I want to avoid this extra work when not necessary.

> Sorry if it confusing still, but please let me know if anything required to
> be updated in  commit log which can bring clarity.
> 
> > Besides, keep in mind, that commits with fixes tags get
> > backported to older kernels (this one dates back to 5.18) and you
> > should also Cc the stable mailing list:
> > 
> > Cc: <stable@vger.kernel.org> # v5.18+
> 
> Sure, will add into CC. was waiting for reviewed-by tag.

No need to resend.

Thanks,
Andi

Re: [PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Mukesh Kumar Savaliya]

On 3/8/2024 12:32 PM, Andi Shyti wrote:
> Hi Mukesh,
> 
> ...
> 
>>>> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
>>>
>>> I still don't understand what's the fix here. You are making a
>>> generic DMA error to be more specific... where is the bug? What
>>> exactly is broken now?
>>>
>> This is about being particular while reporting specific error.
>> Like i mentioned, instead of generic DMA transfer error, it should be
>> particular error 1) NACK 2) BUT_PROTO 3)ARB_LOST.
>> Ofcourse when data transfer via DMA fails, it can be considered as
>> DMA Txfer fail.
>> In summary so far driver was considering all failure as txfer failure,
>> but i2c has errors which are kind of response/condition on the bus.
> 
> I understand that, but what I need to know is: does the system
> crash? does the system act in unexpected way?
> 
> Moving from "you received an error" to "you received a nack" is
> not a fix, it's an improvement and it should not have the Fixes
> tag.
> 
> Having the Fixes tag decides which path this patch will take to
> to reach upstream. It's important because after it gets to
> upstream other people will take your patch and backport it older
> kernels.
> 
> I want to avoid this extra work when not necessary.
> 

Sure, then i think i should be removing fixes tag. It's not a crash but
it's an improvement. That being said, i think don't need to CC stable 
kernel list and i should remove fixes tag ?

>> Sorry if it confusing still, but please let me know if anything required to
>> be updated in  commit log which can bring clarity.
>>
>>> Besides, keep in mind, that commits with fixes tags get
>>> backported to older kernels (this one dates back to 5.18) and you
>>> should also Cc the stable mailing list:
>>>
>>> Cc: <stable@vger.kernel.org> # v5.18+
>>
>> Sure, will add into CC. was waiting for reviewed-by tag.
> 
> No need to resend.

ok, sure.

> 
> Thanks,
> Andi

Re: [PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Andi Shyti]

Hi Mukesh,

> > > > > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
> > > > 
> > > > I still don't understand what's the fix here. You are making a
> > > > generic DMA error to be more specific... where is the bug? What
> > > > exactly is broken now?
> > > > 
> > > This is about being particular while reporting specific error.
> > > Like i mentioned, instead of generic DMA transfer error, it should be
> > > particular error 1) NACK 2) BUT_PROTO 3)ARB_LOST.
> > > Ofcourse when data transfer via DMA fails, it can be considered as
> > > DMA Txfer fail.
> > > In summary so far driver was considering all failure as txfer failure,
> > > but i2c has errors which are kind of response/condition on the bus.
> > 
> > I understand that, but what I need to know is: does the system
> > crash? does the system act in unexpected way?
> > 
> > Moving from "you received an error" to "you received a nack" is
> > not a fix, it's an improvement and it should not have the Fixes
> > tag.
> > 
> > Having the Fixes tag decides which path this patch will take to
> > to reach upstream. It's important because after it gets to
> > upstream other people will take your patch and backport it older
> > kernels.
> > 
> > I want to avoid this extra work when not necessary.
> > 
> 
> Sure, then i think i should be removing fixes tag. It's not a crash but
> it's an improvement. That being said, i think don't need to CC stable kernel
> list and i should remove fixes tag ?

yes, don't need to do anything else, I will take care of
everything from now on. If Wolfram accepts a last minute pull
request, I can queue this up for 6.9.

Thank you,
Andi

Re: [PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Andi Shyti]

Hi

On Fri, 08 Mar 2024 02:25:39 +0530, Mukesh Kumar Savaliya wrote:
> I2C driver currently reports "DMA txn failed" error even though it's
> NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> on the bus instead of generic transfer failure which doesn't give any
> specific clue.
> 
> Make Changes inside i2c driver callback handler function
> i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> stores the error status during error interrupt.
> 
> [...]

Applied to i2c/i2c-host on

git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git

Thank you,
Andi

Patches applied
===============
[1/1] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode
      commit: 313d6aa4c64875ed8a10339a2db8766f49108efb

Re: [PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Wolfram Sang]

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


> On Fri, 08 Mar 2024 02:25:39 +0530, Mukesh Kumar Savaliya wrote:
> > I2C driver currently reports "DMA txn failed" error even though it's
> > NACK OR BUS_PROTO OR ARB_LOST. Detect NACK error when no device ACKs
> > on the bus instead of generic transfer failure which doesn't give any
> > specific clue.
> > 
> > Make Changes inside i2c driver callback handler function
> > i2c_gpi_cb_result() to parse these errors and make sure GSI driver
> > stores the error status during error interrupt.
> > 
> > [...]
> 
> Applied to i2c/i2c-host on
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git

Because this patch touches a file in the DMA realm, we should have an
ack here from one of the maintainers. So they know and are okay with us
changing something in their area.

$ scripts/get_maintainer.pl -f drivers/dma/qcom/gpi.c
Bjorn Andersson <andersson@kernel.org>
Konrad Dybcio <konrad.dybcio@linaro.org>
Vinod Koul <vkoul@kernel.org>
linux-arm-msm@vger.kernel.org
dmaengine@vger.kernel.org
linux-kernel@vger.kernel.org


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

Re: [PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Andi Shyti]

Hi Mukesh,

> +	status = FIELD_GET(I2C_DMA_TX_IRQ_MASK, i2c_res->status);

This fails here:

drivers/i2c/busses/i2c-qcom-geni.c: In function 'i2c_gpi_cb_result':
drivers/i2c/busses/i2c-qcom-geni.c:493:18: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
  493 |         status = FIELD_GET(I2C_DMA_TX_IRQ_MASK, i2c_res->status);
      |                  ^~~~~~~~~
cc1: all warnings being treated as errors

I will remove this patch from the i2c/i2c-host and we will need
to wait for the next merge window to get this through.

Please submit v4 with the Cc list recommended by Wolfram.

Thanks,
Andi

Re: [PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Mukesh Kumar Savaliya]

Hi Andi, Wolfram,

On 3/12/2024 4:19 PM, Andi Shyti wrote:
> Hi Mukesh,
> 
>> +	status = FIELD_GET(I2C_DMA_TX_IRQ_MASK, i2c_res->status);
> 
> This fails here:
> 
> drivers/i2c/busses/i2c-qcom-geni.c: In function 'i2c_gpi_cb_result':
> drivers/i2c/busses/i2c-qcom-geni.c:493:18: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
>    493 |         status = FIELD_GET(I2C_DMA_TX_IRQ_MASK, i2c_res->status);
>        |                  ^~~~~~~~~
> cc1: all warnings being treated as errors
> 
> I will remove this patch from the i2c/i2c-host and we will need
> to wait for the next merge window to get this through.
> 
> Please submit v4 with the Cc list recommended by Wolfram.
>Submitted V4 patch. Let me wait for the dmaengine maintainers to sig off 
too. Sorry for the trouble.

> Thanks,
> Andi
> 

Re: [PATCH v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode

[Andi Shyti]

Hi Mukesh,

> > > +	status = FIELD_GET(I2C_DMA_TX_IRQ_MASK, i2c_res->status);
> > 
> > This fails here:
> > 
> > drivers/i2c/busses/i2c-qcom-geni.c: In function 'i2c_gpi_cb_result':
> > drivers/i2c/busses/i2c-qcom-geni.c:493:18: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
> >    493 |         status = FIELD_GET(I2C_DMA_TX_IRQ_MASK, i2c_res->status);
> >        |                  ^~~~~~~~~
> > cc1: all warnings being treated as errors
> > 
> > I will remove this patch from the i2c/i2c-host and we will need
> > to wait for the next merge window to get this through.
> > 
> > Please submit v4 with the Cc list recommended by Wolfram.
> > Submitted V4 patch. Let me wait for the dmaengine maintainers to sig off
> too. Sorry for the trouble.

Please, don't be sorry... when something goes wrong it's a shared
responsibility.

But fortunately nothing went really wrong here because Stephen
cought it in time :-)

I missed this failure because in the testing config the qcom-geni
was missing.

Thanks for the prompt fix!

Andi