[RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Fam Zheng]

If the device doesn't support shutdown, disabling interrupts may cause
trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
after we disable MSI-X, futher notifications from device will be
delivered to IRQ, which is unexpected. This IRQ will not be cleared, and
may prevent us from making progress, by keep triggering interrupts.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 drivers/pci/pci-driver.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 3cb2210..fb29c96 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
 
 	pm_runtime_resume(dev);
 
-	if (drv && drv->shutdown)
+	if (drv && drv->shutdown) {
 		drv->shutdown(pci_dev);
-	pci_msi_shutdown(pci_dev);
-	pci_msix_shutdown(pci_dev);
+		pci_msi_shutdown(pci_dev);
+		pci_msix_shutdown(pci_dev);
+	}
 
 #ifdef CONFIG_KEXEC
 	/*
-- 
1.9.3

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Paolo Bonzini]

On 12/03/2015 06:21, Fam Zheng wrote:
> If the device doesn't support shutdown, disabling interrupts may cause
> trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
> after we disable MSI-X, futher notifications from device will be
> delivered to IRQ, which is unexpected. This IRQ will not be cleared, and
> may prevent us from making progress, by keep triggering interrupts.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  drivers/pci/pci-driver.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3cb2210..fb29c96 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
>  
>  	pm_runtime_resume(dev);
>  
> -	if (drv && drv->shutdown)
> +	if (drv && drv->shutdown) {
>  		drv->shutdown(pci_dev);
> -	pci_msi_shutdown(pci_dev);
> -	pci_msix_shutdown(pci_dev);
> +		pci_msi_shutdown(pci_dev);
> +		pci_msix_shutdown(pci_dev);
> +	}
>  
>  #ifdef CONFIG_KEXEC
>  	/*
> 

The patch may be okay, but I think the bug here is also that virtio-pci
is not defining a .shutdown callback.  It should define one and call
free_irq (for INTX) and pci_disable_msix.

How is this related to the virtio-scsi patch that you posted?  Do you
need both to fix the problem you reported?

Paolo

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Fam Zheng]

On Thu, 03/12 17:21, Paolo Bonzini wrote:
> On 12/03/2015 06:21, Fam Zheng wrote:
> > If the device doesn't support shutdown, disabling interrupts may cause
> > trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
> > after we disable MSI-X, futher notifications from device will be
> > delivered to IRQ, which is unexpected. This IRQ will not be cleared, and
> > may prevent us from making progress, by keep triggering interrupts.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  drivers/pci/pci-driver.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 3cb2210..fb29c96 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
> >  
> >  	pm_runtime_resume(dev);
> >  
> > -	if (drv && drv->shutdown)
> > +	if (drv && drv->shutdown) {
> >  		drv->shutdown(pci_dev);
> > -	pci_msi_shutdown(pci_dev);
> > -	pci_msix_shutdown(pci_dev);
> > +		pci_msi_shutdown(pci_dev);
> > +		pci_msix_shutdown(pci_dev);
> > +	}
> >  
> >  #ifdef CONFIG_KEXEC
> >  	/*
> > 
> 
> The patch may be okay, but I think the bug here is also that virtio-pci
> is not defining a .shutdown callback.  It should define one and call
> free_irq (for INTX) and pci_disable_msix.

It's not enough. The device has to know we disabled msix, otherwise it will
send us IRQ, which is wrong.

> 
> How is this related to the virtio-scsi patch that you posted?  Do you
> need both to fix the problem you reported?
> 

This one should be enough. I was wrong in saying virtio-scsi hanging the
shutdown is because requests not being completed, it's more because of the
unexpected IRQ as explained in [1].

[1]: https://bugzilla.redhat.com/attachment.cgi?id=998391

Fam

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Fam Zheng]

On Thu, 03/12 17:21, Paolo Bonzini wrote:
> On 12/03/2015 06:21, Fam Zheng wrote:
> > If the device doesn't support shutdown, disabling interrupts may cause
> > trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
> > after we disable MSI-X, futher notifications from device will be
> > delivered to IRQ, which is unexpected. This IRQ will not be cleared, and
> > may prevent us from making progress, by keep triggering interrupts.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  drivers/pci/pci-driver.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 3cb2210..fb29c96 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
> >  
> >  	pm_runtime_resume(dev);
> >  
> > -	if (drv && drv->shutdown)
> > +	if (drv && drv->shutdown) {
> >  		drv->shutdown(pci_dev);
> > -	pci_msi_shutdown(pci_dev);
> > -	pci_msix_shutdown(pci_dev);
> > +		pci_msi_shutdown(pci_dev);
> > +		pci_msix_shutdown(pci_dev);
> > +	}
> >  
> >  #ifdef CONFIG_KEXEC
> >  	/*
> > 
> 
> The patch may be okay, but I think the bug here is also that virtio-pci
> is not defining a .shutdown callback.  It should define one and call
> free_irq (for INTX) and pci_disable_msix.

It's not enough. The device has to know we disabled msix, otherwise it will
send us IRQ, which is wrong.

> 
> How is this related to the virtio-scsi patch that you posted?  Do you
> need both to fix the problem you reported?
> 

This one should be enough. I was wrong in saying virtio-scsi hanging the
shutdown is because requests not being completed, it's more because of the
unexpected IRQ as explained in [1].

[1]: https://bugzilla.redhat.com/attachment.cgi?id=998391

Fam

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Bandan Das]

Hi Fam,

Fam Zheng <famz@redhat.com> writes:

> If the device doesn't support shutdown, disabling interrupts may cause
> trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
> after we disable MSI-X, futher notifications from device will be
> delivered to IRQ, which is unexpected. This IRQ will not be cleared, and
> may prevent us from making progress, by keep triggering interrupts.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  drivers/pci/pci-driver.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3cb2210..fb29c96 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
>  
>  	pm_runtime_resume(dev);
>  
> -	if (drv && drv->shutdown)
> +	if (drv && drv->shutdown) {
>  		drv->shutdown(pci_dev);
> -	pci_msi_shutdown(pci_dev);
> -	pci_msix_shutdown(pci_dev);
> +		pci_msi_shutdown(pci_dev);
> +		pci_msix_shutdown(pci_dev);
> +	}

If the driver doesn't provide a shutdown method and doesn't itself
disable MSI/MSI-X, then pci_msi(x)_shutdown won't be called anymore,
no ?

This is probably ok (but unclean) for a system shutdown, but could
cause problems for something like kexec ?

Bandan

>  #ifdef CONFIG_KEXEC
>  	/*

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Fam Zheng]

On Thu, 03/12 19:56, Bandan Das wrote:
> Hi Fam,
> 
> Fam Zheng <famz@redhat.com> writes:
> 
> > If the device doesn't support shutdown, disabling interrupts may cause
> > trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
> > after we disable MSI-X, futher notifications from device will be
> > delivered to IRQ, which is unexpected. This IRQ will not be cleared, and
> > may prevent us from making progress, by keep triggering interrupts.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  drivers/pci/pci-driver.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 3cb2210..fb29c96 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
> >  
> >  	pm_runtime_resume(dev);
> >  
> > -	if (drv && drv->shutdown)
> > +	if (drv && drv->shutdown) {
> >  		drv->shutdown(pci_dev);
> > -	pci_msi_shutdown(pci_dev);
> > -	pci_msix_shutdown(pci_dev);
> > +		pci_msi_shutdown(pci_dev);
> > +		pci_msix_shutdown(pci_dev);
> > +	}
> 
> If the driver doesn't provide a shutdown method and doesn't itself
> disable MSI/MSI-X, then pci_msi(x)_shutdown won't be called anymore,
> no ?

Right.

> 
> This is probably ok (but unclean) for a system shutdown, but could
> cause problems for something like kexec ?

Doesn't the reset in kexec clean this up during initialization?

Without shutdown, anything in the driver is unclean anyway, for example
free_irq is not called.

Fam

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Bandan Das]

Ccing Alex, he can probably confirm if my understanding is indeed correct.

Fam Zheng <famz@redhat.com> writes:

> On Thu, 03/12 19:56, Bandan Das wrote:
>> Hi Fam,
>> 
>> Fam Zheng <famz@redhat.com> writes:
>> 
>> > If the device doesn't support shutdown, disabling interrupts may cause
>> > trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
>> > after we disable MSI-X, futher notifications from device will be
>> > delivered to IRQ, which is unexpected. This IRQ will not be cleared, and
>> > may prevent us from making progress, by keep triggering interrupts.
>> >
>> > Signed-off-by: Fam Zheng <famz@redhat.com>
>> > ---
>> >  drivers/pci/pci-driver.c | 7 ++++---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> > index 3cb2210..fb29c96 100644
>> > --- a/drivers/pci/pci-driver.c
>> > +++ b/drivers/pci/pci-driver.c
>> > @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
>> >  
>> >  	pm_runtime_resume(dev);
>> >  
>> > -	if (drv && drv->shutdown)
>> > +	if (drv && drv->shutdown) {
>> >  		drv->shutdown(pci_dev);
>> > -	pci_msi_shutdown(pci_dev);
>> > -	pci_msix_shutdown(pci_dev);
>> > +		pci_msi_shutdown(pci_dev);
>> > +		pci_msix_shutdown(pci_dev);
>> > +	}
>> 
>> If the driver doesn't provide a shutdown method and doesn't itself
>> disable MSI/MSI-X, then pci_msi(x)_shutdown won't be called anymore,
>> no ?
>
> Right.
>
>> 
>> This is probably ok (but unclean) for a system shutdown, but could
>> cause problems for something like kexec ?
>
> Doesn't the reset in kexec clean this up during initialization?

I guess it would take the same path as a reboot.

> Without shutdown, anything in the driver is unclean anyway, for example
> free_irq is not called.

True, And that is why the MSI/-X shutdown functions are called here
because pci can't always rely on the individual device drivers to do
the right thing, but atleast can make a best effort at cleaning up.

> Fam

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Fam Zheng]

On Thu, 03/12 23:09, Bandan Das wrote:
> Ccing Alex, he can probably confirm if my understanding is indeed correct.
> 
> Fam Zheng <famz@redhat.com> writes:
> 
> > On Thu, 03/12 19:56, Bandan Das wrote:
> >> Hi Fam,
> >> 
> >> Fam Zheng <famz@redhat.com> writes:
> >> 
> >> > If the device doesn't support shutdown, disabling interrupts may cause
> >> > trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
> >> > after we disable MSI-X, futher notifications from device will be
> >> > delivered to IRQ, which is unexpected. This IRQ will not be cleared, and
> >> > may prevent us from making progress, by keep triggering interrupts.
> >> >
> >> > Signed-off-by: Fam Zheng <famz@redhat.com>
> >> > ---
> >> >  drivers/pci/pci-driver.c | 7 ++++---
> >> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> >> > index 3cb2210..fb29c96 100644
> >> > --- a/drivers/pci/pci-driver.c
> >> > +++ b/drivers/pci/pci-driver.c
> >> > @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
> >> >  
> >> >  	pm_runtime_resume(dev);
> >> >  
> >> > -	if (drv && drv->shutdown)
> >> > +	if (drv && drv->shutdown) {
> >> >  		drv->shutdown(pci_dev);
> >> > -	pci_msi_shutdown(pci_dev);
> >> > -	pci_msix_shutdown(pci_dev);
> >> > +		pci_msi_shutdown(pci_dev);
> >> > +		pci_msix_shutdown(pci_dev);
> >> > +	}
> >> 
> >> If the driver doesn't provide a shutdown method and doesn't itself
> >> disable MSI/MSI-X, then pci_msi(x)_shutdown won't be called anymore,
> >> no ?
> >
> > Right.
> >
> >> 
> >> This is probably ok (but unclean) for a system shutdown, but could
> >> cause problems for something like kexec ?
> >
> > Doesn't the reset in kexec clean this up during initialization?
> 
> I guess it would take the same path as a reboot.

I don't understand, do you mean that no reset will be done before more
operations on the device?

> 
> > Without shutdown, anything in the driver is unclean anyway, for example
> > free_irq is not called.
> 
> True, And that is why the MSI/-X shutdown functions are called here
> because pci can't always rely on the individual device drivers to do
> the right thing, but atleast can make a best effort at cleaning up.

This virtio-scsi case shows us that intermediate state is bad, so I still think
we should call pci_msix_shutdown conditionally.

Fam

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Bandan Das]

Fam Zheng <famz@redhat.com> writes:

> On Thu, 03/12 23:09, Bandan Das wrote:
>> Ccing Alex, he can probably confirm if my understanding is indeed correct.
>> 
>> Fam Zheng <famz@redhat.com> writes:
>> 
>> > On Thu, 03/12 19:56, Bandan Das wrote:
>> >> Hi Fam,
>> >> 
>> >> Fam Zheng <famz@redhat.com> writes:
>> >> 
>> >> > If the device doesn't support shutdown, disabling interrupts may cause
>> >> > trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
>> >> > after we disable MSI-X, futher notifications from device will be
>> >> > delivered to IRQ, which is unexpected. This IRQ will not be cleared, and
>> >> > may prevent us from making progress, by keep triggering interrupts.
>> >> >
>> >> > Signed-off-by: Fam Zheng <famz@redhat.com>
>> >> > ---
>> >> >  drivers/pci/pci-driver.c | 7 ++++---
>> >> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >> >
>> >> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> >> > index 3cb2210..fb29c96 100644
>> >> > --- a/drivers/pci/pci-driver.c
>> >> > +++ b/drivers/pci/pci-driver.c
>> >> > @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
>> >> >  
>> >> >  	pm_runtime_resume(dev);
>> >> >  
>> >> > -	if (drv && drv->shutdown)
>> >> > +	if (drv && drv->shutdown) {
>> >> >  		drv->shutdown(pci_dev);
>> >> > -	pci_msi_shutdown(pci_dev);
>> >> > -	pci_msix_shutdown(pci_dev);
>> >> > +		pci_msi_shutdown(pci_dev);
>> >> > +		pci_msix_shutdown(pci_dev);
>> >> > +	}
>> >> 
>> >> If the driver doesn't provide a shutdown method and doesn't itself
>> >> disable MSI/MSI-X, then pci_msi(x)_shutdown won't be called anymore,
>> >> no ?
>> >
>> > Right.
>> >
>> >> 
>> >> This is probably ok (but unclean) for a system shutdown, but could
>> >> cause problems for something like kexec ?
>> >
>> > Doesn't the reset in kexec clean this up during initialization?
>> 
>> I guess it would take the same path as a reboot.
>
> I don't understand, do you mean that no reset will be done before more
> operations on the device?

I meant that it's upto the individual driver, pci will take the same path
as a regular reboot.

>> 
>> > Without shutdown, anything in the driver is unclean anyway, for example
>> > free_irq is not called.
>> 
>> True, And that is why the MSI/-X shutdown functions are called here
>> because pci can't always rely on the individual device drivers to do
>> the right thing, but atleast can make a best effort at cleaning up.
>
> This virtio-scsi case shows us that intermediate state is bad, so I still think
> we should call pci_msix_shutdown conditionally.
>
> Fam

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Alex Williamson]

On Thu, 2015-03-12 at 23:09 -0400, Bandan Das wrote:
> Ccing Alex, he can probably confirm if my understanding is indeed correct.
> 
> Fam Zheng <famz@redhat.com> writes:
> 
> > On Thu, 03/12 19:56, Bandan Das wrote:
> >> Hi Fam,
> >> 
> >> Fam Zheng <famz@redhat.com> writes:
> >> 
> >> > If the device doesn't support shutdown, disabling interrupts may cause
> >> > trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
> >> > after we disable MSI-X, futher notifications from device will be
> >> > delivered to IRQ, which is unexpected. This IRQ will not be cleared, and
> >> > may prevent us from making progress, by keep triggering interrupts.

Won't it be disabled as a spurious interrupt if it's retriggering that
quickly?  Is there something unique about virtio that makes this worse
than bare metal?

> >> > Signed-off-by: Fam Zheng <famz@redhat.com>
> >> > ---
> >> >  drivers/pci/pci-driver.c | 7 ++++---
> >> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> >> > index 3cb2210..fb29c96 100644
> >> > --- a/drivers/pci/pci-driver.c
> >> > +++ b/drivers/pci/pci-driver.c
> >> > @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
> >> >  
> >> >  	pm_runtime_resume(dev);
> >> >  
> >> > -	if (drv && drv->shutdown)
> >> > +	if (drv && drv->shutdown) {
> >> >  		drv->shutdown(pci_dev);
> >> > -	pci_msi_shutdown(pci_dev);
> >> > -	pci_msix_shutdown(pci_dev);
> >> > +		pci_msi_shutdown(pci_dev);
> >> > +		pci_msix_shutdown(pci_dev);
> >> > +	}
> >> 
> >> If the driver doesn't provide a shutdown method and doesn't itself
> >> disable MSI/MSI-X, then pci_msi(x)_shutdown won't be called anymore,
> >> no ?
> >
> > Right.
> >
> >> 
> >> This is probably ok (but unclean) for a system shutdown, but could
> >> cause problems for something like kexec ?
> >
> > Doesn't the reset in kexec clean this up during initialization?
> 
> I guess it would take the same path as a reboot.

If we were to kexec, the only thing I see touching MSI/X enable bits is
pci_msi_init_pci_dev(), which unconditionally disables MSI/X as we walk
PCI and discover devices.  Why is disabling there any different?  A new
instance of the virtio driver hasn't yet been bound to the device to
quiesce it.

> > Without shutdown, anything in the driver is unclean anyway, for example
> > free_irq is not called.
> 
> True, And that is why the MSI/-X shutdown functions are called here
> because pci can't always rely on the individual device drivers to do
> the right thing, but atleast can make a best effort at cleaning up.

A concern with this patch would be that some drivers potentially don't
implement a shutdown routine because they rely on pci-core to do this
minimal cleanup.  I'm tempted to suggest adding a call to pci_intx() to
disable INTx as part of the PCI-core shutdown, but that sounds like
asking for trouble with broken legacy devices.  It sure seems a lot
safer to make a virtio-scsi-pci shutdown function.  Thanks,

Alex

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Fam Zheng]

On Thu, 03/12 22:13, Alex Williamson wrote:
> On Thu, 2015-03-12 at 23:09 -0400, Bandan Das wrote:
> > Ccing Alex, he can probably confirm if my understanding is indeed correct.
> > 
> > Fam Zheng <famz@redhat.com> writes:
> > 
> > > On Thu, 03/12 19:56, Bandan Das wrote:
> > >> Hi Fam,
> > >> 
> > >> Fam Zheng <famz@redhat.com> writes:
> > >> 
> > >> > If the device doesn't support shutdown, disabling interrupts may cause
> > >> > trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
> > >> > after we disable MSI-X, futher notifications from device will be
> > >> > delivered to IRQ, which is unexpected. This IRQ will not be cleared, and
> > >> > may prevent us from making progress, by keep triggering interrupts.
> 
> Won't it be disabled as a spurious interrupt if it's retriggering that
> quickly?

You're right, it will. Thanks.

Fam

> 
> > >> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > >> > ---
> > >> >  drivers/pci/pci-driver.c | 7 ++++---
> > >> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >> >
> > >> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > >> > index 3cb2210..fb29c96 100644
> > >> > --- a/drivers/pci/pci-driver.c
> > >> > +++ b/drivers/pci/pci-driver.c
> > >> > @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
> > >> >  
> > >> >  	pm_runtime_resume(dev);
> > >> >  
> > >> > -	if (drv && drv->shutdown)
> > >> > +	if (drv && drv->shutdown) {
> > >> >  		drv->shutdown(pci_dev);
> > >> > -	pci_msi_shutdown(pci_dev);
> > >> > -	pci_msix_shutdown(pci_dev);
> > >> > +		pci_msi_shutdown(pci_dev);
> > >> > +		pci_msix_shutdown(pci_dev);
> > >> > +	}
> > >> 
> > >> If the driver doesn't provide a shutdown method and doesn't itself
> > >> disable MSI/MSI-X, then pci_msi(x)_shutdown won't be called anymore,
> > >> no ?
> > >
> > > Right.
> > >
> > >> 
> > >> This is probably ok (but unclean) for a system shutdown, but could
> > >> cause problems for something like kexec ?
> > >
> > > Doesn't the reset in kexec clean this up during initialization?
> > 
> > I guess it would take the same path as a reboot.
> 
> If we were to kexec, the only thing I see touching MSI/X enable bits is
> pci_msi_init_pci_dev(), which unconditionally disables MSI/X as we walk
> PCI and discover devices.  Why is disabling there any different?  A new
> instance of the virtio driver hasn't yet been bound to the device to
> quiesce it.
> 
> > > Without shutdown, anything in the driver is unclean anyway, for example
> > > free_irq is not called.
> > 
> > True, And that is why the MSI/-X shutdown functions are called here
> > because pci can't always rely on the individual device drivers to do
> > the right thing, but atleast can make a best effort at cleaning up.
> 
> A concern with this patch would be that some drivers potentially don't
> implement a shutdown routine because they rely on pci-core to do this
> minimal cleanup.  I'm tempted to suggest adding a call to pci_intx() to
> disable INTx as part of the PCI-core shutdown, but that sounds like
> asking for trouble with broken legacy devices.  It sure seems a lot
> safer to make a virtio-scsi-pci shutdown function.  Thanks,
> 
> Alex
> 

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Paolo Bonzini]

On 13/03/2015 00:21, Fam Zheng wrote:
>> I think the bug here is also that virtio-pci
>> is not defining a .shutdown callback.  It should define one and call
>> free_irq (for INTX) and pci_disable_msix.
> 
> It's not enough. The device has to know we disabled msix, otherwise it will
> send us IRQ, which is wrong.

You can use pci_intx to disable INTX too.  So I think this is a
virtio-pci bug.

Paolo

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Paolo Bonzini]

On 13/03/2015 00:21, Fam Zheng wrote:
>> I think the bug here is also that virtio-pci
>> is not defining a .shutdown callback.  It should define one and call
>> free_irq (for INTX) and pci_disable_msix.
> 
> It's not enough. The device has to know we disabled msix, otherwise it will
> send us IRQ, which is wrong.

You can use pci_intx to disable INTX too.  So I think this is a
virtio-pci bug.

Paolo

Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown

[Michael S. Tsirkin]

On Thu, Mar 12, 2015 at 01:21:22PM +0800, Fam Zheng wrote:
> If the device doesn't support shutdown, disabling interrupts may cause
> trouble. For example, virtio-scsi-pci doesn't implement shutdown, and
> after we disable MSI-X, futher notifications from device will be
> delivered to IRQ, which is unexpected.


> This IRQ will not be cleared, and
> may prevent us from making progress, by keep triggering interrupts.

I would say:
	Since there's no handler, the interrupt line will never be cleared,
	causing a deadlock.

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

However, see below:

> ---
>  drivers/pci/pci-driver.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3cb2210..fb29c96 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -448,10 +448,11 @@ static void pci_device_shutdown(struct device *dev)
>  
>  	pm_runtime_resume(dev);
>  
> -	if (drv && drv->shutdown)
> +	if (drv && drv->shutdown) {
>  		drv->shutdown(pci_dev);
> -	pci_msi_shutdown(pci_dev);
> -	pci_msix_shutdown(pci_dev);
> +		pci_msi_shutdown(pci_dev);
> +		pci_msix_shutdown(pci_dev);
> +	}
>  

So the following bus reset will disable msi anyway,
IMHO there's no need to do it in software.
kexec is more interesting. This is an attempt to leave
device in a consistent state:

	commit d52877c7b1afb8c37ebe17e2005040b79cb618b0
	Author: Yinghai Lu <yhlu.kernel.send@gmail.com>
	Date:   Wed Apr 23 14:58:09 2008 -0700
	    pci/irq: let pci_device_shutdown to call pci_msi_shutdown v2

but arguably, it's better to disable msi/msix when kexec
starts - for example, kexec might run after a crash (kdump)
and shutdown callbacks are not invoked in that case.


The reason this does not trigger for MPT is because
it has an intx handler so that gets invoked.

So here's my proposal:
- for 4.0, let's just do a virtio specific work-around,
  this seems safer.
- for 4.1, let's disable msi/msix first thing on
  kexec startup, before driver probe.

I'll post both patches shortly.

>  #ifdef CONFIG_KEXEC
>  	/*
> -- 
> 1.9.3