Re: [PATCH v3 05/10] s390/pci: Restore IRQ unconditionally for the zPCI device

Farhan Ali
2025-09-15T17:42:22Z

On 9/15/2025 1:39 AM, Niklas Schnelle wrote:
> On Thu, 2025-09-11 at 11:33 -0700, Farhan Ali wrote:
>> Commit c1e18c17bda6 ("s390/pci: add zpci_set_irq()/zpci_clear_irq()"),
>> introduced the zpci_set_irq() and zpci_clear_irq(), to be used while
>> resetting a zPCI device.
>>
>> Commit da995d538d3a ("s390/pci: implement reset_slot for hotplug slot"),
>> mentions zpci_clear_irq() being called in the path for zpci_hot_reset_device().
>> But that is not the case anymore and these functions are not called
>> outside of this file.
> If you're doing another version I think you could add a bit more
> information on why this still works for existing recovery based on my
> investigation in
> https://lore.kernel.org/lkml/052ebdbb6f2d38025ca4345ee51e4857e19bb0e4.camel@linux.ibm.com/
>
> Even if you don't add more explanations, I'd tend to just drop the
> above paragraph as it doesn't seem relevant and sounds like
> zpci_hot_reset_device() doesn't clear IRQs. As explained in the linked
> mail there really is no need to call zpci_clear_irq() in
> zpci_hot_reset_device() as the CLP disable does disable IRQs. It's
> really only the state tracking that can get screwed up but is also fine
> for drivers which end up doing the tear down.

I referenced commit da995d538d3a as that commit introduced the
arch_restore_msi_irqs and describes the reasoning as to why we need it.
It also mentions about zpci_clear_irq being called by
zpci_hot_reset_device. IMHO the message was confusing as it took me my
down the path of trying to identify any commit that changed the behavior
since da995d538d3a. But that wasn't the case and it was an error in the
commit message. I want to keep a reference here to at least clarify that.

I had tried to clarify that this only becomes an issue if a driver tries
restoring state through pci_restore_state(), in the paragraph below. But
should I change it to be more explicit about that it's not an issue for
driver doing setup and tear down through arch_msi_irq_setup and
arch_msi_irq_teardown functions?

>
>> However after a CLP disable/enable reset, the device's IRQ are
>> unregistered, but the flag zdev->irq_registered does not get cleared. It
>> creates an inconsistent state and so arch_restore_msi_irqs() doesn't
>> correctly restore the device's IRQ. This becomes a problem when a PCI
>> driver tries to restore the state of the device through
>> pci_restore_state(). Restore IRQ unconditionally for the device and remove
>> the irq_registered flag as its redundant.
> s/its/it's/

Thanks, will fix.

>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>> arch/s390/include/asm/pci.h | 1 -
>> arch/s390/pci/pci_irq.c | 9 +--------
>> 2 files changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>> index 41f900f693d9..aed19a1aa9d7 100644
>> --- a/arch/s390/include/asm/pci.h
>> +++ b/arch/s390/include/asm/pci.h
>> @@ -145,7 +145,6 @@ struct zpci_dev {
>> u8 has_resources : 1;
>> u8 is_physfn : 1;
>> u8 util_str_avail : 1;
>> - u8 irqs_registered : 1;
>> u8 tid_avail : 1;
>> u8 rtr_avail : 1; /* Relaxed translation allowed */
>> unsigned int devfn; /* DEVFN part of the RID*/
>> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
>> index 84482a921332..e73be96ce5fe 100644
>> --- a/arch/s390/pci/pci_irq.c
>> +++ b/arch/s390/pci/pci_irq.c
>> @@ -107,9 +107,6 @@ static int zpci_set_irq(struct zpci_dev *zdev)
>> else
>> rc = zpci_set_airq(zdev);
>>
>> - if (!rc)
>> - zdev->irqs_registered = 1;
>> -
>> return rc;
>> }
>>
>> @@ -123,9 +120,6 @@ static int zpci_clear_irq(struct zpci_dev *zdev)
>> else
>> rc = zpci_clear_airq(zdev);
>>
>> - if (!rc)
>> - zdev->irqs_registered = 0;
>> -
>> return rc;
>> }
>>
>> @@ -427,8 +421,7 @@ bool arch_restore_msi_irqs(struct pci_dev *pdev)
>> {
>> struct zpci_dev *zdev = to_zpci(pdev);
>>
>> - if (!zdev->irqs_registered)
>> - zpci_set_irq(zdev);
>> + zpci_set_irq(zdev);
>> return true;
>> }
>>
> Code looks good to me. With or without my suggestions for the commit
> message:
>
> Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>

Thanks for reviewing!

Thanks
Farhan