diff mbox series

[V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c

Message ID 20210512082220.7137-1-rojay@codeaurora.org
State New
Headers show
Series [V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c | expand

Commit Message

Roja Rani Yarubandi May 12, 2021, 8:22 a.m. UTC 
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 for i2c driver to suspend the bus
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.

Changes in V9:
 - Fixed possbile 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(). For this added two macros ABORT_XFER and
   STOP_AND_ABORT_XFER.
 - Added 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
   recieve the shutdown call.
 - Added gi2c->cur == NULL check in geni_i2c_irq() to not to process the irq
   even if any transfer is queued and shutdown to HW received.

Changes in V10:
 - As per Stephen's comments, removed ongoing transfers flush and only 
   suspending i2c bus in shutdown callback.
 - Also removed all other changes which have been made for ongoing transfers 
   flush, handling race issues etc., during shutdown callback.
 - Updated commit text accordingly.

 drivers/i2c/busses/i2c-qcom-geni.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Roja Rani Yarubandi May 17, 2021, 6:32 a.m. UTC | #1 
Hi Stephen,

On 2021-05-13 00:27, Stephen Boyd wrote:
> Quoting Roja Rani Yarubandi (2021-05-12 01:22:20)
>> 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 for i2c driver to suspend the bus
>> 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.
>> 
>> Changes in V9:
>>  - Fixed possbile 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(). For this added two macros ABORT_XFER 
>> and
>>    STOP_AND_ABORT_XFER.
>>  - Added 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
>>    recieve the shutdown call.
>>  - Added gi2c->cur == NULL check in geni_i2c_irq() to not to process 
>> the irq
>>    even if any transfer is queued and shutdown to HW received.
>> 
>> Changes in V10:
>>  - As per Stephen's comments, removed ongoing transfers flush and only
>>    suspending i2c bus in shutdown callback.
>>  - Also removed all other changes which have been made for ongoing 
>> transfers
>>    flush, handling race issues etc., during shutdown callback.
>>  - Updated commit text accordingly.
>> 
>>  drivers/i2c/busses/i2c-qcom-geni.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 214b4c913a13..277ab7e7dd51 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -650,6 +650,14 @@ 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);
>> +
>> +       if (!pm_runtime_status_suspended(gi2c->se.dev))
>> +               pm_runtime_set_suspended(gi2c->se.dev);
> 
> What was wrong with my suggested approach of telling i2c core that the
> bus is suspended? This looks to do a bunch of work right before we're
> shutting down, when it would be simpler to just mark the bus as
> suspended and have it block future transactions and spit out a warning.
> 

Sorry, I have overlooked your previous change. I will do that.

> I do see that we should be marking it suspended/resumed during runtime
> suspend/resume. I'm also confused if during system wide suspend/resume
> we actually do anything in this driver. Is runtime suspend called for
> system wide suspend?
> 

In this geni i2c driver, runtime suspend will be called during system 
suspend
if the device is NOT already suspended.

> ----8<----
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
> b/drivers/i2c/busses/i2c-qcom-geni.c
> index 214b4c913a13..ca12d348336b 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -656,6 +656,7 @@ static int __maybe_unused
> geni_i2c_runtime_suspend(struct device *dev)
>  	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
> 
>  	disable_irq(gi2c->irq);
> +	i2c_mark_adapter_suspended(&gi2c->adap);
>  	ret = geni_se_resources_off(&gi2c->se);
>  	if (ret) {
>  		enable_irq(gi2c->irq);
> @@ -682,6 +683,7 @@ static int __maybe_unused
> geni_i2c_runtime_resume(struct device *dev)
>  		return ret;
> 
>  	enable_irq(gi2c->irq);
> +	i2c_mark_adapter_resumed(&gi2c->adap);
>  	gi2c->suspended = 0;
>  	return 0;
>  }

Now, I have made the changes, calling i2c_mark_adapter_suspended() in
shutdown() and i2c_mark_adapter_suspended()/_resumed() from runtime
suspend/resume also and validated the changes. I have also picked
your patch [1] for this validation.

During the device boot up I am seeing multiple traces shown below.
Are these expected now and needs to be fixed from rt5682/respective
client driver?

Trace1:
[   11.709477] i2c i2c-9: Transfer while suspended
[   11.905595] Call trace:
[   11.908124]  __i2c_transfer+0xb8/0x38c
[   11.911984]  i2c_transfer+0xa0/0xf4
[   11.915569]  i2c_transfer_buffer_flags+0x68/0x9c
[   11.920314]  regmap_i2c_write+0x34/0x64
[   11.924255]  _regmap_raw_write_impl+0x4e8/0x7bc
[   11.928911]  _regmap_bus_raw_write+0x70/0x8c
[   11.933301]  _regmap_write+0x100/0x150
[   11.937152]  regmap_write+0x54/0x78
[   11.940744]  soc_component_write_no_lock+0x34/0xa8
[   11.945666]  snd_soc_component_write+0x3c/0x5c
[   11.950242]  rt5682_set_component_pll+0x1e4/0x2b4 [snd_soc_rt5682]
[   11.956588]  snd_soc_component_set_pll+0x50/0xa8
[   11.961328]  snd_soc_dai_set_pll+0x74/0xc8
[   11.965542]  sc7180_snd_startup+0x9c/0x120 [snd_soc_sc7180]
[   11.971262]  snd_soc_link_startup+0x34/0x88
[   11.975557]  soc_pcm_open+0x100/0x538
[   11.979323]  snd_pcm_open_substream+0x530/0x704
[   11.983980]  snd_pcm_open+0xc8/0x210
[   11.987653]  snd_pcm_playback_open+0x50/0x80
[   11.992049]  snd_open+0x120/0x150
[   11.995462]  chrdev_open+0xb8/0x1a4
[   11.999056]  do_dentry_open+0x238/0x358
[   12.003001]  vfs_open+0x34/0x40
[   12.006235]  path_openat+0x9e8/0xd60
[   12.009913]  do_filp_open+0x90/0x10c
[   12.013587]  do_sys_open+0x148/0x314
[   12.017260]  __arm64_compat_sys_openat+0x28/0x34
[   12.022009]  el0_svc_common+0xa4/0x16c
[   12.025860]  el0_svc_compat_handler+0x2c/0x40
[   12.030337]  el0_svc_compat+0x8/0x10
[   12.034018] ---[ end trace 745ead557fcbb5dc ]---
[   12.040151] rt5682 9-001a: ASoC: error at soc_component_write_no_lock 
on rt5682.9-001a: -108
[   12.049055] rt5682 9-001a: ASoC: error at soc_component_write_no_lock 
on rt5682.9-001a: -108
[   12.057742] rt5682 9-001a: ASoC: error at 
snd_soc_component_update_bits on rt5682.9-001a: -108

Trace2:
[    3.515390] i2c i2c-2: Transfer while suspended
[    3.606749] Call trace:
[    3.606751]  __i2c_transfer+0xb8/0x38c
[    3.606752]  i2c_transfer+0xa0/0xf4
[    3.606754]  i2c_transfer_buffer_flags+0x68/0x9c
[    3.639599] hub 2-1.4:1.0: USB hub found
[    3.644375]  regmap_i2c_write+0x34/0x64
[    3.644376]  _regmap_raw_write_impl+0x4e8/0x7bc
[    3.644378]  _regmap_bus_raw_write+0x70/0x8c
[    3.644379]  _regmap_write+0x100/0x150
[    3.644381]  regmap_write+0x54/0x78
[    3.644383]  ti_sn_aux_transfer+0x90/0x244
[    3.650695] hub 2-1.4:1.0: 4 ports detected
[    3.655288]  drm_dp_dpcd_access+0x8c/0x11c
[    3.655289]  drm_dp_dpcd_read+0x64/0x10c
[    3.655290]  ti_sn_bridge_enable+0x5c/0x824
[    3.655292]  drm_atomic_bridge_chain_enable+0x78/0xa0
[    3.655294]  drm_atomic_helper_commit_modeset_enables+0x198/0x238
[    3.655295]  msm_atomic_commit_tail+0x324/0x714
[    3.655297]  commit_tail+0xa4/0x108
[    3.664985] usb 1-1.4: new high-speed USB device number 4 using 
xhci-hcd
[    3.666204]  drm_atomic_helper_commit+0xf4/0xfc
[    3.666205]  drm_atomic_commit+0x50/0x5c
[    3.666206]  drm_atomic_helper_set_config+0x64/0x98
[    3.666208]  drm_mode_setcrtc+0x26c/0x590
[    3.666209]  drm_ioctl_kernel+0x9c/0x114
[    3.701074] hub 2-1.4:1.0: USB hub found
[    3.703347]  drm_ioctl+0x288/0x420
[    3.703349]  drm_compat_ioctl+0xd0/0xe0
[    3.703351]  __arm64_compat_sys_ioctl+0x100/0x2108
[    3.703354]  el0_svc_common+0xa4/0x16c
[    3.708499] hub 2-1.4:1.0: 4 ports detected
[    3.711588]  el0_svc_compat_handler+0x2c/0x40
[    3.711590]  el0_svc_compat+0x8/0x10
[    3.711591] ---[ end trace 745ead557fcbb5db ]---
[    3.772120] usb 1-1.4: New USB device found, idVendor=0bda, 
idProduct=5411, bcdDevice= 1.04
[    3.794990] ti_sn65dsi86 2-002d: [drm:ti_sn_bridge_enable] *ERROR* 
Can't read lane count (-108); assuming 4

[1] 
https://lore.kernel.org/r/20210508075151.1626903-2-swboyd@chromium.org

Thanks,
Roja
Stephen Boyd May 20, 2021, 8:15 a.m. UTC | #2 
Quoting rojay@codeaurora.org (2021-05-16 23:32:50)
> Hi Stephen,

>

> Now, I have made the changes, calling i2c_mark_adapter_suspended() in

> shutdown() and i2c_mark_adapter_suspended()/_resumed() from runtime

> suspend/resume also and validated the changes. I have also picked

> your patch [1] for this validation.

>

> During the device boot up I am seeing multiple traces shown below.

> Are these expected now and needs to be fixed from rt5682/respective

> client driver?

>

> Trace1:

> [   11.709477] i2c i2c-9: Transfer while suspended

> [   11.905595] Call trace:

> [   11.908124]  __i2c_transfer+0xb8/0x38c

> [   11.911984]  i2c_transfer+0xa0/0xf4

> [   11.915569]  i2c_transfer_buffer_flags+0x68/0x9c

> [   11.920314]  regmap_i2c_write+0x34/0x64

> [   11.924255]  _regmap_raw_write_impl+0x4e8/0x7bc

> [   11.928911]  _regmap_bus_raw_write+0x70/0x8c

> [   11.933301]  _regmap_write+0x100/0x150

> [   11.937152]  regmap_write+0x54/0x78

> [   11.940744]  soc_component_write_no_lock+0x34/0xa8

> [   11.945666]  snd_soc_component_write+0x3c/0x5c

> [   11.950242]  rt5682_set_component_pll+0x1e4/0x2b4 [snd_soc_rt5682]

> [   11.956588]  snd_soc_component_set_pll+0x50/0xa8

> [   11.961328]  snd_soc_dai_set_pll+0x74/0xc8

> [   11.965542]  sc7180_snd_startup+0x9c/0x120 [snd_soc_sc7180]

> [   11.971262]  snd_soc_link_startup+0x34/0x88

> [   11.975557]  soc_pcm_open+0x100/0x538

> [   11.979323]  snd_pcm_open_substream+0x530/0x704

> [   11.983980]  snd_pcm_open+0xc8/0x210

> [   11.987653]  snd_pcm_playback_open+0x50/0x80

> [   11.992049]  snd_open+0x120/0x150

> [   11.995462]  chrdev_open+0xb8/0x1a4

> [   11.999056]  do_dentry_open+0x238/0x358

> [   12.003001]  vfs_open+0x34/0x40

> [   12.006235]  path_openat+0x9e8/0xd60

> [   12.009913]  do_filp_open+0x90/0x10c

> [   12.013587]  do_sys_open+0x148/0x314

> [   12.017260]  __arm64_compat_sys_openat+0x28/0x34

> [   12.022009]  el0_svc_common+0xa4/0x16c

> [   12.025860]  el0_svc_compat_handler+0x2c/0x40

> [   12.030337]  el0_svc_compat+0x8/0x10

> [   12.034018] ---[ end trace 745ead557fcbb5dc ]---


Ah I see. Maybe it isn't correct to mark the device as suspended in
runtime PM operations because the bus will be resumed during the
transfer? So only mark it suspended during system wide suspend/resume
transitions?

-Stephen

> [   12.040151] rt5682 9-001a: ASoC: error at soc_component_write_no_lock

> on rt5682.9-001a: -108

> [   12.049055] rt5682 9-001a: ASoC: error at soc_component_write_no_lock

> on rt5682.9-001a: -108

> [   12.057742] rt5682 9-001a: ASoC: error at

> snd_soc_component_update_bits on rt5682.9-001a: -108

>

> Trace2:

> [    3.515390] i2c i2c-2: Transfer while suspended

> [    3.606749] Call trace:

> [    3.606751]  __i2c_transfer+0xb8/0x38c

> [    3.606752]  i2c_transfer+0xa0/0xf4

> [    3.606754]  i2c_transfer_buffer_flags+0x68/0x9c

> [    3.639599] hub 2-1.4:1.0: USB hub found

> [    3.644375]  regmap_i2c_write+0x34/0x64

> [    3.644376]  _regmap_raw_write_impl+0x4e8/0x7bc

> [    3.644378]  _regmap_bus_raw_write+0x70/0x8c

> [    3.644379]  _regmap_write+0x100/0x150

> [    3.644381]  regmap_write+0x54/0x78

> [    3.644383]  ti_sn_aux_transfer+0x90/0x244

> [    3.650695] hub 2-1.4:1.0: 4 ports detected

> [    3.655288]  drm_dp_dpcd_access+0x8c/0x11c

> [    3.655289]  drm_dp_dpcd_read+0x64/0x10c

> [    3.655290]  ti_sn_bridge_enable+0x5c/0x824

> [    3.655292]  drm_atomic_bridge_chain_enable+0x78/0xa0

> [    3.655294]  drm_atomic_helper_commit_modeset_enables+0x198/0x238

> [    3.655295]  msm_atomic_commit_tail+0x324/0x714

> [    3.655297]  commit_tail+0xa4/0x108

> [    3.664985] usb 1-1.4: new high-speed USB device number 4 using

> xhci-hcd

> [    3.666204]  drm_atomic_helper_commit+0xf4/0xfc

> [    3.666205]  drm_atomic_commit+0x50/0x5c

> [    3.666206]  drm_atomic_helper_set_config+0x64/0x98

> [    3.666208]  drm_mode_setcrtc+0x26c/0x590

> [    3.666209]  drm_ioctl_kernel+0x9c/0x114

> [    3.701074] hub 2-1.4:1.0: USB hub found

> [    3.703347]  drm_ioctl+0x288/0x420

> [    3.703349]  drm_compat_ioctl+0xd0/0xe0

> [    3.703351]  __arm64_compat_sys_ioctl+0x100/0x2108

> [    3.703354]  el0_svc_common+0xa4/0x16c

> [    3.708499] hub 2-1.4:1.0: 4 ports detected

> [    3.711588]  el0_svc_compat_handler+0x2c/0x40

> [    3.711590]  el0_svc_compat+0x8/0x10

> [    3.711591] ---[ end trace 745ead557fcbb5db ]---

> [    3.772120] usb 1-1.4: New USB device found, idVendor=0bda,

> idProduct=5411, bcdDevice= 1.04

> [    3.794990] ti_sn65dsi86 2-002d: [drm:ti_sn_bridge_enable] *ERROR*

> Can't read lane count (-108); assuming 4

>

> [1]

> https://lore.kernel.org/r/20210508075151.1626903-2-swboyd@chromium.org

>
Roja Rani Yarubandi May 21, 2021, 4:12 p.m. UTC | #3 
On 2021-05-20 13:45, Stephen Boyd wrote:
> Quoting rojay@codeaurora.org (2021-05-16 23:32:50)

>> Hi Stephen,

>> 

>> Now, I have made the changes, calling i2c_mark_adapter_suspended() in

>> shutdown() and i2c_mark_adapter_suspended()/_resumed() from runtime

>> suspend/resume also and validated the changes. I have also picked

>> your patch [1] for this validation.

>> 

>> During the device boot up I am seeing multiple traces shown below.

>> Are these expected now and needs to be fixed from rt5682/respective

>> client driver?

>> 

>> Trace1:

>> [   11.709477] i2c i2c-9: Transfer while suspended

>> [   11.905595] Call trace:

>> [   11.908124]  __i2c_transfer+0xb8/0x38c

>> [   11.911984]  i2c_transfer+0xa0/0xf4

>> [   11.915569]  i2c_transfer_buffer_flags+0x68/0x9c

>> [   11.920314]  regmap_i2c_write+0x34/0x64

>> [   11.924255]  _regmap_raw_write_impl+0x4e8/0x7bc

>> [   11.928911]  _regmap_bus_raw_write+0x70/0x8c

>> [   11.933301]  _regmap_write+0x100/0x150

>> [   11.937152]  regmap_write+0x54/0x78

>> [   11.940744]  soc_component_write_no_lock+0x34/0xa8

>> [   11.945666]  snd_soc_component_write+0x3c/0x5c

>> [   11.950242]  rt5682_set_component_pll+0x1e4/0x2b4 [snd_soc_rt5682]

>> [   11.956588]  snd_soc_component_set_pll+0x50/0xa8

>> [   11.961328]  snd_soc_dai_set_pll+0x74/0xc8

>> [   11.965542]  sc7180_snd_startup+0x9c/0x120 [snd_soc_sc7180]

>> [   11.971262]  snd_soc_link_startup+0x34/0x88

>> [   11.975557]  soc_pcm_open+0x100/0x538

>> [   11.979323]  snd_pcm_open_substream+0x530/0x704

>> [   11.983980]  snd_pcm_open+0xc8/0x210

>> [   11.987653]  snd_pcm_playback_open+0x50/0x80

>> [   11.992049]  snd_open+0x120/0x150

>> [   11.995462]  chrdev_open+0xb8/0x1a4

>> [   11.999056]  do_dentry_open+0x238/0x358

>> [   12.003001]  vfs_open+0x34/0x40

>> [   12.006235]  path_openat+0x9e8/0xd60

>> [   12.009913]  do_filp_open+0x90/0x10c

>> [   12.013587]  do_sys_open+0x148/0x314

>> [   12.017260]  __arm64_compat_sys_openat+0x28/0x34

>> [   12.022009]  el0_svc_common+0xa4/0x16c

>> [   12.025860]  el0_svc_compat_handler+0x2c/0x40

>> [   12.030337]  el0_svc_compat+0x8/0x10

>> [   12.034018] ---[ end trace 745ead557fcbb5dc ]---

> 

> Ah I see. Maybe it isn't correct to mark the device as suspended in

> runtime PM operations because the bus will be resumed during the

> transfer? So only mark it suspended during system wide suspend/resume

> transitions?

> 

> -Stephen

> 


Yes, we cannot mark device as suspended/resumed during
runtime PM operations. Bus will be resumed during i2c
transfers and before transfer initiation, in __i2c_transfer()
from i2c-core-base.c there is a check to see whether the device
is marked as suspended with "__i2c_check_suspended(adap)" call,
which is "true" in this case and returning from there.

To mark it only suspended during system wide suspend/resume
transitions, currently our geni i2c driver has only
system_suspend implemented (geni_i2c_suspend_noirq()) and
does not have system_resume implemented, which again causes i2c
transfers to fail during system_resume after system_suspend.

Shall I go ahead with marking device suspended during
shutdown() only?

-Roja

>> [   12.040151] rt5682 9-001a: ASoC: error at 

>> soc_component_write_no_lock

>> on rt5682.9-001a: -108

>> [   12.049055] rt5682 9-001a: ASoC: error at 

>> soc_component_write_no_lock

>> on rt5682.9-001a: -108

>> [   12.057742] rt5682 9-001a: ASoC: error at

>> snd_soc_component_update_bits on rt5682.9-001a: -108

>> 

>> Trace2:

>> [    3.515390] i2c i2c-2: Transfer while suspended

>> [    3.606749] Call trace:

>> [    3.606751]  __i2c_transfer+0xb8/0x38c

>> [    3.606752]  i2c_transfer+0xa0/0xf4

>> [    3.606754]  i2c_transfer_buffer_flags+0x68/0x9c

>> [    3.639599] hub 2-1.4:1.0: USB hub found

>> [    3.644375]  regmap_i2c_write+0x34/0x64

>> [    3.644376]  _regmap_raw_write_impl+0x4e8/0x7bc

>> [    3.644378]  _regmap_bus_raw_write+0x70/0x8c

>> [    3.644379]  _regmap_write+0x100/0x150

>> [    3.644381]  regmap_write+0x54/0x78

>> [    3.644383]  ti_sn_aux_transfer+0x90/0x244

>> [    3.650695] hub 2-1.4:1.0: 4 ports detected

>> [    3.655288]  drm_dp_dpcd_access+0x8c/0x11c

>> [    3.655289]  drm_dp_dpcd_read+0x64/0x10c

>> [    3.655290]  ti_sn_bridge_enable+0x5c/0x824

>> [    3.655292]  drm_atomic_bridge_chain_enable+0x78/0xa0

>> [    3.655294]  drm_atomic_helper_commit_modeset_enables+0x198/0x238

>> [    3.655295]  msm_atomic_commit_tail+0x324/0x714

>> [    3.655297]  commit_tail+0xa4/0x108

>> [    3.664985] usb 1-1.4: new high-speed USB device number 4 using

>> xhci-hcd

>> [    3.666204]  drm_atomic_helper_commit+0xf4/0xfc

>> [    3.666205]  drm_atomic_commit+0x50/0x5c

>> [    3.666206]  drm_atomic_helper_set_config+0x64/0x98

>> [    3.666208]  drm_mode_setcrtc+0x26c/0x590

>> [    3.666209]  drm_ioctl_kernel+0x9c/0x114

>> [    3.701074] hub 2-1.4:1.0: USB hub found

>> [    3.703347]  drm_ioctl+0x288/0x420

>> [    3.703349]  drm_compat_ioctl+0xd0/0xe0

>> [    3.703351]  __arm64_compat_sys_ioctl+0x100/0x2108

>> [    3.703354]  el0_svc_common+0xa4/0x16c

>> [    3.708499] hub 2-1.4:1.0: 4 ports detected

>> [    3.711588]  el0_svc_compat_handler+0x2c/0x40

>> [    3.711590]  el0_svc_compat+0x8/0x10

>> [    3.711591] ---[ end trace 745ead557fcbb5db ]---

>> [    3.772120] usb 1-1.4: New USB device found, idVendor=0bda,

>> idProduct=5411, bcdDevice= 1.04

>> [    3.794990] ti_sn65dsi86 2-002d: [drm:ti_sn_bridge_enable] *ERROR*

>> Can't read lane count (-108); assuming 4

>> 

>> [1]

>> https://lore.kernel.org/r/20210508075151.1626903-2-swboyd@chromium.org

>>
Stephen Boyd May 21, 2021, 7:20 p.m. UTC | #4 
Quoting rojay@codeaurora.org (2021-05-21 09:12:02)
>

> Yes, we cannot mark device as suspended/resumed during

> runtime PM operations. Bus will be resumed during i2c

> transfers and before transfer initiation, in __i2c_transfer()

> from i2c-core-base.c there is a check to see whether the device

> is marked as suspended with "__i2c_check_suspended(adap)" call,

> which is "true" in this case and returning from there.

>

> To mark it only suspended during system wide suspend/resume

> transitions, currently our geni i2c driver has only

> system_suspend implemented (geni_i2c_suspend_noirq()) and

> does not have system_resume implemented, which again causes i2c

> transfers to fail during system_resume after system_suspend.


Got it.

>

> Shall I go ahead with marking device suspended during

> shutdown() only?

>


Yes that sounds good. Can you send two patches, one to mark suspend in
the system PM paths and one in the shutdown path? There are various bugs
to fix in other drivers that are trying to use the i2c bus after it has
been suspended and shutdown, but that doesn't block these patches from
moving forward. I'll take a look and see what's going on with
suspend/resume on my board.
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 214b4c913a13..277ab7e7dd51 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -650,6 +650,14 @@  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);
+
+	if (!pm_runtime_status_suspended(gi2c->se.dev))
+		pm_runtime_set_suspended(gi2c->se.dev);
+}
+
 static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
 {
 	int ret;
@@ -714,6 +722,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,