| Message ID | 20200917122558.23110-1-rojay@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V4] i2c: i2c-qcom-geni: Add shutdown callback for i2c | expand |
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? > }; > > 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? > +{ > + 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? > +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. > 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?
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
Quoting rojay@codeaurora.org (2020-09-21 04:21:04) > 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 > >> }; > >> > >> 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? I'm suggesting to copy the cur data out of the gi2c pointer and then pass it to these functions so that it can't race with another transfer. Something like an atomic exchange may work. I haven't thought about it deeply, but we need to make sure the irq handler can't race with the cleanup functions.
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; }; 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; 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) +{ + 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) +{ + 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); + 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; +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; } @@ -394,7 +447,7 @@ 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; @@ -414,6 +467,10 @@ 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->tx_dma = tx_dma; + gi2c->dma_buf = dma_buf; } geni_se_setup_m_cmd(se, I2C_WRITE, m_param); @@ -425,13 +482,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, 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); return gi2c->err; } @@ -630,6 +681,13 @@ static int geni_i2c_remove(struct platform_device *pdev) return 0; } +static void geni_i2c_shutdown(struct platform_device *pdev) +{ + struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev); + + geni_i2c_stop_xfer(gi2c); +} + static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev) { int ret; @@ -694,6 +752,7 @@ MODULE_DEVICE_TABLE(of, geni_i2c_dt_match); static struct platform_driver geni_i2c_driver = { .probe = geni_i2c_probe, .remove = geni_i2c_remove, + .shutdown = geni_i2c_shutdown, .driver = { .name = "geni_i2c", .pm = &geni_i2c_pm_ops, 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);
If the hardware is still accessing memory after SMMU translation is disabled (as part of smmu shutdown callback), then the IOVAs (I/O virtual address) which it was using will go on the bus as the physical addresses which will result in unknown crashes like NoC/interconnect errors. So, implement shutdown callback to i2c driver to stop on-going transfer and unmap DMA mappings during system "reboot" or "shutdown". 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, geni_se_rx_dma_unprep and i2c_put_dma_safe_msg_buf 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. Remove "iova" check from geni_se_tx_dma_unprep and geni_se_rx_dma_unprep fucntions as invalidating with dma_mapping_error() is enough. Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller") Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org> --- Changes in V2: - As per Stephen's comments added seperate function for stop transfer, fixed minor nitpicks. - As per Stephen's comments, changed commit text. Changes in V3: - As per Stephen's comments, squashed patch 1 into patch 2, added Fixes tag. - As per Akash's comments, included FIFO case in stop_xfer, fixed minor nitpicks. Changes in V4: - As per Stephen's comments cleaned up geni_i2c_stop_xfer function, added dma_buf in geni_i2c_dev struct to call i2c_put_dma_safe_msg_buf() from other functions, removed "iova" check in geni_se_rx_dma_unprep() and geni_se_tx_dma_unprep() functions. - Added 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. Updated commit text accordingly. - In stop_xfer check whether it is tx/rx transfer using I2C_M_RD which is valid for both FIFO and DMA cases, so dropped DMA_RX_ACTIVE and DMA_TX_ACTIVE bit check of SE_DMA_DEBUG_REG0 register. drivers/i2c/busses/i2c-qcom-geni.c | 93 ++++++++++++++++++++++++------ drivers/soc/qcom/qcom-geni-se.c | 4 +- 2 files changed, 78 insertions(+), 19 deletions(-)