| Message ID | 20210512082220.7137-1-rojay@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c | expand |
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. 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? ----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; }
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
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 >
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 >>
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 --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,
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(+)