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]
Date: Wed, 20 Mar 2024 08:53:56 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Sean Anderson <sean.anderson@...ux.dev>
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 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.

> 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...

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?

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

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

  Tomi


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ