On 25/03/2024 10:41, Boris Brezillon wrote:
> panthor_xxx_irq_suspend() doesn't mask the interrupts if drm_dev_unplug()
> has been called, which is always the case when our panthor_xxx_unplug()
> helpers are called. Fix that by introducing a panthor_xxx_unplug() helper
> that does what panthor_xxx_irq_suspend() except it does it
> unconditionally.
>
> Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> ---
> Found inadvertently while debugging another issue. I guess I managed to
> call rmmod during a PING and that led to the FW interrupt handler
> being executed after the device suspend happened.
> ---
> drivers/gpu/drm/panthor/panthor_device.h | 8 ++++++++
> drivers/gpu/drm/panthor/panthor_fw.c | 2 +-
> drivers/gpu/drm/panthor/panthor_gpu.c | 2 +-
> drivers/gpu/drm/panthor/panthor_mmu.c | 2 +-
> 4 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 51c9d61b6796..ba43d5ea4e96 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -321,6 +321,14 @@ static irqreturn_t panthor_ ## __name ## _irq_threaded_handler(int irq, void *da
> return ret; \
> } \
> \
> +static inline void panthor_ ## __name ## _irq_unplug(struct panthor_irq *pirq) \
> +{ \
> + pirq->mask = 0; \
> + gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0); \
> + synchronize_irq(pirq->irq); \
> + atomic_set(&pirq->suspended, true); \
> +} \
> + \
This does things in a different order to _irq_suspend, is there a good
reason?
I'd expect:
{
atomic_set(&pirq->suspended, true);
gpu_write(pirq->ptdev, __reg_prefix ## _INT_MASK, 0);
synchronize_irq(pirq->irq);
pirq->mask = 0;
}
In particular I'm wondering if having the atomic_set after
synchronize_irq() could cause problems with an irq handler changing the
INT_MASK again (although AFAICT it should end up setting it to 0).
Otherwise this change looks good.
Thanks,
Steve
> static inline void panthor_ ## __name ## _irq_suspend(struct panthor_irq *pirq) \
> { \
> int cookie; \
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 33c87a59834e..7a9710a38c5f 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -1128,7 +1128,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
>
> /* Make sure the IRQ handler can be called after that point. */
> if (ptdev->fw->irq.irq)
> - panthor_job_irq_suspend(&ptdev->fw->irq);
> + panthor_job_irq_unplug(&ptdev->fw->irq);
>
> panthor_fw_stop(ptdev);
>
> diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
> index 6dbbc4cfbe7e..b84c5b650fd9 100644
> --- a/drivers/gpu/drm/panthor/panthor_gpu.c
> +++ b/drivers/gpu/drm/panthor/panthor_gpu.c
> @@ -174,7 +174,7 @@ void panthor_gpu_unplug(struct panthor_device *ptdev)
> unsigned long flags;
>
> /* Make sure the IRQ handler is not running after that point. */
> - panthor_gpu_irq_suspend(&ptdev->gpu->irq);
> + panthor_gpu_irq_unplug(&ptdev->gpu->irq);
>
> /* Wake-up all waiters. */
> spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index fdd35249169f..1f333cdded0f 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -2622,7 +2622,7 @@ int panthor_vm_prepare_mapped_bos_resvs(struct drm_exec *exec, struct panthor_vm
> */
> void panthor_mmu_unplug(struct panthor_device *ptdev)
> {
> - panthor_mmu_irq_suspend(&ptdev->mmu->irq);
> + panthor_mmu_irq_unplug(&ptdev->mmu->irq);
>
> mutex_lock(&ptdev->mmu->as.slots_lock);
> for (u32 i = 0; i < ARRAY_SIZE(ptdev->mmu->as.slots); i++) {
