Re: [PATCH v6 4/8] drm: zynqmp_dp: Convert to a hard IRQ

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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@linux.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@ideasonboard.com>

  Tomi