| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <70a90d229551bcec21ed74cfd1350b9b@codeaurora.org>
Date: Mon, 17 May 2021 12:02:50 +0530
From: rojay@...eaurora.org
To: Stephen Boyd <swboyd@...omium.org>
Cc: wsa@...nel.org, dianders@...omium.org,
saiprakash.ranjan@...eaurora.org, gregkh@...uxfoundation.org,
mka@...omium.org, skananth@...eaurora.org,
msavaliy@....qualcomm.com, skakit@...eaurora.org,
rnayak@...eaurora.org, agross@...nel.org,
bjorn.andersson@...aro.org, linux-arm-msm@...r.kernel.org,
linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
sumit.semwal@...aro.org, linux-media@...r.kernel.org
Subject: Re: [PATCH V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c
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@...eaurora.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
Powered by blists - more mailing lists