| 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
| ||
|
Message-ID: <5df68078-24f9-4754-b0b9-6fdafc908831@quicinc.com>
Date: Thu, 7 Mar 2024 15:29:00 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: <konrad.dybcio@...aro.org>, <bjorn.andersson@...aro.org>,
<vkoul@...nel.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/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@...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?
>
>> + 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
>>
>>
>
Powered by blists - more mailing lists