Messages in this thread | | | Date | Mon, 21 Sep 2020 16:51:04 +0530 | From | rojay@codeauro ... | 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
| |