| Message ID | 20240307205539.217204-1-quic_msavaliy@quicinc.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | [v3] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI mode | expand |
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>
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
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
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
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
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
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
> 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
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
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 >
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
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 */
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(-)