* [PATCH V8 0/1] Implement Shutdown callback for geni-i2c
@ 2021-01-08 15:05 Roja Rani Yarubandi
2021-01-08 15:05 ` [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi
2021-03-01 19:59 ` [PATCH V8 0/1] Implement Shutdown callback for geni-i2c patchwork-bot+linux-arm-msm
0 siblings, 2 replies; 9+ messages in thread
From: Roja Rani Yarubandi @ 2021-01-08 15:05 UTC (permalink / raw)
To: wsa
Cc: swboyd, dianders, saiprakash.ranjan, gregkh, mka, akashast,
msavaliy, skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media,
Roja Rani Yarubandi
Hi,
Other two patches of this series have been picked, posting this patch
with fixing minor comments.
Roja Rani Yarubandi (1):
i2c: i2c-qcom-geni: Add shutdown callback for i2c
drivers/i2c/busses/i2c-qcom-geni.c | 34 ++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-01-08 15:05 [PATCH V8 0/1] Implement Shutdown callback for geni-i2c Roja Rani Yarubandi
@ 2021-01-08 15:05 ` Roja Rani Yarubandi
2021-01-12 7:30 ` Akash Asthana
2021-01-13 6:54 ` Stephen Boyd
2021-03-01 19:59 ` [PATCH V8 0/1] Implement Shutdown callback for geni-i2c patchwork-bot+linux-arm-msm
1 sibling, 2 replies; 9+ messages in thread
From: Roja Rani Yarubandi @ 2021-01-08 15:05 UTC (permalink / raw)
To: wsa
Cc: swboyd, dianders, saiprakash.ranjan, gregkh, mka, akashast,
msavaliy, skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media,
Roja Rani Yarubandi
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".
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_one_msg_done() and
geni_i2c_tx_one_msg_done() 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.
- Checking 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 checking
Changes in V5:
- As per Stephen's comments, added spin_lock_irqsave & spin_unlock_irqsave in
geni_i2c_stop_xfer() function.
Changes in V6:
- As per Stephen's comments, taken care of unsafe lock order in
geni_i2c_stop_xfer().
- Moved spin_lock/unlock to geni_i2c_rx_msg_cleanup() and
geni_i2c_tx_msg_cleanup() functions.
Changes in V7:
- No changes
Changes in V8:
- As per Wolfram Sang comment, removed goto and modified geni_i2c_stop_xfer()
accordingly.
drivers/i2c/busses/i2c-qcom-geni.c | 34 ++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 214b4c913a13..c3f584795911 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -375,6 +375,32 @@ static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c,
}
}
+static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
+{
+ int ret;
+ u32 geni_status;
+ struct i2c_msg *cur;
+
+ /* 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) {
+ cur = gi2c->cur;
+ geni_i2c_abort_xfer(gi2c);
+ if (cur->flags & I2C_M_RD)
+ geni_i2c_rx_msg_cleanup(gi2c, cur);
+ else
+ geni_i2c_tx_msg_cleanup(gi2c, cur);
+ }
+
+ 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)
{
@@ -650,6 +676,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;
@@ -714,6 +747,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,
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-01-08 15:05 ` [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi
@ 2021-01-12 7:30 ` Akash Asthana
2021-01-13 6:54 ` Stephen Boyd
1 sibling, 0 replies; 9+ messages in thread
From: Akash Asthana @ 2021-01-12 7:30 UTC (permalink / raw)
To: Roja Rani Yarubandi, wsa
Cc: swboyd, dianders, saiprakash.ranjan, gregkh, mka, msavaliy,
skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media
On 1/8/2021 8:35 PM, Roja Rani Yarubandi wrote:
> 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".
>
> Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller")
> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-01-08 15:05 ` [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi
2021-01-12 7:30 ` Akash Asthana
@ 2021-01-13 6:54 ` Stephen Boyd
2021-02-18 14:15 ` rojay
1 sibling, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2021-01-13 6:54 UTC (permalink / raw)
To: Roja Rani Yarubandi, wsa
Cc: dianders, saiprakash.ranjan, gregkh, mka, akashast, msavaliy,
skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media,
Roja Rani Yarubandi
Quoting Roja Rani Yarubandi (2021-01-08 07:05:45)
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 214b4c913a13..c3f584795911 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -375,6 +375,32 @@ static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c,
> }
> }
>
> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
> +{
> + int ret;
> + u32 geni_status;
> + struct i2c_msg *cur;
> +
> + /* 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) {
> + cur = gi2c->cur;
Why don't we need to hold the spinlock gi2c::lock here?
> + geni_i2c_abort_xfer(gi2c);
> + if (cur->flags & I2C_M_RD)
> + geni_i2c_rx_msg_cleanup(gi2c, cur);
> + else
> + geni_i2c_tx_msg_cleanup(gi2c, cur);
> + }
> +
> + 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)
> {
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-01-13 6:54 ` Stephen Boyd
@ 2021-02-18 14:15 ` rojay
2021-02-24 7:06 ` Stephen Boyd
0 siblings, 1 reply; 9+ messages in thread
From: rojay @ 2021-02-18 14:15 UTC (permalink / raw)
To: Stephen Boyd
Cc: wsa, dianders, saiprakash.ranjan, gregkh, mka, akashast,
msavaliy, skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media
Hi Stephen,
On 2021-01-13 12:24, Stephen Boyd wrote:
> Quoting Roja Rani Yarubandi (2021-01-08 07:05:45)
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 214b4c913a13..c3f584795911 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -375,6 +375,32 @@ static void geni_i2c_tx_msg_cleanup(struct
>> geni_i2c_dev *gi2c,
>> }
>> }
>>
>> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> + int ret;
>> + u32 geni_status;
>> + struct i2c_msg *cur;
>> +
>> + /* 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) {
>> + cur = gi2c->cur;
>
> Why don't we need to hold the spinlock gi2c::lock here?
>
I am not seeing any race here. May I know which race are you suspecting
here?
>> + geni_i2c_abort_xfer(gi2c);
>> + if (cur->flags & I2C_M_RD)
>> + geni_i2c_rx_msg_cleanup(gi2c, cur);
>> + else
>> + geni_i2c_tx_msg_cleanup(gi2c, cur);
>> + }
>> +
>> + 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)
>> {
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-02-18 14:15 ` rojay
@ 2021-02-24 7:06 ` Stephen Boyd
2021-02-24 9:20 ` Wolfram Sang
2021-04-20 11:12 ` rojay
0 siblings, 2 replies; 9+ messages in thread
From: Stephen Boyd @ 2021-02-24 7:06 UTC (permalink / raw)
To: rojay
Cc: wsa, dianders, saiprakash.ranjan, gregkh, mka, akashast,
msavaliy, skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media
Quoting rojay@codeaurora.org (2021-02-18 06:15:17)
> Hi Stephen,
>
> On 2021-01-13 12:24, Stephen Boyd wrote:
> > Quoting Roja Rani Yarubandi (2021-01-08 07:05:45)
> >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
> >> b/drivers/i2c/busses/i2c-qcom-geni.c
> >> index 214b4c913a13..c3f584795911 100644
> >> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> >> @@ -375,6 +375,32 @@ static void geni_i2c_tx_msg_cleanup(struct
> >> geni_i2c_dev *gi2c,
> >> }
> >> }
> >>
> >> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
> >> +{
> >> + int ret;
> >> + u32 geni_status;
> >> + struct i2c_msg *cur;
> >> +
> >> + /* 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) {
> >> + cur = gi2c->cur;
> >
> > Why don't we need to hold the spinlock gi2c::lock here?
> >
>
> I am not seeing any race here. May I know which race are you suspecting
> here?
Sorry there are long delays between posting and replies to my review
comments. It takes me some time to remember what we're talking about
because this patch has dragged on for many months.
So my understanding is that gi2c::lock protects the 'cur' pointer. I
imagine this scenario might go bad
CPU0 CPU1
---- ----
geni_i2c_stop_xfer()
... geni_i2c_rx_one_msg()
gi2c->cur = cur1;
cur = gi2c->cur;
... geni_i2c_tx_one_msg()
gi2c->cur = cur2;
geni_i2c_abort_xfer()
<uses cur2>
if (cur->flags & I2C_M_RD)
<uses cur1 for the condition and call; oops that's bad>
It's almost like we should combine the geni_i2c_abort_xfer() logic with
the rx/tx message cleanup functions so that it's all done under one
lock. Unfortunately it's complicated by the fact that there are various
completion waiting timeouts involved. Fun!
But even after all that, I don't see how the geni_i2c_stop_xfer() puts a
stop to future calls to geni_i2c_rx_one_msg() or geni_i2c_tx_one_msg().
The hardware isn't disabled from what I can tell. The irq isn't
disabled, the clks aren't turned off, etc. What is to stop an i2c device
from trying to use the bus after this shutdown function is called? If
anything, this function looks like a "flush", where we flush out any
pending transfer. Where's the "plug" operation that prevents any future
operations from following this call?
BTW, I see this is merged upstream. That's great, but it seems broken.
Please fix it or revert it out.
>
> >> + geni_i2c_abort_xfer(gi2c);
> >> + if (cur->flags & I2C_M_RD)
> >> + geni_i2c_rx_msg_cleanup(gi2c, cur);
> >> + else
> >> + geni_i2c_tx_msg_cleanup(gi2c, cur);
> >> + }
> >> +
> >> + 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)
> >> {
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-02-24 7:06 ` Stephen Boyd
@ 2021-02-24 9:20 ` Wolfram Sang
2021-04-20 11:12 ` rojay
1 sibling, 0 replies; 9+ messages in thread
From: Wolfram Sang @ 2021-02-24 9:20 UTC (permalink / raw)
To: Stephen Boyd
Cc: rojay, dianders, saiprakash.ranjan, gregkh, mka, akashast,
msavaliy, skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media
[-- Attachment #1: Type: text/plain, Size: 269 bytes --]
> BTW, I see this is merged upstream. That's great, but it seems broken.
> Please fix it or revert it out.
Sorry, that was my mistake! I was aware of this discussion. It seems I
accidently applied it. I'll send a revert and we can reapply it once
everyone is happy.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V8 0/1] Implement Shutdown callback for geni-i2c
2021-01-08 15:05 [PATCH V8 0/1] Implement Shutdown callback for geni-i2c Roja Rani Yarubandi
2021-01-08 15:05 ` [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi
@ 2021-03-01 19:59 ` patchwork-bot+linux-arm-msm
1 sibling, 0 replies; 9+ messages in thread
From: patchwork-bot+linux-arm-msm @ 2021-03-01 19:59 UTC (permalink / raw)
To: Roja Rani Yarubandi; +Cc: linux-arm-msm
Hello:
This patch was applied to qcom/linux.git (refs/heads/for-next):
On Fri, 8 Jan 2021 20:35:44 +0530 you wrote:
> Hi,
>
> Other two patches of this series have been picked, posting this patch
> with fixing minor comments.
>
> Roja Rani Yarubandi (1):
> i2c: i2c-qcom-geni: Add shutdown callback for i2c
>
> [...]
Here is the summary with links:
- [V8,1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c
https://git.kernel.org/qcom/c/e0371298ddc5
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-02-24 7:06 ` Stephen Boyd
2021-02-24 9:20 ` Wolfram Sang
@ 2021-04-20 11:12 ` rojay
1 sibling, 0 replies; 9+ messages in thread
From: rojay @ 2021-04-20 11:12 UTC (permalink / raw)
To: Stephen Boyd
Cc: wsa, dianders, saiprakash.ranjan, gregkh, mka, akashast,
msavaliy, skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media
Hi Stephen,
On 2021-02-24 12:36, Stephen Boyd wrote:
> Quoting rojay@codeaurora.org (2021-02-18 06:15:17)
>> Hi Stephen,
>>
>> On 2021-01-13 12:24, Stephen Boyd wrote:
>> > Quoting Roja Rani Yarubandi (2021-01-08 07:05:45)
>> >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
>> >> b/drivers/i2c/busses/i2c-qcom-geni.c
>> >> index 214b4c913a13..c3f584795911 100644
>> >> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> >> @@ -375,6 +375,32 @@ static void geni_i2c_tx_msg_cleanup(struct
>> >> geni_i2c_dev *gi2c,
>> >> }
>> >> }
>> >>
>> >> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
>> >> +{
>> >> + int ret;
>> >> + u32 geni_status;
>> >> + struct i2c_msg *cur;
>> >> +
>> >> + /* 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) {
>> >> + cur = gi2c->cur;
>> >
>> > Why don't we need to hold the spinlock gi2c::lock here?
>> >
>>
>> I am not seeing any race here. May I know which race are you
>> suspecting
>> here?
>
> Sorry there are long delays between posting and replies to my review
> comments. It takes me some time to remember what we're talking about
> because this patch has dragged on for many months.
>
Sorry for the delayed responses.
> So my understanding is that gi2c::lock protects the 'cur' pointer. I
> imagine this scenario might go bad
>
> CPU0 CPU1
> ---- ----
> geni_i2c_stop_xfer()
> ... geni_i2c_rx_one_msg()
> gi2c->cur = cur1;
> cur = gi2c->cur;
> ... geni_i2c_tx_one_msg()
> gi2c->cur = cur2;
> geni_i2c_abort_xfer()
> <uses cur2>
> if (cur->flags & I2C_M_RD)
> <uses cur1 for the condition and call; oops that's bad>
>
> It's almost like we should combine the geni_i2c_abort_xfer() logic with
> the rx/tx message cleanup functions so that it's all done under one
> lock. Unfortunately it's complicated by the fact that there are various
> completion waiting timeouts involved. Fun!
>
Thanks for the explanation. Fixed this possible race by protecting
gi2c->cur
and calling geni_i2c_abort_xfer() with adding another parameter to
differentiate
from which sequence is the geni_i2c_abort_xfer() called and handle the
spin_lock/spin_unlock accordingly inside geni_i2c_abort_xfer()
> But even after all that, I don't see how the geni_i2c_stop_xfer() puts
> a
> stop to future calls to geni_i2c_rx_one_msg() or geni_i2c_tx_one_msg().
Now handled this by adding a bool variable "stop_xfer" in geni_i2c_dev
struct,
used to put stop to upcoming geni_i2c_rx_one_msg() and
geni_i2c_tx_one_msg() calls
once we receive the shutdown call.
> The hardware isn't disabled from what I can tell. The irq isn't
> disabled, the clks aren't turned off, etc. What is to stop an i2c
> device
> from trying to use the bus after this shutdown function is called? If
> anything, this function looks like a "flush", where we flush out any
> pending transfer. Where's the "plug" operation that prevents any future
> operations from following this call?
>
We are turning off clocks and disabling irq in
geni_i2c_runtime_suspend().
IIUC about shutdown sequence, during "remove" we will unplug the device
with opposite
calls to "probe's" plug operations example i2c_del_adapter(). For
"shutdown", as system
is going to shutdown, there is no need of unplug operations to be done.
> BTW, I see this is merged upstream. That's great, but it seems broken.
> Please fix it or revert it out.
>
>>
>> >> + geni_i2c_abort_xfer(gi2c);
>> >> + if (cur->flags & I2C_M_RD)
>> >> + geni_i2c_rx_msg_cleanup(gi2c, cur);
>> >> + else
>> >> + geni_i2c_tx_msg_cleanup(gi2c, cur);
>> >> + }
>> >> +
>> >> + 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)
>> >> {
Thanks,
Roja
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-04-20 11:12 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-08 15:05 [PATCH V8 0/1] Implement Shutdown callback for geni-i2c Roja Rani Yarubandi
2021-01-08 15:05 ` [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi
2021-01-12 7:30 ` Akash Asthana
2021-01-13 6:54 ` Stephen Boyd
2021-02-18 14:15 ` rojay
2021-02-24 7:06 ` Stephen Boyd
2021-02-24 9:20 ` Wolfram Sang
2021-04-20 11:12 ` rojay
2021-03-01 19:59 ` [PATCH V8 0/1] Implement Shutdown callback for geni-i2c patchwork-bot+linux-arm-msm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).