Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: rojay=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Mon, 21 Sep 2020 16:51:04 +0530
From:   rojay@codeaurora.org
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     wsa@kernel.org, dianders@chromium.org,
        saiprakash.ranjan@codeaurora.org, gregkh@linuxfoundation.org,
        mka@chromium.org, akashast@codeaurora.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: [PATCH V4] i2c: i2c-qcom-geni: Add shutdown callback for i2c
In-Reply-To: <160037421089.4188128.9425314091585708436@swboyd.mtv.corp.google.com>
References: <20200917122558.23110-1-rojay@codeaurora.org>
 <160037421089.4188128.9425314091585708436@swboyd.mtv.corp.google.com>
Message-ID: <f6cb2d7bc02dab409030ec42bf1d17c9@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

Hi Stephen,

On 2020-09-18 01:53, Stephen Boyd wrote:
> Quoting Roja Rani Yarubandi (2020-09-17 05:25:58)
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index dead5db3315a..b0d8043c8cb2 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -86,6 +86,10 @@ 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 tx_dma;
>> +       dma_addr_t rx_dma;
> 
> Do we need both tx_dma and rx_dma? Seems that we use cur->flags to
> figure out if the transfer is tx or rx so we could have juat dma_buf 
> and
> dma_addr here?
> 

Okay.

>>  };
>> 
>>  struct geni_i2c_err_log {
>> @@ -307,7 +311,6 @@ static void geni_i2c_abort_xfer(struct 
>> geni_i2c_dev *gi2c)
>> 
>>         spin_lock_irqsave(&gi2c->lock, flags);
>>         geni_i2c_err(gi2c, GENI_TIMEOUT);
>> -       gi2c->cur = NULL;
> 
> This looks concerning. We're moving this out from under the spinlock.
> The irq handler in this driver seems to hold the spinlock all the time
> while processing and this function grabs it here to keep cur consistent
> when aborting the transfer due to a timeout. Otherwise it looks like 
> the
> irqhandler can race with this and try to complete the transfer while
> it's being torn down here.
> 
>>         geni_se_abort_m_cmd(&gi2c->se);
>>         spin_unlock_irqrestore(&gi2c->lock, flags);
>>         do {
>> @@ -349,10 +352,62 @@ 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)
> 
> So maybe pass cur to this function?
> 

Sorry, i did not understand why to pass cur to this function?

>> +{
>> +       struct geni_se *se = &gi2c->se;
>> +
>> +       gi2c->cur_rd = 0;
>> +       if (gi2c->dma_buf) {
>> +               if (gi2c->err)
>> +                       geni_i2c_rx_fsm_rst(gi2c);
>> +               geni_se_rx_dma_unprep(se, gi2c->rx_dma, 
>> gi2c->xfer_len);
>> +               i2c_put_dma_safe_msg_buf(gi2c->dma_buf, gi2c->cur, 
>> !gi2c->err);
>> +       }
>> +}
>> +
>> +static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c)
> 
> And this one?
> 
>> +{
>> +       struct geni_se *se = &gi2c->se;
>> +
>> +       gi2c->cur_wr = 0;
>> +       if (gi2c->dma_buf) {
>> +               if (gi2c->err)
>> +                       geni_i2c_tx_fsm_rst(gi2c);
>> +               geni_se_tx_dma_unprep(se, gi2c->tx_dma, 
>> gi2c->xfer_len);
>> +               i2c_put_dma_safe_msg_buf(gi2c->dma_buf, gi2c->cur, 
>> !gi2c->err);
>> +       }
>> +}
>> +
>> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> +       int ret;
>> +       u32 geni_status;
>> +
>> +       /* Resume device, as runtime suspend can happen anytime during 
>> transfer */
>> +       ret = pm_runtime_get_sync(gi2c->se.dev);
>> +       if (ret < 0) {
>> +               dev_err(gi2c->se.dev, "Failed to resume device: %d\n", 
>> ret);
>> +               return;
>> +       }
>> +
>> +       geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
> 
> And this probably needs to hold the lock?
> 
>> +       if (!(geni_status & M_GENI_CMD_ACTIVE))
>> +               goto out;
>> +
>> +       geni_i2c_abort_xfer(gi2c);
>> +       if (gi2c->cur->flags & I2C_M_RD)
>> +               geni_i2c_rx_msg_cleanup(gi2c);
>> +       else
>> +               geni_i2c_tx_msg_cleanup(gi2c);
>> +       gi2c->cur = NULL;
> 
> until here?
> 

Okay.

>> +out:
>> +       pm_runtime_put_sync_suspend(gi2c->se.dev);
>> +}
>> +
>>  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;
>> @@ -372,6 +427,10 @@ 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->rx_dma = rx_dma;
>> +               gi2c->dma_buf = dma_buf;
>>         }
>> 
>>         geni_se_setup_m_cmd(se, I2C_READ, m_param);
>> @@ -380,13 +439,7 @@ static int geni_i2c_rx_one_msg(struct 
>> geni_i2c_dev *gi2c, struct i2c_msg *msg,
>>         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);
>> 
>>         return gi2c->err;
>>  }
> 
> It may make sense to extract the cleanup stuff into another patch. Then
> have a patch after that which does the shutdown hook. So three patches
> total.
> 

Okay, I will make separate patches, one for cleanup and another for 
shutdown hook.

>> diff --git a/drivers/soc/qcom/qcom-geni-se.c 
>> b/drivers/soc/qcom/qcom-geni-se.c
>> index d0e4f520cff8..0216b38c1e9a 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -705,7 +705,7 @@ void geni_se_tx_dma_unprep(struct geni_se *se, 
>> dma_addr_t iova, size_t len)
>>  {
>>         struct geni_wrapper *wrapper = se->wrapper;
>> 
>> -       if (iova && !dma_mapping_error(wrapper->dev, iova))
>> +       if (!dma_mapping_error(wrapper->dev, iova))
>>                 dma_unmap_single(wrapper->dev, iova, len, 
>> DMA_TO_DEVICE);
>>  }
>>  EXPORT_SYMBOL(geni_se_tx_dma_unprep);
>> @@ -722,7 +722,7 @@ void geni_se_rx_dma_unprep(struct geni_se *se, 
>> dma_addr_t iova, size_t len)
>>  {
>>         struct geni_wrapper *wrapper = se->wrapper;
>> 
>> -       if (iova && !dma_mapping_error(wrapper->dev, iova))
>> +       if (!dma_mapping_error(wrapper->dev, iova))
>>                 dma_unmap_single(wrapper->dev, iova, len, 
>> DMA_FROM_DEVICE);
>>  }
>>  EXPORT_SYMBOL(geni_se_rx_dma_unprep);
> 
> I'd make this a different patch. Nothing depends on this change, right?

Yes this is independent patch. I will make this as separate patch which 
will be the third one.

Thanks,
Roja