| 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: <73e2a37e-23db-d614-5f5c-8120f1869158@quicinc.com>
Date: Fri, 22 Apr 2022 17:12:08 -0700
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Kuogee Hsieh <quic_khsieh@...cinc.com>, <robdclark@...il.com>,
<sean@...rly.run>, <swboyd@...omium.org>, <vkoul@...nel.org>,
<daniel@...ll.ch>, <airlied@...ux.ie>, <agross@...nel.org>,
<bjorn.andersson@...aro.org>
CC: <quic_sbillaka@...cinc.com>, <linux-arm-msm@...r.kernel.org>,
<dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
<quic_aravindh@...cinc.com>, <freedreno@...ts.freedesktop.org>
Subject: Re: [PATCH] drm/msm/dp: move add fail safe mode to
dp_connector_get_mode()
On 4/22/2022 5:07 PM, Dmitry Baryshkov wrote:
> On 23/04/2022 02:45, Kuogee Hsieh wrote:
>> Current DP driver implementation has adding safe mode done at
>> dp_hpd_plug_handle() which is expected to be executed under event
>> thread context.
>>
>> However there is possible circular locking happen (see blow stack trace)
>> after edp driver call dp_hpd_plug_handle() from dp_bridge_enable() which
>> is executed under drm_thread context.
>>
>> To break this circular locking, this patch have safe mode added at
>> dp_connector_get_mode() which is executed under drm thread context.
>> Therefore no lock acquired required for &dev->mode_config.mutex while
>> adding fail safe mode since it has been hold by drm thread already.
>>
>> ======================================================
>> WARNING: possible circular locking dependency detected
>> 5.15.35-lockdep #6 Tainted: G W
>> ------------------------------------------------------
>> frecon/429 is trying to acquire lock:
>> ffffff808dc3c4e8 (&dev->mode_config.mutex){+.+.}-{3:3}, at:
>> dp_panel_add_fail_safe_mode+0x4c/0xa0
>>
>> but task is already holding lock:
>> ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at:
>> lock_crtcs+0xb4/0x124
>>
>> which lock already depends on the new lock.
>>
>> the existing dependency chain (in reverse order) is:
>>
>> -> #3 (&kms->commit_lock[i]){+.+.}-{3:3}:
>> __mutex_lock_common+0x174/0x1a64
>> mutex_lock_nested+0x98/0xac
>> lock_crtcs+0xb4/0x124
>> msm_atomic_commit_tail+0x330/0x748
>> commit_tail+0x19c/0x278
>> drm_atomic_helper_commit+0x1dc/0x1f0
>> drm_atomic_commit+0xc0/0xd8
>> drm_atomic_helper_set_config+0xb4/0x134
>> drm_mode_setcrtc+0x688/0x1248
>> drm_ioctl_kernel+0x1e4/0x338
>> drm_ioctl+0x3a4/0x684
>> __arm64_sys_ioctl+0x118/0x154
>> invoke_syscall+0x78/0x224
>> el0_svc_common+0x178/0x200
>> do_el0_svc+0x94/0x13c
>> el0_svc+0x5c/0xec
>> el0t_64_sync_handler+0x78/0x108
>> el0t_64_sync+0x1a4/0x1a8
>>
>> -> #2 (crtc_ww_class_mutex){+.+.}-{3:3}:
>> __mutex_lock_common+0x174/0x1a64
>> ww_mutex_lock+0xb8/0x278
>> modeset_lock+0x304/0x4ac
>> drm_modeset_lock+0x4c/0x7c
>> drmm_mode_config_init+0x4a8/0xc50
>> msm_drm_init+0x274/0xac0
>> msm_drm_bind+0x20/0x2c
>> try_to_bring_up_master+0x3dc/0x470
>> __component_add+0x18c/0x3c0
>> component_add+0x1c/0x28
>> dp_display_probe+0x954/0xa98
>> platform_probe+0x124/0x15c
>> really_probe+0x1b0/0x5f8
>> __driver_probe_device+0x174/0x20c
>> driver_probe_device+0x70/0x134
>> __device_attach_driver+0x130/0x1d0
>> bus_for_each_drv+0xfc/0x14c
>> __device_attach+0x1bc/0x2bc
>> device_initial_probe+0x1c/0x28
>> bus_probe_device+0x94/0x178
>> deferred_probe_work_func+0x1a4/0x1f0
>> process_one_work+0x5d4/0x9dc
>> worker_thread+0x898/0xccc
>> kthread+0x2d4/0x3d4
>> ret_from_fork+0x10/0x20
>>
>> -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
>> ww_acquire_init+0x1c4/0x2c8
>> drm_modeset_acquire_init+0x44/0xc8
>> drm_helper_probe_single_connector_modes+0xb0/0x12dc
>> drm_mode_getconnector+0x5dc/0xfe8
>> drm_ioctl_kernel+0x1e4/0x338
>> drm_ioctl+0x3a4/0x684
>> __arm64_sys_ioctl+0x118/0x154
>> invoke_syscall+0x78/0x224
>> el0_svc_common+0x178/0x200
>> do_el0_svc+0x94/0x13c
>> el0_svc+0x5c/0xec
>> el0t_64_sync_handler+0x78/0x108
>> el0t_64_sync+0x1a4/0x1a8
>>
>> -> #0 (&dev->mode_config.mutex){+.+.}-{3:3}:
>> __lock_acquire+0x2650/0x672c
>> lock_acquire+0x1b4/0x4ac
>> __mutex_lock_common+0x174/0x1a64
>> mutex_lock_nested+0x98/0xac
>> dp_panel_add_fail_safe_mode+0x4c/0xa0
>> dp_hpd_plug_handle+0x1f0/0x280
>> dp_bridge_enable+0x94/0x2b8
>> drm_atomic_bridge_chain_enable+0x11c/0x168
>> drm_atomic_helper_commit_modeset_enables+0x500/0x740
>> msm_atomic_commit_tail+0x3e4/0x748
>> commit_tail+0x19c/0x278
>> drm_atomic_helper_commit+0x1dc/0x1f0
>> drm_atomic_commit+0xc0/0xd8
>> drm_atomic_helper_set_config+0xb4/0x134
>> drm_mode_setcrtc+0x688/0x1248
>> drm_ioctl_kernel+0x1e4/0x338
>> drm_ioctl+0x3a4/0x684
>> __arm64_sys_ioctl+0x118/0x154
>> invoke_syscall+0x78/0x224
>> el0_svc_common+0x178/0x200
>> do_el0_svc+0x94/0x13c
>> el0_svc+0x5c/0xec
>> el0t_64_sync_handler+0x78/0x108
>> el0t_64_sync+0x1a4/0x1a8
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
>> ---
>> drivers/gpu/drm/msm/dp/dp_display.c | 6 ------
>> drivers/gpu/drm/msm/dp/dp_panel.c | 23 +++++++++++++----------
>> 2 files changed, 13 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 92cd50f..01453db 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -555,12 +555,6 @@ static int dp_hpd_plug_handle(struct
>> dp_display_private *dp, u32 data)
>> mutex_unlock(&dp->event_mutex);
>> - /*
>> - * add fail safe mode outside event_mutex scope
>> - * to avoid potiential circular lock with drm thread
>> - */
>> - dp_panel_add_fail_safe_mode(dp->dp_display.connector);
>> -
>> /* uevent will complete connection part */
>> return 0;
>> };
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>> index 1aa9aa8c..23fee42 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>> @@ -151,15 +151,6 @@ static int dp_panel_update_modes(struct
>> drm_connector *connector,
>> return rc;
>> }
>> -void dp_panel_add_fail_safe_mode(struct drm_connector *connector)
>> -{
>> - /* fail safe edid */
>> - mutex_lock(&connector->dev->mode_config.mutex);
>> - if (drm_add_modes_noedid(connector, 640, 480))
>> - drm_set_preferred_mode(connector, 640, 480);
>> - mutex_unlock(&connector->dev->mode_config.mutex);
>> -}
>> -
>> int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
>> struct drm_connector *connector)
>> {
>> @@ -216,7 +207,11 @@ int dp_panel_read_sink_caps(struct dp_panel
>> *dp_panel,
>> goto end;
>> }
>> - dp_panel_add_fail_safe_mode(connector);
>> + /* fail safe edid */
>> + mutex_lock(&connector->dev->mode_config.mutex);
>> + if (drm_add_modes_noedid(connector, 640, 480))
>> + drm_set_preferred_mode(connector, 640, 480);
>> + mutex_unlock(&connector->dev->mode_config.mutex);
>> }
>> if (panel->aux_cfg_update_done) {
>> @@ -266,6 +261,14 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
>> return -EINVAL;
>> }
>> + /*
>> + * add fail safe mode (640x480) here
>> + * since we are executed in drm_thread context,
>> + * no mode_config.mutex acquired required
>> + */
>> + if (drm_add_modes_noedid(connector, 640, 480))
>> + drm_set_preferred_mode(connector, 640, 480);
>> +
>> if (dp_panel->edid)
>> return dp_panel_update_modes(connector, dp_panel->edid);
> Also, wouldn't calling get_modes() several times make cause adding more
> and more 640x480 modes to the modes list?
>
Shouldnt DRM be blocking that here? Call should trickle down here only
if count_modes was 0
if (out_resp->count_modes == 0) {
if (is_current_master)
connector->funcs->fill_modes(connector,
dev->mode_config.max_width,
dev->mode_config.max_height);
else
drm_dbg_kms(dev, "User-space requested a forced probe on
[CONNECTOR:%d:%s] but is not the DRM master, demoting to read-only probe",
connector->base.id, connector->name);
}
>
Powered by blists - more mailing lists