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.
Have you check whether devm_free_irq() in ipu6_pci_remove() can help you?
>
>> +
>> + 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
>
--
Best regards,
Bingbu Cao
