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

[Mukesh Kumar Savaliya]

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