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

From: Mukesh Kumar Savaliya
Date: Thu Mar 07 2024 - 05:07:23 EST




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