| Message ID | 20220405234551.359453-2-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: properly add and remove internal bridges | expand |
Quoting Dmitry Baryshkov (2022-04-05 16:45:48) > Add calls to drm_bridge_add()/drm_bridge_remove() for the internal HDMI > bridges. This fixes the following warning. > > [ 2.195003] ------------[ cut here ]------------ Usually this line is left out > [ 2.195044] WARNING: CPU: 0 PID: 1 at kernel/locking/mutex.c:579 __mutex_lock+0x840/0x9f4 And the timestamp is removed > [ 2.198774] DEBUG_LOCKS_WARN_ON(lock->magic != lock) > [ 2.198786] Modules linked in: > [ 2.211868] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-00002-g3054695a0d27-dirty #55 > [ 2.214671] Hardware name: Generic DT based system > [ 2.223265] unwind_backtrace from show_stack+0x10/0x14 > [ 2.228036] show_stack from dump_stack_lvl+0x58/0x70 > [ 2.233159] dump_stack_lvl from __warn+0xc8/0x1e8 > [ 2.238367] __warn from warn_slowpath_fmt+0x78/0xa8 > [ 2.243054] warn_slowpath_fmt from __mutex_lock+0x840/0x9f4 > [ 2.248174] __mutex_lock from mutex_lock_nested+0x1c/0x24 > [ 2.253818] mutex_lock_nested from drm_bridge_hpd_enable+0x2c/0x84 > [ 2.259116] drm_bridge_hpd_enable from msm_hdmi_modeset_init+0xc0/0x21c > [ 2.265279] msm_hdmi_modeset_init from mdp4_kms_init+0x53c/0x90c > [ 2.272223] mdp4_kms_init from msm_drm_bind+0x514/0x698 > [ 2.278212] msm_drm_bind from try_to_bring_up_aggregate_device+0x160/0x1bc I'd probably cut it off here. Is there any Fixes tag for this? Still seems worthwhile to have one even if this is a lockdep warning.
On 06/04/2022 05:51, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2022-04-05 16:45:48) >> Add calls to drm_bridge_add()/drm_bridge_remove() for the internal HDMI >> bridges. This fixes the following warning. >> >> [ 2.195003] ------------[ cut here ]------------ > > Usually this line is left out > >> [ 2.195044] WARNING: CPU: 0 PID: 1 at kernel/locking/mutex.c:579 __mutex_lock+0x840/0x9f4 > > And the timestamp is removed Ack for both notes, will fix in v2 > >> [ 2.198774] DEBUG_LOCKS_WARN_ON(lock->magic != lock) >> [ 2.198786] Modules linked in: >> [ 2.211868] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-00002-g3054695a0d27-dirty #55 >> [ 2.214671] Hardware name: Generic DT based system >> [ 2.223265] unwind_backtrace from show_stack+0x10/0x14 >> [ 2.228036] show_stack from dump_stack_lvl+0x58/0x70 >> [ 2.233159] dump_stack_lvl from __warn+0xc8/0x1e8 >> [ 2.238367] __warn from warn_slowpath_fmt+0x78/0xa8 >> [ 2.243054] warn_slowpath_fmt from __mutex_lock+0x840/0x9f4 >> [ 2.248174] __mutex_lock from mutex_lock_nested+0x1c/0x24 >> [ 2.253818] mutex_lock_nested from drm_bridge_hpd_enable+0x2c/0x84 >> [ 2.259116] drm_bridge_hpd_enable from msm_hdmi_modeset_init+0xc0/0x21c >> [ 2.265279] msm_hdmi_modeset_init from mdp4_kms_init+0x53c/0x90c >> [ 2.272223] mdp4_kms_init from msm_drm_bind+0x514/0x698 >> [ 2.278212] msm_drm_bind from try_to_bring_up_aggregate_device+0x160/0x1bc > > I'd probably cut it off here. ack > > Is there any Fixes tag for this? Still seems worthwhile to have one even > if this is a lockdep warning. I thought about this before sending v1, but ended up not doing so. Each of these changes is not atomic. A call to drm_bridge_add() without final drm_bridge_remove() in the core msm code would leave dangling pointers in the drm core. A drm_bridge_remove() is not sensible without converting _all_ users. So there are two alternatives: - leave this patch series as is w/o a Fixes tag - squash four patches into a single patch and add 'Fixes: a3376e3ec81c ("drm/msm: convert to drm_bridge")' tag What would you prefer?
Quoting Dmitry Baryshkov (2022-04-06 03:21:25) > On 06/04/2022 05:51, Stephen Boyd wrote: > > > > Is there any Fixes tag for this? Still seems worthwhile to have one even > > if this is a lockdep warning. > I thought about this before sending v1, but ended up not doing so. Each > of these changes is not atomic. A call to drm_bridge_add() without final > drm_bridge_remove() in the core msm code would leave dangling pointers > in the drm core. A drm_bridge_remove() is not sensible without > converting _all_ users. > > So there are two alternatives: > - leave this patch series as is w/o a Fixes tag > - squash four patches into a single patch and add 'Fixes: a3376e3ec81c > ("drm/msm: convert to drm_bridge")' tag > The atomic change. That's because bisection could trip over these patches and then break because it picked the middle of the patch series. One atomic commit fixes that.
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c index 10ebe2089cb6..97c24010c4d1 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c @@ -15,6 +15,7 @@ void msm_hdmi_bridge_destroy(struct drm_bridge *bridge) struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge); msm_hdmi_hpd_disable(hdmi_bridge); + drm_bridge_remove(bridge); } static void msm_hdmi_power_on(struct drm_bridge *bridge) @@ -349,6 +350,8 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi) DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID; + drm_bridge_add(bridge); + ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (ret) goto fail;
Add calls to drm_bridge_add()/drm_bridge_remove() for the internal HDMI bridges. This fixes the following warning. [ 2.195003] ------------[ cut here ]------------ [ 2.195044] WARNING: CPU: 0 PID: 1 at kernel/locking/mutex.c:579 __mutex_lock+0x840/0x9f4 [ 2.198774] DEBUG_LOCKS_WARN_ON(lock->magic != lock) [ 2.198786] Modules linked in: [ 2.211868] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-00002-g3054695a0d27-dirty #55 [ 2.214671] Hardware name: Generic DT based system [ 2.223265] unwind_backtrace from show_stack+0x10/0x14 [ 2.228036] show_stack from dump_stack_lvl+0x58/0x70 [ 2.233159] dump_stack_lvl from __warn+0xc8/0x1e8 [ 2.238367] __warn from warn_slowpath_fmt+0x78/0xa8 [ 2.243054] warn_slowpath_fmt from __mutex_lock+0x840/0x9f4 [ 2.248174] __mutex_lock from mutex_lock_nested+0x1c/0x24 [ 2.253818] mutex_lock_nested from drm_bridge_hpd_enable+0x2c/0x84 [ 2.259116] drm_bridge_hpd_enable from msm_hdmi_modeset_init+0xc0/0x21c [ 2.265279] msm_hdmi_modeset_init from mdp4_kms_init+0x53c/0x90c [ 2.272223] mdp4_kms_init from msm_drm_bind+0x514/0x698 [ 2.278212] msm_drm_bind from try_to_bring_up_aggregate_device+0x160/0x1bc [ 2.283595] try_to_bring_up_aggregate_device from component_master_add_with_match+0xc4/0xf8 [ 2.290281] component_master_add_with_match from msm_pdev_probe+0x274/0x350 [ 2.298958] msm_pdev_probe from platform_probe+0x5c/0xbc [ 2.305990] platform_probe from really_probe.part.0+0x9c/0x290 [ 2.311284] really_probe.part.0 from __driver_probe_device+0xa8/0x13c [ 2.317011] __driver_probe_device from driver_probe_device+0x34/0x10c [ 2.323610] driver_probe_device from __driver_attach+0xbc/0x178 [ 2.330122] __driver_attach from bus_for_each_dev+0x74/0xc0 [ 2.336282] bus_for_each_dev from bus_add_driver+0x160/0x1e4 [ 2.341925] bus_add_driver from driver_register+0x88/0x118 [ 2.347570] driver_register from do_one_initcall+0x6c/0x334 [ 2.352953] do_one_initcall from kernel_init_freeable+0x1bc/0x220 [ 2.358853] kernel_init_freeable from kernel_init+0x18/0x12c [ 2.364842] kernel_init from ret_from_fork+0x14/0x2c [ 2.370657] Exception stack(0xf081dfb0 to 0xf081dff8) [ 2.375693] dfa0: 00000000 00000000 00000000 00000000 [ 2.380733] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 2.388893] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 2.397170] irq event stamp: 202911 [ 2.403551] hardirqs last enabled at (202911): [<c0f86044>] _raw_spin_unlock_irqrestore+0x44/0x48 [ 2.406944] hardirqs last disabled at (202910): [<c0f85e18>] _raw_spin_lock_irqsave+0x68/0x6c [ 2.416063] softirqs last enabled at (202738): [<c03015f0>] __do_softirq+0x2f8/0x530 [ 2.424638] softirqs last disabled at (202733): [<c032bc68>] __irq_exit_rcu+0x14c/0x170 [ 2.432453] ---[ end trace 0000000000000000 ]--- Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 3 +++ 1 file changed, 3 insertions(+)