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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 7 Mar 2024 15:13:20 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Vinod Koul <vkoul@...nel.org>, Bjorn Andersson <andersson@...nel.org>
CC: <konrad.dybcio@...aro.org>, <bjorn.andersson@...aro.org>,
        <andi.shyti@...nel.org>, <wsa@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <dmaengine@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-i2c@...r.kernel.org>,
        <quic_vdadhani@...cinc.com>
Subject: Re: [PATCH v1] i2c: i2c-qcom-geni: Parse Error correctly in i2c GSI
 mode



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