Bingbu,
On Wed, 2024-01-03 at 21:18 +0800, Bingbu Cao wrote:
> Andreas,
>
> On 1/3/24 9:11 PM, Bingbu Cao wrote:
> > Andreas,
> >
> > On 1/3/24 5:22 PM, Andreas Helbech Kleist wrote:
> > > Hi Bingbu,
> > >
> > > On Tue, 2023-10-24 at 19:29 +0800, bingbu.cao@xxxxxxxxx wrote:
> > > > From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> > > >
> > > > The IPU6 buttress is the interface between IPU device (input system
> > > > and processing system) with rest of the SoC. It contains overall IPU
> > > > hardware control registers, these control registers are used as the
> > > > interfaces with the Intel Converged Security Engine and Punit to do
> > > > firmware authentication and power management.
> > > >
> > > > Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> > > > ---
> > >
> > > ...
> > >
> > > > +static irqreturn_t ipu6_buttress_call_isr(struct ipu6_bus_device *adev)
> > > > +{
> > > > + irqreturn_t ret = IRQ_WAKE_THREAD;
> > > > +
> > > > + if (!adev || !adev->auxdrv || !adev->auxdrv_data)
> > > > + return IRQ_NONE;
> > > > +
> > > > + if (adev->auxdrv_data->isr)
> > > > + ret = adev->auxdrv_data->isr(adev);
> > > > +
> > > > + if (ret == IRQ_WAKE_THREAD && !adev->auxdrv_data->isr_threaded)
> > > > + ret = IRQ_NONE;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
> > > > +{
> > > > + struct ipu6_device *isp = isp_ptr;
> > > > + struct ipu6_bus_device *adev[] = { isp->isys, isp->psys };
> > > > + struct ipu6_buttress *b = &isp->buttress;
> > > > + u32 reg_irq_sts = BUTTRESS_REG_ISR_STATUS;
> > > > + irqreturn_t ret = IRQ_NONE;
> > > > + u32 disable_irqs = 0;
> > > > + u32 irq_status;
> > > > + u32 i, count = 0;
> > > > +
> > > > + pm_runtime_get_noresume(&isp->pdev->dev);
> > > > +
> > > > + irq_status = readl(isp->base + reg_irq_sts);
> > > > + if (!irq_status) {
> > > > + pm_runtime_put_noidle(&isp->pdev->dev);
> > > > + return IRQ_NONE;
> > > > + }
> > > > +
> > > > + do {
> > > > + writel(irq_status, isp->base + BUTTRESS_REG_ISR_CLEAR);
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(ipu6_adev_irq_mask); i++) {
> > > > + irqreturn_t r = ipu6_buttress_call_isr(adev[i]);
> > > > +
> > > > + if (!(irq_status & ipu6_adev_irq_mask[i]))
> > > > + continue;
> > > > +
> > > > + if (r == IRQ_WAKE_THREAD) {
> > > > + ret = IRQ_WAKE_THREAD;
> > > > + disable_irqs |= ipu6_adev_irq_mask[i];
> > > > + } else if (ret == IRQ_NONE && r == IRQ_HANDLED) {
> > > > + ret = IRQ_HANDLED;
> > > > + }
> > > > + }
> > >
> > > It seems wrong to call the ISR for a adev[i] before checking the
> > > corresponding IRQ mask. If the mask is not set, the ISR is still
> > > called, but the result is thrown away.
> > >
> > > I started investigating this because I'm seeing "general protection
> > > fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b" in this
> > > function when unbinding the IPU4 driver.
> > >
> > > How do you ensure that the ISR is not called on a ipu6-bus device that
> > > has been deleted? Specifically in ipu6_pci_remove, ipu6_bus_del_devices
> > > is called before ipu6_buttress_exit (which disables buttress IRQs).
> > > Perhaps the above for loop should really be a "for each ipu6-bus
> > > device" loop?
> >
> > You are right, the buttress_exit() should be called before
> > ipu6_bus_del_devices().
> >
> > Even with this, driver cannot guarantee that hardware irq was actually
> > disabled on IPU4. In some error cases, HW could report unmasked errors on
> > IPU4 (no such case on IPU6), if I remember correctly.
Thanks for the info, that's good to know.
> > Have you check whether devm_free_irq() in ipu6_pci_remove() can help you?
It looks like it might help.
> BTW, could you share the irq_status in your case?
Sorry, I don't have a log of it. And it seems to be a bit heisenbug-
like - it doesn't always happen when I add logging :/
/Andreas
