| Message ID | 20240809193600.3360015-5-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: zynqmp_dp: IRQ cleanups and debugfs support | expand |
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
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);
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(-)