LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
@ 2015-03-12  5:21 Fam Zheng
  2015-03-12 16:21 ` Paolo Bonzini
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Fam Zheng @ 2015-03-12  5:21 UTC (permalink / raw)
  To: linux-kernel; +Cc: Bjorn Helgaas, linux-pci, famz

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
  2015-03-12  5:21 [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown Fam Zheng
@ 2015-03-12 16:21 ` Paolo Bonzini
  2015-03-12 23:21   ` Fam Zheng
  2015-03-12 23:56 ` Bandan Das
  2015-03-16 12:14 ` Michael S. Tsirkin
  2 siblings, 1 reply; 12+ messages in thread
From: Paolo Bonzini @ 2015-03-12 16:21 UTC (permalink / raw)
  To: Fam Zheng, linux-kernel; +Cc: Bjorn Helgaas, linux-pci, Linux Virtualization



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
  2015-03-12 16:21 ` Paolo Bonzini
@ 2015-03-12 23:21   ` Fam Zheng
  2015-03-13 13:03     ` Paolo Bonzini
  0 siblings, 1 reply; 12+ messages in thread
From: Fam Zheng @ 2015-03-12 23:21 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: linux-kernel, Bjorn Helgaas, linux-pci, Linux Virtualization, mst

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
  2015-03-12  5:21 [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown Fam Zheng
  2015-03-12 16:21 ` Paolo Bonzini
@ 2015-03-12 23:56 ` Bandan Das
  2015-03-13  2:09   ` Fam Zheng
  2015-03-16 12:14 ` Michael S. Tsirkin
  2 siblings, 1 reply; 12+ messages in thread
From: Bandan Das @ 2015-03-12 23:56 UTC (permalink / raw)
  To: Fam Zheng; +Cc: linux-kernel, Bjorn Helgaas, linux-pci

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
>  	/*

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
  2015-03-12 23:56 ` Bandan Das
@ 2015-03-13  2:09   ` Fam Zheng
  2015-03-13  3:09     ` Bandan Das
  0 siblings, 1 reply; 12+ messages in thread
From: Fam Zheng @ 2015-03-13  2:09 UTC (permalink / raw)
  To: Bandan Das; +Cc: linux-kernel, Bjorn Helgaas, linux-pci

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
  2015-03-13  2:09   ` Fam Zheng
@ 2015-03-13  3:09     ` Bandan Das
  2015-03-13  3:17       ` Fam Zheng
  2015-03-13  4:13       ` Alex Williamson
  0 siblings, 2 replies; 12+ messages in thread
From: Bandan Das @ 2015-03-13  3:09 UTC (permalink / raw)
  To: Fam Zheng; +Cc: linux-kernel, Bjorn Helgaas, linux-pci, Alex Williamson

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
  2015-03-13  3:09     ` Bandan Das
@ 2015-03-13  3:17       ` Fam Zheng
  2015-03-13  3:35         ` Bandan Das
  2015-03-13  4:13       ` Alex Williamson
  1 sibling, 1 reply; 12+ messages in thread
From: Fam Zheng @ 2015-03-13  3:17 UTC (permalink / raw)
  To: Bandan Das; +Cc: linux-kernel, Bjorn Helgaas, linux-pci, Alex Williamson

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
  2015-03-13  3:17       ` Fam Zheng
@ 2015-03-13  3:35         ` Bandan Das
  0 siblings, 0 replies; 12+ messages in thread
From: Bandan Das @ 2015-03-13  3:35 UTC (permalink / raw)
  To: Fam Zheng; +Cc: linux-kernel, Bjorn Helgaas, linux-pci, Alex Williamson

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
  2015-03-13  3:09     ` Bandan Das
  2015-03-13  3:17       ` Fam Zheng
@ 2015-03-13  4:13       ` Alex Williamson
  2015-03-13  4:53         ` Fam Zheng
  1 sibling, 1 reply; 12+ messages in thread
From: Alex Williamson @ 2015-03-13  4:13 UTC (permalink / raw)
  To: Bandan Das; +Cc: Fam Zheng, linux-kernel, Bjorn Helgaas, linux-pci

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
  2015-03-13  4:13       ` Alex Williamson
@ 2015-03-13  4:53         ` Fam Zheng
  0 siblings, 0 replies; 12+ messages in thread
From: Fam Zheng @ 2015-03-13  4:53 UTC (permalink / raw)
  To: Alex Williamson; +Cc: Bandan Das, linux-kernel, Bjorn Helgaas, linux-pci

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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
  2015-03-12 23:21   ` Fam Zheng
@ 2015-03-13 13:03     ` Paolo Bonzini
  0 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2015-03-13 13:03 UTC (permalink / raw)
  To: Fam Zheng
  Cc: linux-kernel, Bjorn Helgaas, linux-pci, Linux Virtualization, mst



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown
  2015-03-12  5:21 [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown Fam Zheng
  2015-03-12 16:21 ` Paolo Bonzini
  2015-03-12 23:56 ` Bandan Das
@ 2015-03-16 12:14 ` Michael S. Tsirkin
  2 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2015-03-16 12:14 UTC (permalink / raw)
  To: Fam Zheng; +Cc: linux-kernel, Bjorn Helgaas, linux-pci, yhlu.kernel.send

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





^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-03-16 12:14 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-12  5:21 [RFC PATCH] PCI: Disable MSI/MSI-X only if device is shutdown Fam Zheng
2015-03-12 16:21 ` Paolo Bonzini
2015-03-12 23:21   ` Fam Zheng
2015-03-13 13:03     ` Paolo Bonzini
2015-03-12 23:56 ` Bandan Das
2015-03-13  2:09   ` Fam Zheng
2015-03-13  3:09     ` Bandan Das
2015-03-13  3:17       ` Fam Zheng
2015-03-13  3:35         ` Bandan Das
2015-03-13  4:13       ` Alex Williamson
2015-03-13  4:53         ` Fam Zheng
2015-03-16 12:14 ` Michael S. Tsirkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).