Re: [PATCH v2 0/4] Fix device_lock deadlock on two probe() paths

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hi Jason,

On 09.08.2023 16:43, Jason Gunthorpe wrote:
> I missed two paths where __iommu_probe_device() can be called while
> already holding the device_lock() for the device that is to be probed.
>
> This causes a deadlock because __iommu_probe_device() will attempt to
> re-acquire the lock.
>
> Organize things so that these two paths can re-use the existing already
> held device_lock by marking the call chains based on if the lock is held
> or not.
>
> Also the order of iommu_init_device() was not correct for
> generic_single_device_group()

I've just noticed that there is still one more issue left to fix after 
applying this patchset. On Qualcomm's RB5 board I get the following 
warning (captured on vanilla next-20230817):

------------[ cut here ]------------
WARNING: CPU: 6 PID: 274 at include/linux/device.h:1012 
iommu_probe_device_locked+0xd4/0xe4
Modules linked in: apr pdr_interface venus_dec venus_enc 
videobuf2_dma_contig rpmsg_ctrl fastrpc qrtr_smd rpmsg_char 
lontium_lt9611uxc msm mcp251xfd qcom_camss can_dev videobuf2_dma_sg 
ocmem imx412 gpu_sched venus_core snd_soc_sm8250 phy_qcom_qmp_combo 
videobuf2_memops v4l2_fwnode leds_qcom_lpg v4l2_mem2mem drm_dp_aux_bus 
v4l2_async snd_soc_qcom_sdw videobuf2_v4l2 ax88179_178a onboard_usb_hub 
videodev rtc_pm8xxx qcom_spmi_adc5 qcom_pon qcom_spmi_adc_tm5 
led_class_multicolor qcom_spmi_temp_alarm qcom_vadc_common crct10dif_ce 
i2c_qcom_geni snd_soc_qcom_common qrtr drm_display_helper 
videobuf2_common camcc_sm8250 mc typec i2c_qcom_cci spi_geni_qcom 
qcom_stats llcc_qcom phy_qcom_qmp_usb qcom_rng icc_bwmon qcom_q6v5_pas 
qcrypto phy_qcom_snps_femto_v2 qcom_pil_info sha256_generic 
soundwire_qcom coresight_stm coresight_tmc stm_core coresight_funnel 
coresight_replicator libsha256 qcom_q6v5 pinctrl_sm8250_lpass_lpi 
soundwire_bus snd_soc_lpass_va_macro snd_soc_lpass_macro_common 
pinctrl_lpass_lpi coresight authenc
  lpass_gfm_sm8250 qcom_sysmon slimbus snd_soc_lpass_wsa_macro libdes 
qcom_common qcom_glink_smem socinfo mdt_loader qmi_helpers 
phy_qcom_qmp_pcie icc_osm_l3 qcom_wdt display_connector ip_tables 
x_tables ipv6
CPU: 6 PID: 274 Comm: kworker/u16:8 Not tainted 6.5.0-rc6-next-20230817 
#13867
Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
Workqueue: events_unbound deferred_probe_work_func
pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : iommu_probe_device_locked+0xd4/0xe4
lr : iommu_probe_device_locked+0x78/0xe4
...
Call trace:
  iommu_probe_device_locked+0xd4/0xe4
  of_iommu_configure+0x10c/0x200
  of_dma_configure_id+0x104/0x3b8
  a6xx_gmu_init+0x4c/0xccc [msm]
  a6xx_gpu_init+0x3ac/0x770 [msm]
  adreno_bind+0x174/0x2ac [msm]
  component_bind_all+0x118/0x24c
  msm_drm_bind+0x1e8/0x6c4 [msm]
  try_to_bring_up_aggregate_device+0x168/0x1d4
  __component_add+0xa8/0x170
  component_add+0x14/0x20
  dsi_dev_attach+0x20/0x2c [msm]
  dsi_host_attach+0x9c/0x144 [msm]
  devm_mipi_dsi_attach+0x34/0xb4
  lt9611uxc_attach_dsi.isra.0+0x84/0xfc [lontium_lt9611uxc]
  lt9611uxc_probe+0x5c8/0x68c [lontium_lt9611uxc]
  i2c_device_probe+0x14c/0x290
  really_probe+0x148/0x2b4
  __driver_probe_device+0x78/0x12c
  driver_probe_device+0x3c/0x160
  __device_attach_driver+0xb8/0x138
  bus_for_each_drv+0x84/0xe0
  __device_attach+0xa8/0x1b0
  device_initial_probe+0x14/0x20
  bus_probe_device+0xb0/0xb4
  deferred_probe_work_func+0x8c/0xc8
  process_one_work+0x1ec/0x53c
  worker_thread+0x298/0x408
  kthread+0x124/0x128
  ret_from_fork+0x10/0x20
irq event stamp: 207712
hardirqs last  enabled at (207711): [<ffffcfe16b46b160>] 
_raw_spin_unlock_irqrestore+0x74/0x78
hardirqs last disabled at (207712): [<ffffcfe16b458630>] el1_dbg+0x24/0x8c
softirqs last  enabled at (206480): [<ffffcfe16a290a10>] 
__do_softirq+0x438/0x4ec
softirqs last disabled at (206473): [<ffffcfe16a296980>] 
____do_softirq+0x10/0x1c
---[ end trace 0000000000000000 ]---
------------[ cut here ]------------

Let me know if you need more information or some tests on the hardware.


>
> v2:
>   - New patch to correct the order of setting iommu_dev during
>     iommu_init_device()
>   - Spelling fixes
>   - Simply block probing the iommu device itself instead of trying to do it
> v1: https://lore.kernel.org/r/0-v1-8612b9ef48da+333-iommu_group_locking2_jgg@nvidia.com
>
> Jason Gunthorpe (4):
>    iommu: Provide iommu_probe_device_locked()
>    iommu: Pass in the iommu_device to probe for in bus_iommu_probe()
>    iommu: Do not attempt to re-lock the iommu device when probing
>    iommu: dev->iommu->iommu_dev must be set before ops->device_group()
>
>   drivers/acpi/scan.c        |  5 +--
>   drivers/iommu/iommu.c      | 65 +++++++++++++++++++++++++++-----------
>   drivers/iommu/of_iommu.c   |  2 +-
>   drivers/iommu/omap-iommu.c | 12 +++++--
>   include/linux/iommu.h      |  6 +++-
>   5 files changed, 65 insertions(+), 25 deletions(-)
>
>
> base-commit: 8d3740021d5d461e1ec4c17fc5625b9b4237c2f8

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland