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
| ||
|
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