we are seeing protocol errors like NACK as transfer failure but
ideally it should report exact error like NACK, BUS_PROTO or ARB_LOST.
Hence we are adding such error support in GSI mode and reporting it
accordingly by adding respective error logs.
geni_i2c_gpi_xfer() needed to allocate heap based memory instead of
stack memory to handle and store the geni_i2c_dev handle.
Copy event status from GSI driver to the i2c device status and parse
error when callback comes from gsi driver to the i2c driver. In the
gpi.c, we need to store callback param into i2c config data structure
so that inside the i2c driver, we can check what exactly the error is
and parse it accordingly.
Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
Co-developed-by: Viken Dadhaniya <[email protected]>
Signed-off-by: Viken Dadhaniya <[email protected]>
Signed-off-by: Mukesh Kumar Savaliya <[email protected]>
---
drivers/dma/qcom/gpi.c | 12 +++++++-
drivers/i2c/busses/i2c-qcom-geni.c | 46 +++++++++++++++++++-----------
include/linux/dma/qcom-gpi-dma.h | 4 +++
3 files changed, 44 insertions(+), 18 deletions(-)
diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index 1c93864e0e4d..6d718916fba4 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_config *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..5092d10e8f47 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -484,9 +484,16 @@ 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_config *i2c = cb;
+ struct geni_i2c_dev *gi2c = i2c->gi2c;
+
+ if (i2c->status & (BIT(NACK) << 5)) {
+ geni_i2c_err(gi2c, NACK);
+ } else if (i2c->status & (BIT(BUS_PROTO) << 5)) {
+ geni_i2c_err(gi2c, BUS_PROTO);
+ } else if (i2c->status & (BIT(ARB_LOST) << 5)) {
+ 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 +575,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 = peripheral;
dmaengine_submit(desc);
*buf = dma_buf;
@@ -585,33 +592,38 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], int num)
{
struct dma_slave_config config = {};
- struct gpi_i2c_config peripheral = {};
+ struct gpi_i2c_config *peripheral;
int i, ret = 0, timeout;
dma_addr_t tx_addr, rx_addr;
void *tx_buf = NULL, *rx_buf = NULL;
const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
- config.peripheral_config = &peripheral;
- config.peripheral_size = sizeof(peripheral);
+ peripheral = devm_kzalloc(gi2c->se.dev, sizeof(*peripheral), GFP_KERNEL);
+ if (!peripheral)
+ return -ENOMEM;
+
+ config.peripheral_config = peripheral;
+ config.peripheral_size = sizeof(struct gpi_i2c_config);
- peripheral.pack_enable = I2C_PACK_TX | I2C_PACK_RX;
- peripheral.cycle_count = itr->t_cycle_cnt;
- peripheral.high_count = itr->t_high_cnt;
- peripheral.low_count = itr->t_low_cnt;
- peripheral.clk_div = itr->clk_div;
- peripheral.set_config = 1;
- peripheral.multi_msg = false;
+ peripheral->gi2c = gi2c;
+ peripheral->pack_enable = I2C_PACK_TX | I2C_PACK_RX;
+ peripheral->cycle_count = itr->t_cycle_cnt;
+ peripheral->high_count = itr->t_high_cnt;
+ peripheral->low_count = itr->t_low_cnt;
+ peripheral->clk_div = itr->clk_div;
+ peripheral->set_config = 1;
+ peripheral->multi_msg = false;
for (i = 0; i < num; i++) {
gi2c->cur = &msgs[i];
gi2c->err = 0;
dev_dbg(gi2c->se.dev, "msg[%d].len:%d\n", i, gi2c->cur->len);
- peripheral.stretch = 0;
+ peripheral->stretch = 0;
if (i < num - 1)
- peripheral.stretch = 1;
+ peripheral->stretch = 1;
- peripheral.addr = msgs[i].addr;
+ peripheral->addr = msgs[i].addr;
ret = geni_i2c_gpi(gi2c, &msgs[i], &config,
&tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);
diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
index 6680dd1a43c6..af264f769344 100644
--- a/include/linux/dma/qcom-gpi-dma.h
+++ b/include/linux/dma/qcom-gpi-dma.h
@@ -64,6 +64,8 @@ enum i2c_op {
* @set_config: set peripheral config
* @rx_len: receive length for buffer
* @op: i2c cmd
+ * @status: stores gpi event status based on interrupt
+ * @gi2c: pointer to i2c device handle
* @muli-msg: is part of multi i2c r-w msgs
*/
struct gpi_i2c_config {
@@ -78,6 +80,8 @@ struct gpi_i2c_config {
u32 rx_len;
enum i2c_op op;
bool multi_msg;
+ u32 status;
+ struct geni_i2c_dev *gi2c;
};
#endif /* QCOM_GPI_DMA_H */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Fri, Mar 01, 2024 at 04:56:38PM +0530, Mukesh Kumar Savaliya wrote:
> we are seeing protocol errors like NACK as transfer failure but
> ideally it should report exact error like NACK, BUS_PROTO or ARB_LOST.
>
> Hence we are adding such error support in GSI mode and reporting it
> accordingly by adding respective error logs.
>
> geni_i2c_gpi_xfer() needed to allocate heap based memory instead of
> stack memory to handle and store the geni_i2c_dev handle.
>
> Copy event status from GSI driver to the i2c device status and parse
> error when callback comes from gsi driver to the i2c driver. In the
> gpi.c, we need to store callback param into i2c config data structure
> so that inside the i2c driver, we can check what exactly the error is
> and parse it accordingly.
>
> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
> Co-developed-by: Viken Dadhaniya <[email protected]>
> Signed-off-by: Viken Dadhaniya <[email protected]>
> Signed-off-by: Mukesh Kumar Savaliya <[email protected]>
> ---
> drivers/dma/qcom/gpi.c | 12 +++++++-
> drivers/i2c/busses/i2c-qcom-geni.c | 46 +++++++++++++++++++-----------
> include/linux/dma/qcom-gpi-dma.h | 4 +++
> 3 files changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index 1c93864e0e4d..6d718916fba4 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_config *i2c;
> +
> + dmaengine_desc_get_callback(&vd->tx, &cb);
> + i2c = cb.callback_param;
> + i2c->status = compl_event->status;
What would the DMA maintainer say about extending struct
dmaengine_tx_result with some protocol-specific status field?
> + 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..5092d10e8f47 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -484,9 +484,16 @@ 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_config *i2c = cb;
> + struct geni_i2c_dev *gi2c = i2c->gi2c;
> +
> + if (i2c->status & (BIT(NACK) << 5)) {
Wouldn't it be cleaner to:
status = FIELD_GET(SOME_MASK, i2c->status);
if (status == BIT(NACK)) {
..
Or can multiple of these be set? Still would like to see you extract the
field instead of having the shift in every single conditional.
> + geni_i2c_err(gi2c, NACK);
> + } else if (i2c->status & (BIT(BUS_PROTO) << 5)) {
> + geni_i2c_err(gi2c, BUS_PROTO);
> + } else if (i2c->status & (BIT(ARB_LOST) << 5)) {
> + 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 +575,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 = peripheral;
>
> dmaengine_submit(desc);
> *buf = dma_buf;
> @@ -585,33 +592,38 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], int num)
> {
> struct dma_slave_config config = {};
> - struct gpi_i2c_config peripheral = {};
> + struct gpi_i2c_config *peripheral;
> int i, ret = 0, timeout;
> dma_addr_t tx_addr, rx_addr;
> void *tx_buf = NULL, *rx_buf = NULL;
> const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>
> - config.peripheral_config = &peripheral;
> - config.peripheral_size = sizeof(peripheral);
> + peripheral = devm_kzalloc(gi2c->se.dev, sizeof(*peripheral), GFP_KERNEL);
This will be allocated for every transfer, and only freed when you
remove the geni bus driver, i.e. this is in practice a memory leak.
But do you really need to move this to the heap? If I understand the DMA
api, the callback will not be invoked after you exit this function, so
it should be fine to have it on the stack.
> + if (!peripheral)
> + return -ENOMEM;
> +
> + config.peripheral_config = peripheral;
> + config.peripheral_size = sizeof(struct gpi_i2c_config);
>
> - peripheral.pack_enable = I2C_PACK_TX | I2C_PACK_RX;
> - peripheral.cycle_count = itr->t_cycle_cnt;
> - peripheral.high_count = itr->t_high_cnt;
> - peripheral.low_count = itr->t_low_cnt;
> - peripheral.clk_div = itr->clk_div;
> - peripheral.set_config = 1;
> - peripheral.multi_msg = false;
> + peripheral->gi2c = gi2c;
> + peripheral->pack_enable = I2C_PACK_TX | I2C_PACK_RX;
> + peripheral->cycle_count = itr->t_cycle_cnt;
> + peripheral->high_count = itr->t_high_cnt;
> + peripheral->low_count = itr->t_low_cnt;
> + peripheral->clk_div = itr->clk_div;
> + peripheral->set_config = 1;
> + peripheral->multi_msg = false;
>
> for (i = 0; i < num; i++) {
> gi2c->cur = &msgs[i];
> gi2c->err = 0;
> dev_dbg(gi2c->se.dev, "msg[%d].len:%d\n", i, gi2c->cur->len);
>
> - peripheral.stretch = 0;
> + peripheral->stretch = 0;
> if (i < num - 1)
> - peripheral.stretch = 1;
> + peripheral->stretch = 1;
>
> - peripheral.addr = msgs[i].addr;
> + peripheral->addr = msgs[i].addr;
>
> ret = geni_i2c_gpi(gi2c, &msgs[i], &config,
> &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);
> diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
> index 6680dd1a43c6..af264f769344 100644
> --- a/include/linux/dma/qcom-gpi-dma.h
> +++ b/include/linux/dma/qcom-gpi-dma.h
> @@ -64,6 +64,8 @@ enum i2c_op {
> * @set_config: set peripheral config
> * @rx_len: receive length for buffer
> * @op: i2c cmd
> + * @status: stores gpi event status based on interrupt
> + * @gi2c: pointer to i2c device handle
The order doesn't match the struct below.
> * @muli-msg: is part of multi i2c r-w msgs
> */
> struct gpi_i2c_config {
> @@ -78,6 +80,8 @@ struct gpi_i2c_config {
> u32 rx_len;
> enum i2c_op op;
> bool multi_msg;
> + u32 status;
> + struct geni_i2c_dev *gi2c;
These two entries doesn't have anything to do with the "gpi i2c config",
just as the remainder of the properties has nothing to do with the "dma
callback".
Please split them off into their own structure.
Regards,
Bjorn
> };
>
> #endif /* QCOM_GPI_DMA_H */
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>
Hi Mukesh,
On Fri, Mar 01, 2024 at 04:56:38PM +0530, Mukesh Kumar Savaliya wrote:
> we are seeing protocol errors like NACK as transfer failure but
/we/We/
> ideally it should report exact error like NACK, BUS_PROTO or ARB_LOST.
>
> Hence we are adding such error support in GSI mode and reporting it
> accordingly by adding respective error logs.
>
> geni_i2c_gpi_xfer() needed to allocate heap based memory instead of
Please use the imperative form.
> stack memory to handle and store the geni_i2c_dev handle.
>
> Copy event status from GSI driver to the i2c device status and parse
> error when callback comes from gsi driver to the i2c driver. In the
> gpi.c, we need to store callback param into i2c config data structure
> so that inside the i2c driver, we can check what exactly the error is
> and parse it accordingly.
>
> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
What bug are you fixing here? The description doesn't talk about
fixes rather than support added.
..
> - config.peripheral_config = &peripheral;
> - config.peripheral_size = sizeof(peripheral);
> + peripheral = devm_kzalloc(gi2c->se.dev, sizeof(*peripheral), GFP_KERNEL);
This is a massive leak. Why are you deciding to make the
allocation dynamic?
Thanks,
Andi
Hi Mukesh,
(I'm sorry for the noise but my mail server has marked this mail
as spam and put the spam tag in front of the subject. Therefore,
my reply might have been marked as spam.)
I'm going to send a new e-mail with the old answers.
On Fri, Mar 01, 2024 at 04:56:38PM +0530, Mukesh Kumar Savaliya wrote:
> we are seeing protocol errors like NACK as transfer failure but
/we/We/
> ideally it should report exact error like NACK, BUS_PROTO or ARB_LOST.
>
> Hence we are adding such error support in GSI mode and reporting it
> accordingly by adding respective error logs.
>
> geni_i2c_gpi_xfer() needed to allocate heap based memory instead of
Please use the imperative form.
> stack memory to handle and store the geni_i2c_dev handle.
>
> Copy event status from GSI driver to the i2c device status and parse
> error when callback comes from gsi driver to the i2c driver. In the
> gpi.c, we need to store callback param into i2c config data structure
> so that inside the i2c driver, we can check what exactly the error is
> and parse it accordingly.
>
> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
What bug are you fixing here? The description doesn't talk about
fixes rather than support added.
..
> - config.peripheral_config = &peripheral;
> - config.peripheral_size = sizeof(peripheral);
> + peripheral = devm_kzalloc(gi2c->se.dev, sizeof(*peripheral), GFP_KERNEL);
> + if (!peripheral)
> + return -ENOMEM;
This is a massive leak. Why are you deciding to make the
allocation dynamic?
Thanks,
Andi
On 01-03-24, 10:37, Bjorn Andersson wrote:
> On Fri, Mar 01, 2024 at 04:56:38PM +0530, Mukesh Kumar Savaliya wrote:
> > we are seeing protocol errors like NACK as transfer failure but
> > ideally it should report exact error like NACK, BUS_PROTO or ARB_LOST.
> >
> > Hence we are adding such error support in GSI mode and reporting it
> > accordingly by adding respective error logs.
> >
> > geni_i2c_gpi_xfer() needed to allocate heap based memory instead of
> > stack memory to handle and store the geni_i2c_dev handle.
> >
> > Copy event status from GSI driver to the i2c device status and parse
> > error when callback comes from gsi driver to the i2c driver. In the
> > gpi.c, we need to store callback param into i2c config data structure
> > so that inside the i2c driver, we can check what exactly the error is
> > and parse it accordingly.
> >
> > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
> > Co-developed-by: Viken Dadhaniya <[email protected]>
> > Signed-off-by: Viken Dadhaniya <[email protected]>
> > Signed-off-by: Mukesh Kumar Savaliya <[email protected]>
> > ---
> > drivers/dma/qcom/gpi.c | 12 +++++++-
> > drivers/i2c/busses/i2c-qcom-geni.c | 46 +++++++++++++++++++-----------
> > include/linux/dma/qcom-gpi-dma.h | 4 +++
> > 3 files changed, 44 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> > index 1c93864e0e4d..6d718916fba4 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_config *i2c;
> > +
> > + dmaengine_desc_get_callback(&vd->tx, &cb);
> > + i2c = cb.callback_param;
> > + i2c->status = compl_event->status;
>
> What would the DMA maintainer say about extending struct
> dmaengine_tx_result with some protocol-specific status field?
That would be sane thing to do if we can get protocol status. Most of
the times DMA txn would tell you success or fail, here we have firmware
which tells us this and I see no reason why this can't be propagated.
--
~Vinod
On 3/3/2024 7:42 PM, Vinod Koul wrote:
> On 01-03-24, 10:37, Bjorn Andersson wrote:
>> On Fri, Mar 01, 2024 at 04:56:38PM +0530, Mukesh Kumar Savaliya wrote:
>>> we are seeing protocol errors like NACK as transfer failure but
>>> ideally it should report exact error like NACK, BUS_PROTO or ARB_LOST.
>>>
>>> Hence we are adding such error support in GSI mode and reporting it
>>> accordingly by adding respective error logs.
>>>
>>> geni_i2c_gpi_xfer() needed to allocate heap based memory instead of
>>> stack memory to handle and store the geni_i2c_dev handle.
>>>
>>> Copy event status from GSI driver to the i2c device status and parse
>>> error when callback comes from gsi driver to the i2c driver. In the
>>> gpi.c, we need to store callback param into i2c config data structure
>>> so that inside the i2c driver, we can check what exactly the error is
>>> and parse it accordingly.
>>>
>>> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
>>> Co-developed-by: Viken Dadhaniya <[email protected]>
>>> Signed-off-by: Viken Dadhaniya <[email protected]>
>>> Signed-off-by: Mukesh Kumar Savaliya <[email protected]>
>>> ---
>>> drivers/dma/qcom/gpi.c | 12 +++++++-
>>> drivers/i2c/busses/i2c-qcom-geni.c | 46 +++++++++++++++++++-----------
>>> include/linux/dma/qcom-gpi-dma.h | 4 +++
>>> 3 files changed, 44 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
>>> index 1c93864e0e4d..6d718916fba4 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_config *i2c;
>>> +
>>> + dmaengine_desc_get_callback(&vd->tx, &cb);
>>> + i2c = cb.callback_param;
>>> + i2c->status = compl_event->status;
>>
>> What would the DMA maintainer say about extending struct
>> dmaengine_tx_result with some protocol-specific status field?
>
> That would be sane thing to do if we can get protocol status. Most of
> the times DMA txn would tell you success or fail, here we have firmware
> which tells us this and I see no reason why this can't be propagated.
Thanks Vinod for reviewing the change. I have uploaded latest patch
considering other review comments.
>
On 3/1/2024 10:07 PM, Bjorn Andersson wrote:
> On Fri, Mar 01, 2024 at 04:56:38PM +0530, Mukesh Kumar Savaliya wrote:
>> we are seeing protocol errors like NACK as transfer failure but
>> ideally it should report exact error like NACK, BUS_PROTO or ARB_LOST.
>>
>> Hence we are adding such error support in GSI mode and reporting it
>> accordingly by adding respective error logs.
>>
>> geni_i2c_gpi_xfer() needed to allocate heap based memory instead of
>> stack memory to handle and store the geni_i2c_dev handle.
>>
>> Copy event status from GSI driver to the i2c device status and parse
>> error when callback comes from gsi driver to the i2c driver. In the
>> gpi.c, we need to store callback param into i2c config data structure
>> so that inside the i2c driver, we can check what exactly the error is
>> and parse it accordingly.
>>
>> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
>> Co-developed-by: Viken Dadhaniya <[email protected]>
>> Signed-off-by: Viken Dadhaniya <[email protected]>
>> Signed-off-by: Mukesh Kumar Savaliya <[email protected]>
>> ---
>> drivers/dma/qcom/gpi.c | 12 +++++++-
>> drivers/i2c/busses/i2c-qcom-geni.c | 46 +++++++++++++++++++-----------
>> include/linux/dma/qcom-gpi-dma.h | 4 +++
>> 3 files changed, 44 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
>> index 1c93864e0e4d..6d718916fba4 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_config *i2c;
>> +
>> + dmaengine_desc_get_callback(&vd->tx, &cb);
>> + i2c = cb.callback_param;
>> + i2c->status = compl_event->status;
>
> What would the DMA maintainer say about extending struct
> dmaengine_tx_result with some protocol-specific status field?
>
>> + 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..5092d10e8f47 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -484,9 +484,16 @@ 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_config *i2c = cb;
>> + struct geni_i2c_dev *gi2c = i2c->gi2c;
>> +
>> + if (i2c->status & (BIT(NACK) << 5)) {
>
> Wouldn't it be cleaner to:
>
> status = FIELD_GET(SOME_MASK, i2c->status);
> if (status == BIT(NACK)) {
> ...
>
> Or can multiple of these be set? Still would like to see you extract the
> field instead of having the shift in every single conditional.
>
Thanks Bjorn !
Yes, agreed to use FIELT_GET.
For these three error, Either one of the error would get reported.
>> + geni_i2c_err(gi2c, NACK);
>> + } else if (i2c->status & (BIT(BUS_PROTO) << 5)) {
>> + geni_i2c_err(gi2c, BUS_PROTO);
>> + } else if (i2c->status & (BIT(ARB_LOST) << 5)) {
>> + 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 +575,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 = peripheral;
>>
>> dmaengine_submit(desc);
>> *buf = dma_buf;
>> @@ -585,33 +592,38 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>> static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], int num)
>> {
>> struct dma_slave_config config = {};
>> - struct gpi_i2c_config peripheral = {};
>> + struct gpi_i2c_config *peripheral;
>> int i, ret = 0, timeout;
>> dma_addr_t tx_addr, rx_addr;
>> void *tx_buf = NULL, *rx_buf = NULL;
>> const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>>
>> - config.peripheral_config = &peripheral;
>> - config.peripheral_size = sizeof(peripheral);
>> + peripheral = devm_kzalloc(gi2c->se.dev, sizeof(*peripheral), GFP_KERNEL);
>
> This will be allocated for every transfer, and only freed when you
> remove the geni bus driver, i.e. this is in practice a memory leak.
>
>
> But do you really need to move this to the heap? If I understand the DMA
> api, the callback will not be invoked after you exit this function, so
> it should be fine to have it on the stack.
>
yes, I do Agree, its a memory leak. Removed heap memory allocation.
Next comment addresses this comment since i have removed heap
memory allocation and added a new structure as part of the
existing structure to get the error status.
>> + if (!peripheral)
>> + return -ENOMEM;
>> +
>> + config.peripheral_config = peripheral;
>> + config.peripheral_size = sizeof(struct gpi_i2c_config);
>>
>> - peripheral.pack_enable = I2C_PACK_TX | I2C_PACK_RX;
>> - peripheral.cycle_count = itr->t_cycle_cnt;
>> - peripheral.high_count = itr->t_high_cnt;
>> - peripheral.low_count = itr->t_low_cnt;
>> - peripheral.clk_div = itr->clk_div;
>> - peripheral.set_config = 1;
>> - peripheral.multi_msg = false;
>> + peripheral->gi2c = gi2c;
>> + peripheral->pack_enable = I2C_PACK_TX | I2C_PACK_RX;
>> + peripheral->cycle_count = itr->t_cycle_cnt;
>> + peripheral->high_count = itr->t_high_cnt;
>> + peripheral->low_count = itr->t_low_cnt;
>> + peripheral->clk_div = itr->clk_div;
>> + peripheral->set_config = 1;
>> + peripheral->multi_msg = false;
>>
>> for (i = 0; i < num; i++) {
>> gi2c->cur = &msgs[i];
>> gi2c->err = 0;
>> dev_dbg(gi2c->se.dev, "msg[%d].len:%d\n", i, gi2c->cur->len);
>>
>> - peripheral.stretch = 0;
>> + peripheral->stretch = 0;
>> if (i < num - 1)
>> - peripheral.stretch = 1;
>> + peripheral->stretch = 1;
>>
>> - peripheral.addr = msgs[i].addr;
>> + peripheral->addr = msgs[i].addr;
>>
>> ret = geni_i2c_gpi(gi2c, &msgs[i], &config,
>> &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);
>> diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
>> index 6680dd1a43c6..af264f769344 100644
>> --- a/include/linux/dma/qcom-gpi-dma.h
>> +++ b/include/linux/dma/qcom-gpi-dma.h
>> @@ -64,6 +64,8 @@ enum i2c_op {
>> * @set_config: set peripheral config
>> * @rx_len: receive length for buffer
>> * @op: i2c cmd
>> + * @status: stores gpi event status based on interrupt
>> + * @gi2c: pointer to i2c device handle
>
> The order doesn't match the struct below.
>
Yes, removed this change since we don't need to have new member inside
this structure.
>> * @muli-msg: is part of multi i2c r-w msgs
>> */
>> struct gpi_i2c_config {
>> @@ -78,6 +80,8 @@ struct gpi_i2c_config {
>> u32 rx_len;
>> enum i2c_op op;
>> bool multi_msg;
>> + u32 status;
>> + struct geni_i2c_dev *gi2c;
>
> These two entries doesn't have anything to do with the "gpi i2c config",
> just as the remainder of the properties has nothing to do with the "dma
> callback".
>
> Please split them off into their own structure.
>
Sure, made the chages by adding new structure.
> Regards,
> Bjorn
>
>> };
>>
>> #endif /* QCOM_GPI_DMA_H */
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
>>
>
On 3/2/2024 12:14 AM, Andi Shyti wrote:
> Hi Mukesh,
>
> (I'm sorry for the noise but my mail server has marked this mail
> as spam and put the spam tag in front of the subject. Therefore,
> my reply might have been marked as spam.)
>
> I'm going to send a new e-mail with the old answers.
>
Sure, no problem. Sorry for the late reply.
> On Fri, Mar 01, 2024 at 04:56:38PM +0530, Mukesh Kumar Savaliya wrote:
>> we are seeing protocol errors like NACK as transfer failure but
>
> /we/We/
>
Done
>> ideally it should report exact error like NACK, BUS_PROTO or ARB_LOST.
>>
>> Hence we are adding such error support in GSI mode and reporting it
>> accordingly by adding respective error logs.
>>
>> geni_i2c_gpi_xfer() needed to allocate heap based memory instead of
>
> Please use the imperative form.
>
Thanks. New patch uploaded which doesn't need memory allocation dynamically.
>> stack memory to handle and store the geni_i2c_dev handle.
>>
>> Copy event status from GSI driver to the i2c device status and parse
>> error when callback comes from gsi driver to the i2c driver. In the
>> gpi.c, we need to store callback param into i2c config data structure
>> so that inside the i2c driver, we can check what exactly the error is
>> and parse it accordingly.
>>
>> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
>
> What bug are you fixing here? The description doesn't talk about
> fixes rather than support added.
>
Updated commit log in a latest patch. Basically we are getting simple
transfer error which ideally should be NACK error. This happens while
running scan test for devices.
> ...
>
>> - config.peripheral_config = &peripheral;
>> - config.peripheral_size = sizeof(peripheral);
>> + peripheral = devm_kzalloc(gi2c->se.dev, sizeof(*peripheral), GFP_KERNEL);
>> + if (!peripheral)
>> + return -ENOMEM;
>
> This is a massive leak. Why are you deciding to make the
> allocation dynamic?
>
Agree, It's a memory leak. Thanks for catching BUG. Considered other
approach which doesn't need new structure and dynamic memory allocation.
> Thanks,
> Andi
>