| 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: <83129bad-44a9-bec7-f931-8067ef1b9d4d@quicinc.com>
Date: Fri, 22 Apr 2022 09:04:57 -0700
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Doug Anderson <dianders@...omium.org>,
Sankeerth Billakanti <quic_sbillaka@...cinc.com>
CC: quic_kalyant <quic_kalyant@...cinc.com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
quic_vproddut <quic_vproddut@...cinc.com>,
David Airlie <airlied@...ux.ie>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
dri-devel <dri-devel@...ts.freedesktop.org>,
Stephen Boyd <swboyd@...omium.org>,
Sean Paul <sean@...rly.run>, Sean Paul <seanpaul@...omium.org>,
Steev Klimaszewski <steev@...i.org>,
"Dmitry Baryshkov" <dmitry.baryshkov@...aro.org>,
"Aravind Venkateswaran (QUIC)" <quic_aravindh@...cinc.com>,
"Kuogee Hsieh (QUIC)" <quic_khsieh@...cinc.com>,
freedreno <freedreno@...ts.freedesktop.org>
Subject: Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG
interrupts for eDP
Hi Doug
For the lockdep error, the splat looks similar to what kuogee fixed
recently.
Can you please check if below patch is present in your tree?
https://patchwork.freedesktop.org/patch/481396/
Thanks
Abhinav
On 4/22/2022 8:55 AM, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 22, 2022 at 2:11 AM Sankeerth Billakanti
> <quic_sbillaka@...cinc.com> wrote:
>>
>> The panel-edp enables the eDP panel power during probe, get_modes
>> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
>> controller are directly dependent on panel power. As eDP display can be
>> assumed as always connected, the controller driver can skip the eDP
>> connect and disconnect interrupts. Any disruption in the link status
>> will be indicated via the IRQ_HPD interrupts.
>>
>> So, the eDP controller driver can just enable the IRQ_HPD and replug
>> interrupts. The DP controller driver still needs to enable all the
>> interrupts.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@...cinc.com>
>> ---
>> Changes in v9:
>> - add comment explaining the interrupt status register
>>
>> Changes in v8:
>> - add comment explaining the interrupt status return
>>
>> Changes in v7:
>> - reordered the patch in the series
>> - modified the return statement for isr
>> - connector check modified to just check for eDP
>>
>> drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++++++++++------
>> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index fac815f..df9670d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>>
>> u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>>
>> - /* enable HPD plug and unplug interrupts */
>> - dp_catalog_hpd_config_intr(dp_catalog,
>> - DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
>> -
>> /* Configure REFTIMER and enable it */
>> reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
>> dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
>> @@ -599,13 +595,21 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
>> {
>> struct dp_catalog_private *catalog = container_of(dp_catalog,
>> struct dp_catalog_private, dp_catalog);
>> - int isr = 0;
>> + int isr, mask;
>>
>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
>> (isr & DP_DP_HPD_INT_MASK));
>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>>
>> - return isr;
>> + /*
>> + * We only want to return interrupts that are unmasked to the caller.
>> + * However, the interrupt status field also contains other
>> + * informational bits about the HPD state status, so we only mask
>> + * out the part of the register that tells us about which interrupts
>> + * are pending.
>> + */
>> + return isr & (mask | ~DP_DP_HPD_INT_MASK);
>> }
>>
>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 055681a..dea4de9 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>> dp_display_handle_plugged_change(&dp->dp_display, false);
>>
>> /* enable HDP plug interrupt to prepare for next plugin */
>> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
>> + if (!dp->dp_display.is_edp)
>> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
>>
>> DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
>> dp->dp_display.connector_type, state);
>> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
>> dp_display_host_init(dp);
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>> + /* Enable plug and unplug interrupts only for external DisplayPort */
>> + if (!dp->dp_display.is_edp)
>> + dp_catalog_hpd_config_intr(dp->catalog,
>> + DP_DP_HPD_PLUG_INT_MASK |
>> + DP_DP_HPD_UNPLUG_INT_MASK,
>> + true);
>> +
>> /* Enable interrupt first time
>> * we are leaving dp clocks on during disconnect
>> * and never disable interrupt
>> @@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev)
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>>
>> + if (!dp->dp_display.is_edp)
>> + dp_catalog_hpd_config_intr(dp->catalog,
>> + DP_DP_HPD_PLUG_INT_MASK |
>> + DP_DP_HPD_UNPLUG_INT_MASK,
>> + true);
>> +
>> if (dp_catalog_link_is_connected(dp->catalog)) {
>> /*
>> * set sink to normal operation mode -- D0
>> @@ -1659,6 +1673,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>> return;
>> }
>>
>> + if (dp->is_edp)
>> + dp_hpd_plug_handle(dp_display, 0);
>
> So I finally got a chance to test and unfortunately this is getting a
> lockdep error. :( Here's the crawl with my current set of patches
> (which, admittedly is on the chromeos-5.15 tree) instead of pure
> upstream. I avoid the errors with this (sorry for the whitespace
> damage, but it's really just a one-line change):
>
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -582,7 +582,8 @@ static int dp_hpd_plug_handle(struct
> dp_display_private *dp, u32 data)
> * 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);
> + if (!dp->dp_display.is_edp)
> + dp_panel_add_fail_safe_mode(dp->dp_display.connector);
>
> /* uevent will complete connection part */
> return 0;
>
> That's a bit gross, but at least shows the problem. It's not a
> _terrible_ fix because the failsafe modes don't make sense for eDP,
> right? That being said, why are we hacking this in here? Shouldn't
> this be in the core? ...or at least we should just be providing them
> in get_modes()?
>
> FWIW: the error was:
>
> ======================================================
> 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
>
> other info that might help us debug this:
>
> Chain exists of:
> &dev->mode_config.mutex --> crtc_ww_class_mutex --> &kms->commit_lock[i]
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&kms->commit_lock[i]);
> lock(crtc_ww_class_mutex);
> lock(&kms->commit_lock[i]);
> lock(&dev->mode_config.mutex);
>
> *** DEADLOCK ***
>
> 3 locks held by frecon/429:
> #0: ffffffc00e197ab0 (crtc_ww_class_acquire){+.+.}-{0:0}, at:
> drm_modeset_acquire_init+0x44/0xc8
> #1: ffffff808dc3c588 (crtc_ww_class_mutex){+.+.}-{3:3}, at:
> modeset_lock+0x18c/0x4ac
> #2: ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at:
> lock_crtcs+0xb4/0x124
>
> stack backtrace:
> CPU: 5 PID: 429 Comm: frecon Tainted: G W
> 5.15.35-lockdep #6 9ba2ecd8f15354021fe165873da3aaa99f5b6798
> Hardware name: Google Herobrine (rev1+) (DT)
> Call trace:
> dump_backtrace+0x0/0x3c4
> show_stack+0x20/0x2c
> dump_stack_lvl+0x78/0x9c
> dump_stack+0x18/0x44
> print_circular_bug+0x17c/0x1a8
> check_noncircular+0x260/0x30c
> __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
Powered by blists - more mailing lists