| 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: <fdd2919bc0705b4bd54b8be92fbc9fe5@codeaurora.org>
Date: Thu, 17 Sep 2020 17:33:30 +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,
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 V3] i2c: i2c-qcom-geni: Add shutdown callback for i2c
Hi Stephen,
On 2020-09-09 01:00, Stephen Boyd wrote:
> Why is dri-devel on here? And linaro-mm-sig?
>
Ok, I will remove these lists.
> Quoting Roja Rani Yarubandi (2020-09-07 06:07:31)
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index dead5db3315a..b3609760909f 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> struct geni_i2c_err_log {
>> @@ -384,7 +387,8 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev
>> *gi2c, struct i2c_msg *msg,
>> if (dma_buf) {
>> if (gi2c->err)
>> geni_i2c_rx_fsm_rst(gi2c);
>> - geni_se_rx_dma_unprep(se, rx_dma, len);
>> + geni_se_rx_dma_unprep(se, gi2c->rx_dma, len);
>> + gi2c->rx_dma = (dma_addr_t)NULL;
>> i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>> }
>>
>> @@ -394,12 +398,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;
>> unsigned long time_left;
>> void *dma_buf = NULL;
>> struct geni_se *se = &gi2c->se;
>> size_t len = msg->len;
>>
>> + gi2c->xfer_len = len;
>> if (!of_machine_is_compatible("lenovo,yoga-c630"))
>> dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
>>
>> @@ -410,7 +414,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev
>> *gi2c, struct i2c_msg *msg,
>>
>> writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN);
>>
>> - if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, &tx_dma))
>> {
>> + if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len,
>> &gi2c->tx_dma)) {
>> geni_se_select_mode(se, GENI_SE_FIFO);
>> i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
>> dma_buf = NULL;
>> @@ -429,7 +433,8 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev
>> *gi2c, struct i2c_msg *msg,
>> if (dma_buf) {
>> if (gi2c->err)
>> geni_i2c_tx_fsm_rst(gi2c);
>> - geni_se_tx_dma_unprep(se, tx_dma, len);
>> + geni_se_tx_dma_unprep(se, gi2c->tx_dma, len);
>> + gi2c->tx_dma = (dma_addr_t)NULL;
>> i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>> }
>>
>> @@ -479,6 +484,51 @@ static int geni_i2c_xfer(struct i2c_adapter
>> *adap,
>> return ret;
>> }
>>
>> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> + int ret;
>> + u32 dma;
>> + u32 val;
>> + u32 geni_status;
>> + struct geni_se *se = &gi2c->se;
>> +
>> + ret = pm_runtime_get_sync(gi2c->se.dev);
>> + if (ret < 0) {
>> + dev_err(gi2c->se.dev, "Failed to resume device: %d\n",
>> ret);
>
> Is this print really necessary? Doesn't PM core already print this sort
> of information?
>
PM core will not print any such information.
Here we wanted to know our driver's pm runtime resume is successful.
>> + return;
>> + }
>> +
>> + geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
>> + if (geni_status & M_GENI_CMD_ACTIVE) {
>
> Please try to de-indent all this.
>
Okay.
> if (!(geni_status & M_GENI_CMD_ACTIVE))
> goto out;
>
>> + geni_i2c_abort_xfer(gi2c);
>> + dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
>> + if (dma) {
>
> if (!dma)
> goto out;
>
>> + val = readl_relaxed(gi2c->se.base +
>> SE_DMA_DEBUG_REG0);
>> + if (val & DMA_TX_ACTIVE) {
>> + gi2c->cur_wr = 0;
>> + if (gi2c->err)
>> + geni_i2c_tx_fsm_rst(gi2c);
>> + if (gi2c->tx_dma) {
>> + geni_se_tx_dma_unprep(se,
>> + gi2c->tx_dma,
>> gi2c->xfer_len);
>> + gi2c->tx_dma =
>> (dma_addr_t)NULL;
>
> Almost nobody does this. In fact, grep shows me one hit in the kernel.
> If nobody else is doing it something is probably wrong. When would dma
> mode be active and tx_dma not be set to something that should be
> stopped? If it really is necessary I suppose we should assign this to
> DMA_MAPPING_ERROR instead of casting NULL. Then the check above for
> tx_dma being valid can be dropped because geni_se_tx_dma_unprep()
> already checks for a valid mapping before doing anything. But really,
> we
> should probably be tracking the dma buffer mapped to the CPU as well as
> the dma address that was used for the mapping. Not storing both is a
> problem, see below.
>
You are correct, setting gi2c->tx_dma to NULL and check for tx_dma is
not required. Will correct this.
>> + }
>> + } else if (val & DMA_RX_ACTIVE) {
>> + gi2c->cur_rd = 0;
>> + if (gi2c->err)
>> + geni_i2c_rx_fsm_rst(gi2c);
>> + if (gi2c->rx_dma) {
>> + geni_se_rx_dma_unprep(se,
>> + gi2c->rx_dma,
>> gi2c->xfer_len);
>
> Looking closely it seems that the geni dma wrappers shouldn't even be
> checking for an iova being non-zero. Instead they should make sure that
> it just isn't invalid with !dma_mapping_error().
>
Yes. I will remove iova check in geni_se_rx_dma_unprep() function(also
in tx_dma_unprep)
>> + gi2c->rx_dma =
>> (dma_addr_t)NULL;
>
> If we're stopping some dma transaction doesn't that mean the
>
> i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>
> code needs to run also? I fail to see where we free the buffer that has
> been mapped for DMA.
>
Yes, this is required. I will do this cleanup.
>> + }
>> + }
>> + }
>> + }
>> +
>
> out:
>
>> + pm_runtime_put_sync_suspend(gi2c->se.dev);
>> +}
>> +
Powered by blists - more mailing lists