Received-SPF: pass (google.com: domain of linux-kernel+bounces-95286-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <5df68078-24f9-4754-b0b9-6fdafc908831@quicinc.com>
Date: Thu, 7 Mar 2024 15:29:00 +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: Bjorn Andersson <andersson@kernel.org>
CC: <konrad.dybcio@linaro.org>, <bjorn.andersson@linaro.org>,
        <vkoul@kernel.org>, <andi.shyti@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>
 <fuggv2kghhxijcljavzsus5uagjiknj5mrzwmqbxbhkyov5t75@smpxbhpdz7cv>
From: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
In-Reply-To: <fuggv2kghhxijcljavzsus5uagjiknj5mrzwmqbxbhkyov5t75@smpxbhpdz7cv>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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 <quic_vdadhani@quicinc.com>
>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>>   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
>>
>>
>