linux-kernel - Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53b2df23-d5ea-498b-a501-b64f753c0074@linux.dev>
Date: Thu, 21 Mar 2024 11:52:23 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: Michal Simek <michal.simek@....com>, David Airlie <airlied@...il.com>,
 linux-kernel@...r.kernel.org, Daniel Vetter <daniel@...ll.ch>,
 linux-arm-kernel@...ts.infradead.org,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

On 3/20/24 02:53, Tomi Valkeinen wrote:
> On 20/03/2024 00:51, Sean Anderson wrote:
>> Retraining the link can take a while, and might involve waiting for
>> DPCD reads/writes to complete. This is inappropriate for an IRQ handler.
>> Just schedule this work for later completion. This is racy, but will be
>> fixed in the next commit.
> 
> You should add the locks first, and use them here, rather than first
> adding a buggy commit and fixing it in the next one.

I didn't think I could add the locks first since I only noticed the IRQ
was threaded right before sending out this series. So yeah, we could add
locking, add the workqueue, and then unthread the IRQ.

>> Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
>> ---
>> Actually, on second look this IRQ is threaded. So why do we have a
>> workqueue for HPD events? Maybe we should make it unthreaded?
> 
> Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded.
> 
> We could move the queued work to be inside the threaded irq handler,
> but with a quick look, the HPD work has lines like "msleep(100)" (and
> that's inside a for loop...), which is probably not a good thing to do
> even in threaded irq handler.
> 
> Although I'm not sure if that code is good to have anywhere. Why do we
> even have such code in the HPD work path... We already got the HPD
> interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get
> the HPD signal with some monitors" even mean...

The documentation for this bit is

| HPD_STATE	0	ro	0x0	Contains the raw state of the HPD pin on the DisplayPort connector.

So I think the idea is to perform some debouncing.

> Would it be possible to clean up the work funcs a bit (I haven't
> looked a the new work func yet), to remove the worst extra sleeps, and
> just do all that inside the threaded irq handler?

Probably not, since a HPD IRQ results in link retraining, which can take a while.

> Do we need to handle interrupts while either delayed work is being done?

Probably not.

> If we do need a delayed work, would just one work be enough which
> handles both HPD_EVENT and HPD_IRQ, instead of two?

Maybe, but then we need to determine which pending events we need to
handle. I think since we have only two events it will be easier to just
have separate workqueues.

--Sean