| Message ID | 20230208190200.2966723-1-abel.vesa@linaro.org |
|---|---|
| Headers | show |
| Series | sm8550: Add support for eUSB2 repeater | expand |
On 08-02-23, 21:01, Abel Vesa wrote: > This patchset adds support for the eUSB2 repeater found in pmic PM8550B, > used along with SM8550. Since there is no dedicated generic framework > for eUSB2 repeaters, the most appropriate subsystem to model it is the > generic phy. This patchset also adds support for such repeater to the > eUSB2 PHY found in SM8550. Basically, the eUSB2 PHY will have its own > "phy" which is actually a repeater. Applied phy patches, thanks
On Wed, 8 Feb 2023 21:01:53 +0200, Abel Vesa wrote: > This patchset adds support for the eUSB2 repeater found in pmic PM8550B, > used along with SM8550. Since there is no dedicated generic framework > for eUSB2 repeaters, the most appropriate subsystem to model it is the > generic phy. This patchset also adds support for such repeater to the > eUSB2 PHY found in SM8550. Basically, the eUSB2 PHY will have its own > "phy" which is actually a repeater. > > [...] Applied, thanks! [6/7] arm64: dts: qcom: pm8550b: Add eUSB2 repeater node commit: fdaa922585f4474c88fbfaa129f8114f38200660 [7/7] arm64: dts: qcom: sm8550-mtp: Add eUSB2 repeater node commit: 749078e38e1ee28357e8fdcd4eac382a465bd1e7 Best regards,
On Wed, Feb 08, 2023 at 09:01:53PM +0200, Abel Vesa wrote: > This patchset adds support for the eUSB2 repeater found in pmic PM8550B, > used along with SM8550. Since there is no dedicated generic framework > for eUSB2 repeaters, the most appropriate subsystem to model it is the > generic phy. This patchset also adds support for such repeater to the > eUSB2 PHY found in SM8550. Basically, the eUSB2 PHY will have its own > "phy" which is actually a repeater. The decision to model the repeater as a PHY unfortunately breaks lockdep as you now have functions like phy_init() calling phy_init() for a second PHY (the repeater, see splat below). As long as the locks are always taken in the same order there should be no risk for a deadlock, but can you please verify that and add the missing lockdep annotation so that lockdep can be used on platforms like x1e80100 (e.g. to prevent further locking issues from being introduced)? Johan [ 8.613248] ============================================ [ 8.669073] WARNING: possible recursive locking detected [ 8.669074] 6.10.0-rc5 #122 Not tainted [ 8.669075] -------------------------------------------- [ 8.669075] kworker/u50:0/77 is trying to acquire lock: [ 8.669076] ffff5cae8733ecf8 (&phy->mutex){+.+.}-{3:3}, at: phy_init+0x4c/0x12c [ 8.669087] but task is already holding lock: [ 8.669088] ffff5cae8a056cf8 (&phy->mutex){+.+.}-{3:3}, at: phy_init+0x4c/0x12c [ 8.669092] other info that might help us debug this: [ 8.669092] Possible unsafe locking scenario: [ 8.669093] CPU0 [ 8.669093] ---- [ 8.669094] lock(&phy->mutex); [ 8.669095] lock(&phy->mutex); [ 8.669097] *** DEADLOCK *** [ 8.669097] May be due to missing lock nesting notation [ 8.669097] 4 locks held by kworker/u50:0/77: [ 8.669099] #0: ffff5cae80010948 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x1a4/0x638 [ 8.669108] #1: ffff800080333de0 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1cc/0x638 [ 8.669112] #2: ffff5cae854038f8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x38/0x1d4 [ 8.669117] #3: ffff5cae8a056cf8 (&phy->mutex){+.+.}-{3:3}, at: phy_init+0x4c/0x12c [ 8.669121] stack backtrace: [ 8.669122] CPU: 9 PID: 77 Comm: kworker/u50:0 Not tainted 6.10.0-rc5 #122 [ 8.669124] Hardware name: Qualcomm CRD, BIOS 6.0.231221.BOOT.MXF.2.4-00348.1-HAMOA-1 12/21/2023 [ 8.669125] Workqueue: events_unbound deferred_probe_work_func [ 8.669128] Call trace: [ 8.669129] dump_backtrace+0x9c/0x11c [ 8.870384] show_stack+0x18/0x24 [ 8.870386] dump_stack_lvl+0x90/0xd0 [ 8.870391] dump_stack+0x18/0x24 [ 8.870393] print_deadlock_bug+0x25c/0x348 [ 8.870396] __lock_acquire+0x10a4/0x2064 [ 8.870399] lock_acquire.part.0+0xc8/0x20c [ 8.870401] lock_acquire+0x68/0x84 [ 8.870403] __mutex_lock+0x98/0x428 [ 8.870407] mutex_lock_nested+0x24/0x30 [ 8.870410] phy_init+0x4c/0x12c [ 8.870412] qcom_snps_eusb2_hsphy_init+0x54/0x420 [phy_qcom_snps_eusb2] [ 8.870416] phy_init+0xe0/0x12c [ 8.870418] dwc3_core_init+0x484/0x1214 [ 8.870421] dwc3_probe+0xe54/0x171c [ 8.870424] platform_probe+0x68/0xd8 [ 8.870426] really_probe+0xc0/0x388 [ 8.870427] __driver_probe_device+0x7c/0x160 [ 8.870429] driver_probe_device+0x40/0x114 [ 8.870430] __device_attach_driver+0xbc/0x158 [ 8.870432] bus_for_each_drv+0x84/0xe0 [ 8.870433] __device_attach+0xa8/0x1d4 [ 8.870435] device_initial_probe+0x14/0x20 [ 8.870436] bus_probe_device+0xb0/0xb4 [ 8.870437] deferred_probe_work_func+0xa0/0xf4 [ 8.870439] process_one_work+0x224/0x638 [ 8.870441] worker_thread+0x268/0x3a8 [ 8.870442] kthread+0x124/0x128 [ 8.870443] ret_from_fork+0x10/0x20
On 24-06-25 08:47:29, Johan Hovold wrote: > On Wed, Feb 08, 2023 at 09:01:53PM +0200, Abel Vesa wrote: > > This patchset adds support for the eUSB2 repeater found in pmic PM8550B, > > used along with SM8550. Since there is no dedicated generic framework > > for eUSB2 repeaters, the most appropriate subsystem to model it is the > > generic phy. This patchset also adds support for such repeater to the > > eUSB2 PHY found in SM8550. Basically, the eUSB2 PHY will have its own > > "phy" which is actually a repeater. > > The decision to model the repeater as a PHY unfortunately breaks lockdep > as you now have functions like phy_init() calling phy_init() for a > second PHY (the repeater, see splat below). > This was reported by Bjorn off-list a couple of months ago. I did check it then and the order is perfectly fine. The solution here should be to use mutex_lock_nested in the PHY framework. This would allow supporting chain-linked PHYs. The possibility of moving out the repeater out of PHY was also discussed. Unfortunately, I didn't have the bandwidth to circle back and properly investigate and fix it. > As long as the locks are always taken in the same order there should be > no risk for a deadlock, but can you please verify that and add the > missing lockdep annotation so that lockdep can be used on platforms like > x1e80100 (e.g. to prevent further locking issues from being introduced)? > > Johan > > > [ 8.613248] ============================================ > [ 8.669073] WARNING: possible recursive locking detected > [ 8.669074] 6.10.0-rc5 #122 Not tainted > [ 8.669075] -------------------------------------------- > [ 8.669075] kworker/u50:0/77 is trying to acquire lock: > [ 8.669076] ffff5cae8733ecf8 (&phy->mutex){+.+.}-{3:3}, at: phy_init+0x4c/0x12c > [ 8.669087] > but task is already holding lock: > [ 8.669088] ffff5cae8a056cf8 (&phy->mutex){+.+.}-{3:3}, at: phy_init+0x4c/0x12c > [ 8.669092] > other info that might help us debug this: > [ 8.669092] Possible unsafe locking scenario: > > [ 8.669093] CPU0 > [ 8.669093] ---- > [ 8.669094] lock(&phy->mutex); > [ 8.669095] lock(&phy->mutex); > [ 8.669097] > *** DEADLOCK *** > > [ 8.669097] May be due to missing lock nesting notation > > [ 8.669097] 4 locks held by kworker/u50:0/77: > [ 8.669099] #0: ffff5cae80010948 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x1a4/0x638 > [ 8.669108] #1: ffff800080333de0 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1cc/0x638 > [ 8.669112] #2: ffff5cae854038f8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x38/0x1d4 > [ 8.669117] #3: ffff5cae8a056cf8 (&phy->mutex){+.+.}-{3:3}, at: phy_init+0x4c/0x12c > [ 8.669121] > stack backtrace: > [ 8.669122] CPU: 9 PID: 77 Comm: kworker/u50:0 Not tainted 6.10.0-rc5 #122 > [ 8.669124] Hardware name: Qualcomm CRD, BIOS 6.0.231221.BOOT.MXF.2.4-00348.1-HAMOA-1 12/21/2023 > [ 8.669125] Workqueue: events_unbound deferred_probe_work_func > [ 8.669128] Call trace: > [ 8.669129] dump_backtrace+0x9c/0x11c > [ 8.870384] show_stack+0x18/0x24 > [ 8.870386] dump_stack_lvl+0x90/0xd0 > [ 8.870391] dump_stack+0x18/0x24 > [ 8.870393] print_deadlock_bug+0x25c/0x348 > [ 8.870396] __lock_acquire+0x10a4/0x2064 > [ 8.870399] lock_acquire.part.0+0xc8/0x20c > [ 8.870401] lock_acquire+0x68/0x84 > [ 8.870403] __mutex_lock+0x98/0x428 > [ 8.870407] mutex_lock_nested+0x24/0x30 > [ 8.870410] phy_init+0x4c/0x12c > [ 8.870412] qcom_snps_eusb2_hsphy_init+0x54/0x420 [phy_qcom_snps_eusb2] > [ 8.870416] phy_init+0xe0/0x12c > [ 8.870418] dwc3_core_init+0x484/0x1214 > [ 8.870421] dwc3_probe+0xe54/0x171c > [ 8.870424] platform_probe+0x68/0xd8 > [ 8.870426] really_probe+0xc0/0x388 > [ 8.870427] __driver_probe_device+0x7c/0x160 > [ 8.870429] driver_probe_device+0x40/0x114 > [ 8.870430] __device_attach_driver+0xbc/0x158 > [ 8.870432] bus_for_each_drv+0x84/0xe0 > [ 8.870433] __device_attach+0xa8/0x1d4 > [ 8.870435] device_initial_probe+0x14/0x20 > [ 8.870436] bus_probe_device+0xb0/0xb4 > [ 8.870437] deferred_probe_work_func+0xa0/0xf4 > [ 8.870439] process_one_work+0x224/0x638 > [ 8.870441] worker_thread+0x268/0x3a8 > [ 8.870442] kthread+0x124/0x128 > [ 8.870443] ret_from_fork+0x10/0x20
On 25/06/2024 09:37, Abel Vesa wrote: > On 24-06-25 08:47:29, Johan Hovold wrote: >> On Wed, Feb 08, 2023 at 09:01:53PM +0200, Abel Vesa wrote: >>> This patchset adds support for the eUSB2 repeater found in pmic PM8550B, >>> used along with SM8550. Since there is no dedicated generic framework >>> for eUSB2 repeaters, the most appropriate subsystem to model it is the >>> generic phy. This patchset also adds support for such repeater to the >>> eUSB2 PHY found in SM8550. Basically, the eUSB2 PHY will have its own >>> "phy" which is actually a repeater. >> >> The decision to model the repeater as a PHY unfortunately breaks lockdep >> as you now have functions like phy_init() calling phy_init() for a >> second PHY (the repeater, see splat below). >> > > This was reported by Bjorn off-list a couple of months ago. I did check > it then and the order is perfectly fine. The solution here should be to > use mutex_lock_nested in the PHY framework. This would allow supporting > chain-linked PHYs. The possibility of moving out the repeater out of PHY > was also discussed. Unfortunately, I didn't have the bandwidth to > circle back and properly investigate and fix it. Well technically it's a PHY, and moving out from PHY will basically duplicate the PHY core code... so we should rather make sure we can call phy code from phy callbacks safely. Neil > >> As long as the locks are always taken in the same order there should be >> no risk for a deadlock, but can you please verify that and add the >> missing lockdep annotation so that lockdep can be used on platforms like >> x1e80100 (e.g. to prevent further locking issues from being introduced)? >> >> Johan >> >> >> [ 8.613248] ============================================ >> [ 8.669073] WARNING: possible recursive locking detected >> [ 8.669074] 6.10.0-rc5 #122 Not tainted >> [ 8.669075] -------------------------------------------- >> [ 8.669075] kworker/u50:0/77 is trying to acquire lock: >> [ 8.669076] ffff5cae8733ecf8 (&phy->mutex){+.+.}-{3:3}, at: phy_init+0x4c/0x12c >> [ 8.669087] >> but task is already holding lock: >> [ 8.669088] ffff5cae8a056cf8 (&phy->mutex){+.+.}-{3:3}, at: phy_init+0x4c/0x12c >> [ 8.669092] >> other info that might help us debug this: >> [ 8.669092] Possible unsafe locking scenario: >> >> [ 8.669093] CPU0 >> [ 8.669093] ---- >> [ 8.669094] lock(&phy->mutex); >> [ 8.669095] lock(&phy->mutex); >> [ 8.669097] >> *** DEADLOCK *** >> >> [ 8.669097] May be due to missing lock nesting notation >> >> [ 8.669097] 4 locks held by kworker/u50:0/77: >> [ 8.669099] #0: ffff5cae80010948 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x1a4/0x638 >> [ 8.669108] #1: ffff800080333de0 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1cc/0x638 >> [ 8.669112] #2: ffff5cae854038f8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x38/0x1d4 >> [ 8.669117] #3: ffff5cae8a056cf8 (&phy->mutex){+.+.}-{3:3}, at: phy_init+0x4c/0x12c >> [ 8.669121] >> stack backtrace: >> [ 8.669122] CPU: 9 PID: 77 Comm: kworker/u50:0 Not tainted 6.10.0-rc5 #122 >> [ 8.669124] Hardware name: Qualcomm CRD, BIOS 6.0.231221.BOOT.MXF.2.4-00348.1-HAMOA-1 12/21/2023 >> [ 8.669125] Workqueue: events_unbound deferred_probe_work_func >> [ 8.669128] Call trace: >> [ 8.669129] dump_backtrace+0x9c/0x11c >> [ 8.870384] show_stack+0x18/0x24 >> [ 8.870386] dump_stack_lvl+0x90/0xd0 >> [ 8.870391] dump_stack+0x18/0x24 >> [ 8.870393] print_deadlock_bug+0x25c/0x348 >> [ 8.870396] __lock_acquire+0x10a4/0x2064 >> [ 8.870399] lock_acquire.part.0+0xc8/0x20c >> [ 8.870401] lock_acquire+0x68/0x84 >> [ 8.870403] __mutex_lock+0x98/0x428 >> [ 8.870407] mutex_lock_nested+0x24/0x30 >> [ 8.870410] phy_init+0x4c/0x12c >> [ 8.870412] qcom_snps_eusb2_hsphy_init+0x54/0x420 [phy_qcom_snps_eusb2] >> [ 8.870416] phy_init+0xe0/0x12c >> [ 8.870418] dwc3_core_init+0x484/0x1214 >> [ 8.870421] dwc3_probe+0xe54/0x171c >> [ 8.870424] platform_probe+0x68/0xd8 >> [ 8.870426] really_probe+0xc0/0x388 >> [ 8.870427] __driver_probe_device+0x7c/0x160 >> [ 8.870429] driver_probe_device+0x40/0x114 >> [ 8.870430] __device_attach_driver+0xbc/0x158 >> [ 8.870432] bus_for_each_drv+0x84/0xe0 >> [ 8.870433] __device_attach+0xa8/0x1d4 >> [ 8.870435] device_initial_probe+0x14/0x20 >> [ 8.870436] bus_probe_device+0xb0/0xb4 >> [ 8.870437] deferred_probe_work_func+0xa0/0xf4 >> [ 8.870439] process_one_work+0x224/0x638 >> [ 8.870441] worker_thread+0x268/0x3a8 >> [ 8.870442] kthread+0x124/0x128 >> [ 8.870443] ret_from_fork+0x10/0x20
This patchset adds support for the eUSB2 repeater found in pmic PM8550B, used along with SM8550. Since there is no dedicated generic framework for eUSB2 repeaters, the most appropriate subsystem to model it is the generic phy. This patchset also adds support for such repeater to the eUSB2 PHY found in SM8550. Basically, the eUSB2 PHY will have its own "phy" which is actually a repeater. This patchset is based on the following patchset: https://lore.kernel.org/all/20230208183421.2874423-1-abel.vesa@linaro.org/ Changes since v4: * added Krzysztof's R-b tag got patch #1 and A-b tag to patch #3 Changes since v3: * This time is a normal patchset, rather than an RFC Changes since v2: * moved the bindings qcom,spmi-pmic patch before the repeater schema patch in order to avoid the repeater schema validation failure due to phy pattern property not being found in qcom,spmi-pmic schema Changes since v1: * the repeater driver is implemented now as a PHY rather than adding a new generic framework for USB repeaters Abel Vesa (5): dt-bindings: phy: Add qcom,snps-eusb2-repeater schema file dt-bindings: mfd: qcom,spmi-pmic: Add pattern property for phy dt-bindings: phy: qcom,snps-eusb2-phy: Add phys property for the repeater phy: qcom: Add QCOM SNPS eUSB2 repeater driver arm64: dts: qcom: sm8550-mtp: Add eUSB2 repeater node Neil Armstrong (2): phy: qcom: phy-qcom-snps-eusb2: Add support for eUSB2 repeater arm64: dts: qcom: pm8550b: Add eUSB2 repeater node .../bindings/mfd/qcom,spmi-pmic.yaml | 4 + .../bindings/phy/qcom,snps-eusb2-phy.yaml | 5 + .../phy/qcom,snps-eusb2-repeater.yaml | 52 ++++ arch/arm64/boot/dts/qcom/pm8550b.dtsi | 6 + arch/arm64/boot/dts/qcom/sm8550-mtp.dts | 7 + drivers/phy/qualcomm/Kconfig | 9 + drivers/phy/qualcomm/Makefile | 1 + .../phy/qualcomm/phy-qcom-eusb2-repeater.c | 259 ++++++++++++++++++ drivers/phy/qualcomm/phy-qcom-snps-eusb2.c | 19 +- 9 files changed, 361 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/phy/qcom,snps-eusb2-repeater.yaml create mode 100644 drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c