From: rojay@codeaurora.org
To: Akash Asthana <akashast@codeaurora.org>
Cc: wsa@kernel.org, swboyd@chromium.org, dianders@chromium.org,
saiprakash.ranjan@codeaurora.org, gregkh@linuxfoundation.org,
mka@chromium.org, msavaliy@qti.qualcomm.com,
skakit@codeaurora.org, vkaur@codeaurora.org,
pyarlaga@codeaurora.org, rnayak@codeaurora.org,
agross@kernel.org, bjorn.andersson@linaro.org,
linux-arm-msm@vger.kernel.org, linux-i2c@vger.kernel.org,
linux-kernel@vger.kernel.org, sumit.semwal@linaro.org,
linux-media@vger.kernel.org
Subject: Re: [RESEND PATCH V6 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct
Date: Mon, 21 Dec 2020 18:04:13 +0530 [thread overview]
Message-ID: <585a9b7ab44e3a99eb803536af6334e5@codeaurora.org> (raw)
In-Reply-To: <049c28e9-1211-377f-941d-ba169645dd24@codeaurora.org>
On 2020-12-09 18:29, Akash Asthana wrote:
> Hi Roja,
>
> On 12/3/2020 4:01 PM, Roja Rani Yarubandi wrote:
>> Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping
>> data scope. For example during shutdown callback to unmap DMA mapping,
>> this stored DMA mapping data can be used to call geni_se_tx_dma_unprep
>> and geni_se_rx_dma_unprep functions.
>>
>> Add two helper functions geni_i2c_rx_msg_cleanup and
>> geni_i2c_tx_msg_cleanup to unwrap the things after rx/tx FIFO/DMA
>> transfers, so that the same can be used in geni_i2c_stop_xfer()
>> function during shutdown callback.
>>
>> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
>> ---
>> Changes in V5:
>> - As per Stephen's comments separated this patch from shutdown
>> callback patch, gi2c->cur = NULL is not removed from
>> geni_i2c_abort_xfer(), and made a copy of gi2c->cur and passed
>> to cleanup functions.
>>
>> Changes in V6:
>> - Added spin_lock/unlock in geni_i2c_rx_msg_cleanup() and
>> geni_i2c_tx_msg_cleanup() functions.
>>
>> drivers/i2c/busses/i2c-qcom-geni.c | 69
>> +++++++++++++++++++++++-------
>> 1 file changed, 53 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index dce75b85253c..bfbc80f65006 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -86,6 +86,9 @@ struct geni_i2c_dev {
>> u32 clk_freq_out;
>> const struct geni_i2c_clk_fld *clk_fld;
>> int suspended;
>> + void *dma_buf;
>> + size_t xfer_len;
>> + dma_addr_t dma_addr;
>> };
>> struct geni_i2c_err_log {
>> @@ -348,14 +351,49 @@ static void geni_i2c_tx_fsm_rst(struct
>> geni_i2c_dev *gi2c)
>> dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n");
>> }
>> +static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c,
>> + struct i2c_msg *cur)
>> +{
>> + struct geni_se *se = &gi2c->se;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&gi2c->lock, flags);
>> + gi2c->cur_rd = 0;
>> + if (gi2c->dma_buf) {
>> + if (gi2c->err)
>> + geni_i2c_rx_fsm_rst(gi2c);
>
> Which race we are trying to avoid here by holding spinlock?
>
Thought that race might occur with "cur" here.
> We cannot call any sleeping API by holding spinlock,
> geni_i2c_rx_fsm_rst calls *wait-for-completion*, which is a sleeping
> call.
>
Fixed this.
>> + geni_se_rx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len);
>> + i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err);
>> + }
>> + spin_unlock_irqrestore(&gi2c->lock, flags);
>> +}
>> +
>> +static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c,
>> + struct i2c_msg *cur)
>> +{
>> + struct geni_se *se = &gi2c->se;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&gi2c->lock, flags);
>> + gi2c->cur_wr = 0;
>> + if (gi2c->dma_buf) {
>> + if (gi2c->err)
>> + geni_i2c_tx_fsm_rst(gi2c);
>
> Same here
>
> Regards,
>
> Akash
>
>> + geni_se_tx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len);
>> + i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err);
>> + }
>> + spin_unlock_irqrestore(&gi2c->lock, flags);
>> +}
>> +
>> static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct
>> i2c_msg *msg,
>> u32 m_param)
>> {
>> - dma_addr_t rx_dma;
>> + dma_addr_t rx_dma = 0;
>> unsigned long time_left;
>> void *dma_buf = NULL;
>> struct geni_se *se = &gi2c->se;
>> size_t len = msg->len;
>> + struct i2c_msg *cur;
>> if (!of_machine_is_compatible("lenovo,yoga-c630"))
>> dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
>> @@ -372,19 +410,18 @@ static int geni_i2c_rx_one_msg(struct
>> geni_i2c_dev *gi2c, struct i2c_msg *msg,
>> geni_se_select_mode(se, GENI_SE_FIFO);
>> i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
>> dma_buf = NULL;
>> + } else {
>> + gi2c->xfer_len = len;
>> + gi2c->dma_addr = rx_dma;
>> + gi2c->dma_buf = dma_buf;
>> }
>> + cur = gi2c->cur;
>> time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
>> if (!time_left)
>> geni_i2c_abort_xfer(gi2c);
>> - gi2c->cur_rd = 0;
>> - if (dma_buf) {
>> - if (gi2c->err)
>> - geni_i2c_rx_fsm_rst(gi2c);
>> - geni_se_rx_dma_unprep(se, rx_dma, len);
>> - i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>> - }
>> + geni_i2c_rx_msg_cleanup(gi2c, cur);
>> return gi2c->err;
>> }
>> @@ -392,11 +429,12 @@ static int geni_i2c_rx_one_msg(struct
>> geni_i2c_dev *gi2c, struct i2c_msg *msg,
>> static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct
>> i2c_msg *msg,
>> u32 m_param)
>> {
>> - dma_addr_t tx_dma;
>> + dma_addr_t tx_dma = 0;
>> unsigned long time_left;
>> void *dma_buf = NULL;
>> struct geni_se *se = &gi2c->se;
>> size_t len = msg->len;
>> + struct i2c_msg *cur;
>> if (!of_machine_is_compatible("lenovo,yoga-c630"))
>> dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
>> @@ -413,22 +451,21 @@ static int geni_i2c_tx_one_msg(struct
>> geni_i2c_dev *gi2c, struct i2c_msg *msg,
>> geni_se_select_mode(se, GENI_SE_FIFO);
>> i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
>> dma_buf = NULL;
>> + } else {
>> + gi2c->xfer_len = len;
>> + gi2c->dma_addr = tx_dma;
>> + gi2c->dma_buf = dma_buf;
>> }
>> if (!dma_buf) /* Get FIFO IRQ */
>> writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG);
>> + cur = gi2c->cur;
>> time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
>> if (!time_left)
>> geni_i2c_abort_xfer(gi2c);
>> - gi2c->cur_wr = 0;
>> - if (dma_buf) {
>> - if (gi2c->err)
>> - geni_i2c_tx_fsm_rst(gi2c);
>> - geni_se_tx_dma_unprep(se, tx_dma, len);
>> - i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>> - }
>> + geni_i2c_tx_msg_cleanup(gi2c, cur);
>> return gi2c->err;
>> }
next prev parent reply other threads:[~2020-12-21 12:35 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-03 10:31 [RESEND PATCH V6 0/2] Implement Shutdown callback for geni-i2c Roja Rani Yarubandi
2020-12-03 10:31 ` [RESEND PATCH V6 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct Roja Rani Yarubandi
2020-12-09 12:59 ` Akash Asthana
2020-12-21 12:34 ` rojay [this message]
2020-12-03 10:31 ` [RESEND PATCH V6 2/2] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=585a9b7ab44e3a99eb803536af6334e5@codeaurora.org \
--to=rojay@codeaurora.org \
--cc=agross@kernel.org \
--cc=akashast@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=dianders@chromium.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mka@chromium.org \
--cc=msavaliy@qti.qualcomm.com \
--cc=pyarlaga@codeaurora.org \
--cc=rnayak@codeaurora.org \
--cc=saiprakash.ranjan@codeaurora.org \
--cc=skakit@codeaurora.org \
--cc=sumit.semwal@linaro.org \
--cc=swboyd@chromium.org \
--cc=vkaur@codeaurora.org \
--cc=wsa@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.