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?
> +
> + if ((irq_status & BUTTRESS_EVENT) && ret == IRQ_NONE)
> + ret = IRQ_HANDLED;
> +
> + if (irq_status & BUTTRESS_ISR_IPC_FROM_CSE_IS_WAITING) {
> + dev_dbg(&isp->pdev->dev,
> + "BUTTRESS_ISR_IPC_FROM_CSE_IS_WAITING\n");
> + ipu6_buttress_ipc_recv(isp, &b->cse, &b->cse.recv_data);
> + complete(&b->cse.recv_complete);
> + }
> +
> + if (irq_status & BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING) {
> + dev_dbg(&isp->pdev->dev,
> + "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
> + ipu6_buttress_ipc_recv(isp, &b->ish, &b->ish.recv_data);
> + complete(&b->ish.recv_complete);
> + }
> +
> + if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE) {
> + dev_dbg(&isp->pdev->dev,
> + "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
> + complete(&b->cse.send_complete);
> + }
> +
> + if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_ISH) {
> + dev_dbg(&isp->pdev->dev,
> + "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
> + complete(&b->ish.send_complete);
> + }
> +
> + if (irq_status & BUTTRESS_ISR_SAI_VIOLATION &&
> + ipu6_buttress_get_secure_mode(isp))
> + dev_err(&isp->pdev->dev,
> + "BUTTRESS_ISR_SAI_VIOLATION\n");
> +
> + if (irq_status & (BUTTRESS_ISR_IS_FATAL_MEM_ERR |
> + BUTTRESS_ISR_PS_FATAL_MEM_ERR))
> + dev_err(&isp->pdev->dev,
> + "BUTTRESS_ISR_FATAL_MEM_ERR\n");
> +
> + if (irq_status & BUTTRESS_ISR_UFI_ERROR)
> + dev_err(&isp->pdev->dev, "BUTTRESS_ISR_UFI_ERROR\n");
> +
> + if (++count == BUTTRESS_MAX_CONSECUTIVE_IRQS) {
> + dev_err(&isp->pdev->dev, "too many consecutive IRQs\n");
> + ret = IRQ_NONE;
> + break;
> + }
> +
> + irq_status = readl(isp->base + reg_irq_sts);
> + } while (irq_status);
> +
> + if (disable_irqs)
> + writel(BUTTRESS_IRQS & ~disable_irqs,
> + isp->base + BUTTRESS_REG_ISR_ENABLE);
> +
> + pm_runtime_put(&isp->pdev->dev);
> +
> + return ret;
> +}
...
/Andreas
