Re: [PATCH v2 1/4] media: staging/ipu7: ignore interrupts when device is suspended

[Bingbu Cao <bingbu.cao@linux.intel.com>]

Sakari,

Thanks for the review.

On 12/22/25 4:43 PM, Sakari Ailus wrote:
> Hi Bingbu,
> 
> On Mon, Dec 22, 2025 at 03:06:26PM +0800, bingbu.cao@intel.com wrote:
>> From: Bingbu Cao <bingbu.cao@intel.com>
>>
>> IPU7 devices have shared interrupts with others. In some case when
>> IPU7 device is suspended, driver get unexpected interrupt and invalid
>> irq status 0xffffffff from ISR_STATUS and PB LOCAL_STATUS
>> registers as interrupt is triggered from other device on shared
>> irq line.
>>
>> In order to avoid this issue use pm_runtime_get_if_active() to check
>> if IPU7 device is resumed, ignore the invalid irq status and use
>> synchronize_irq() in suspend.
>>
>> Cc: Stable@vger.kernel.org
>> Fixes: b7fe4c0019b1 ("media: staging/ipu7: add Intel IPU7 PCI device driver")
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> ---
>>  drivers/staging/media/ipu7/ipu7-buttress.c | 12 ++++++++++--
>>  drivers/staging/media/ipu7/ipu7.c          |  4 ++++
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/ipu7/ipu7-buttress.c b/drivers/staging/media/ipu7/ipu7-buttress.c
>> index e5707f5e300b..e4328cafe91d 100644
>> --- a/drivers/staging/media/ipu7/ipu7-buttress.c
>> +++ b/drivers/staging/media/ipu7/ipu7-buttress.c
>> @@ -342,14 +342,22 @@ irqreturn_t ipu_buttress_isr(int irq, void *isp_ptr)
>>  	u32 disable_irqs = 0;
>>  	u32 irq_status;
>>  	unsigned int i;
>> +	int active;
>>  
>> -	pm_runtime_get_noresume(dev);
>> +	active = pm_runtime_get_if_active(dev);
>> +	if (active <= 0)
>> +		return IRQ_NONE;
>>  
>>  	pb_irq = readl(isp->pb_base + INTERRUPT_STATUS);
>>  	writel(pb_irq, isp->pb_base + INTERRUPT_STATUS);
>>  
>>  	/* check btrs ATS, CFI and IMR errors, BIT(0) is unused for IPU */
>>  	pb_local_irq = readl(isp->pb_base + BTRS_LOCAL_INTERRUPT_MASK);
>> +	if (WARN_ON_ONCE(pb_local_irq == 0xffffffff)) {
>> +		pm_runtime_put_noidle(dev);
>> +		return IRQ_NONE;
>> +	}
>> +
>>  	if (pb_local_irq & ~BIT(0)) {
>>  		dev_warn(dev, "PB interrupt status 0x%x local 0x%x\n", pb_irq,
>>  			 pb_local_irq);
>> @@ -365,7 +373,7 @@ irqreturn_t ipu_buttress_isr(int irq, void *isp_ptr)
>>  	}
>>  
>>  	irq_status = readl(isp->base + BUTTRESS_REG_IRQ_STATUS);
>> -	if (!irq_status) {
>> +	if (!irq_status || WARN_ON_ONCE(irq_status == 0xffffffff)) {
>>  		pm_runtime_put_noidle(dev);
>>  		return IRQ_NONE;
> 
> Doesn't this apply to the ipu6 driver as well? E.g. on my Alder lake system
> the interrupt is shared with i801_smbus and processor_thermal_device_pci.
> It may be no interrupts are generated by those devices at inconvenient
> times for ipu6, but the ipu driver can't assume that.

I remember that Stanislaw made a fix for IPU6.
> 
> Is the WARN_ON_ONCE() necessary? I'd use dev_warn_once() here if you'd like
> to write a message to the log.
>
I will update that, dev_warn_once() makes more sense.

>>  	}
>> diff --git a/drivers/staging/media/ipu7/ipu7.c b/drivers/staging/media/ipu7/ipu7.c
>> index 5cddc09c72bf..6c8c3eea44ac 100644
>> --- a/drivers/staging/media/ipu7/ipu7.c
>> +++ b/drivers/staging/media/ipu7/ipu7.c
>> @@ -2684,6 +2684,10 @@ static void ipu7_pci_reset_done(struct pci_dev *pdev)
>>   */
>>  static int ipu7_suspend(struct device *dev)
>>  {
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	synchronize_irq(pdev->irq);
>> +
>>  	return 0;
>>  }
>>  
> 

-- 
Best regards,
Bingbu Cao