| Message ID | 20240325135705.3717293-3-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] drm/panthor: Fix IO-page mmap() for 32-bit userspace on 64-bit kernel | expand |
On Mon, Mar 25, 2024 at 02:57:05PM +0100, 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. I understand that drm_dev_unplug() messes up with the cleanup, but I'm a bit reluctant to see a function that completely ignores if the device has been unplugged or not. Like mentioned on the review of 2/3, can we move the masking of the interrupts outside the critical section and not add drm_dev_unplug() ? > > v2: > - Add Steve's R-b > > Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block") > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > Reviewed-by: Steven Price <steven.price@arm.com> > --- > 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 3a930a368ae1..5634e9490c7f 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); \ > +} \ > + \ > 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. */ While reviewing this I've spotted that the comment needs updating: "... handler *can't* be called ..." Best regards, Liviu > 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++) { > -- > 2.44.0 >
On Mon, 25 Mar 2024 17:24:17 +0000 Liviu Dudau <liviu.dudau@arm.com> wrote: > On Mon, Mar 25, 2024 at 02:57:05PM +0100, 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. > > I understand that drm_dev_unplug() messes up with the cleanup, but I'm a bit > reluctant to see a function that completely ignores if the device has been > unplugged or not. Like mentioned on the review of 2/3, can we move the masking > of the interrupts outside the critical section and not add drm_dev_unplug() ? Nope, because the whole point of this drm_dev_enter/exit() section was to prevent accesses to registers when the associated iomem range has been returned to the system after the device has been removed. If you move this gpu_write() outside of the drm_dev_enter/exit() you're better off dropping this check entirely...
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h index 3a930a368ae1..5634e9490c7f 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); \ +} \ + \ 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++) {
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. v2: - Add Steve's R-b Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block") Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> Reviewed-by: Steven Price <steven.price@arm.com> --- 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(-)