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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f3a91866-3897-4872-8336-384bb8e568a4@linux.ibm.com>
Date: Mon, 15 Sep 2025 10:42:12 -0700
From: Farhan Ali <alifm@...ux.ibm.com>
To: Niklas Schnelle <schnelle@...ux.ibm.com>, linux-s390@...r.kernel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org
Cc: alex.williamson@...hat.com, helgaas@...nel.org, mjrosato@...ux.ibm.com
Subject: Re: [PATCH v3 05/10] s390/pci: Restore IRQ unconditionally for the
 zPCI device


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@...ux.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@...ux.ibm.com>

Thanks for reviewing!

Thanks
Farhan