linux-kernel - Re: [PATCH v6 4/8] drm: zynqmp

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: <1846295a-aea7-41fe-a7a6-756a260ef3a3@ideasonboard.com>
Date: Wed, 2 Oct 2024 16:52:55 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Sean Anderson <sean.anderson@...ux.dev>,
 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
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 David Airlie <airlied@...il.com>, Michal Simek <michal.simek@....com>,
 Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH v6 4/8] drm: zynqmp_dp: Convert to a hard IRQ

Hi,

On 09/08/2024 22:35, Sean Anderson wrote:
> Now that all of the sleeping work is done outside of the IRQ, we can
> convert it to a hard IRQ. Shared IRQs may be triggered even after
> calling disable_irq, so use free_irq instead which removes our callback
> altogether.
> 
> Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
> ---
> 
> Changes in v6:
> - Fix hang upon driver removal
> 
> Changes in v3:
> - New
> 
>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index cec5711c7026..532e103713b3 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1831,9 +1831,8 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>   	 * Now that the hardware is initialized and won't generate spurious
>   	 * interrupts, request the IRQ.
>   	 */
> -	ret = devm_request_threaded_irq(dp->dev, dp->irq, NULL,
> -					zynqmp_dp_irq_handler, IRQF_ONESHOT,
> -					dev_name(dp->dev), dp);
> +	ret = devm_request_irq(dp->dev, dp->irq, zynqmp_dp_irq_handler,
> +			       IRQF_SHARED, dev_name(dp->dev), dp);
>   	if (ret < 0)
>   		goto err_phy_exit;
>   
> @@ -1858,7 +1857,7 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>   	struct zynqmp_dp *dp = dpsub->dp;
>   
>   	zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_ALL);
> -	disable_irq(dp->irq);
> +	devm_free_irq(dp->dev, dp->irq, dp);
>   
>   	cancel_work_sync(&dp->hpd_irq_work);
>   	cancel_work_sync(&dp->hpd_work);

Not much point using devm for request irq, I think, as it's manually freed.

Another thing, which I think is not an issue at the moment nor related 
to this series as such, is that we use IRQF_SHARED and 
zynqmp_dp_irq_handler() will read a DP register right away. This means 
that if the DP is runtime suspended and we get an interrupt, the driver 
accesses a register on a suspended IP, which often leads to crash/hang.

Here I think we enable all necessary clocks at probe time, so the DP is 
always enabled and the above is not an issue.

In any case, even with the current devm:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>

  Tomi