Received-SPF: pass (google.com: domain of linux-kernel+bounces-95294-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <9769c51f-a672-4ec9-b1c8-d8022f1988f5@quicinc.com>
Date: Thu, 7 Mar 2024 15:32:59 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI
 mode
Content-Language: en-US
To: Andi Shyti <andi.shyti@kernel.org>
CC: <konrad.dybcio@linaro.org>, <bjorn.andersson@linaro.org>,
        <vkoul@kernel.org>, <wsa@kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <dmaengine@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-i2c@vger.kernel.org>, <quic_vdadhani@quicinc.com>
References: <20240301112638.990045-1-quic_msavaliy@quicinc.com>
 <2wala6lz4vanhvfx6jtpdexnpohabuvhzt4i7kt2xvmlfrapq4@tmvl37npj7jy>
From: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
In-Reply-To: <2wala6lz4vanhvfx6jtpdexnpohabuvhzt4i7kt2xvmlfrapq4@tmvl37npj7jy>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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
>