Re: [RESEND PATCH V6 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct

[rojay]

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 <ro...@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;
  }