| 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: <f6cb2d7bc02dab409030ec42bf1d17c9@codeaurora.org>
Date: Mon, 21 Sep 2020 16:51:04 +0530
From: rojay@...eaurora.org
To: Stephen Boyd <swboyd@...omium.org>
Cc: wsa@...nel.org, dianders@...omium.org,
saiprakash.ranjan@...eaurora.org, gregkh@...uxfoundation.org,
mka@...omium.org, akashast@...eaurora.org,
msavaliy@....qualcomm.com, skakit@...eaurora.org,
vkaur@...eaurora.org, pyarlaga@...eaurora.org,
rnayak@...eaurora.org, agross@...nel.org,
bjorn.andersson@...aro.org, linux-arm-msm@...r.kernel.org,
linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
sumit.semwal@...aro.org, linux-media@...r.kernel.org
Subject: Re: [PATCH V4] i2c: i2c-qcom-geni: Add shutdown callback for i2c
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
Powered by blists - more mailing lists