[PATCH v6 0/2] pci: drop msi disable on shutdown

[PATCH v6 0/2] pci: drop msi disable on shutdown

[Michael S. Tsirkin]

Fam Zheng noticed that pci shutdown disables msi and msix of a device.  This
was intended to fix kexec with fusion devices but is no longer necessary, and
can in theory be harmful.

See individual patches for the detailed explanation.

Please review, and consider for 4.2.

Eric, I would especially appreciate your feedback - I did my best
to update the commit logs with answers to questions you raised
on v5.

changes from v5:
	rebased on top of pci/msi
	fixed commit log, including comments by Bjorn
	and adding explanation to address comments/questions by Eric
	dropped stable Cc, this patch does not seem to qualify for stable
changes from v4:
        Yijing Wang <wangyijing@huawei.com> noted that
        early fixups rely on pci_msi_off.
        Split out the functionality and move off the
        required part to run early during pci_device_setup.
Changes from v3:
        fix a copy-and-paste error in
          pci: drop some duplicate code
        other patches are unchanged
        drop Cc stable for now
Changes from v2:
        move code from probe to device enumeration
        add patches to unexport pci_msi_off


Michael S. Tsirkin (2):
  PCI/MSI: Don't disable MSI/MSI-X at shutdown
  PCI/MSI: Make pci_msi_shutdown(), pci_msix_shutdown() static

 include/linux/pci.h      | 4 ----
 drivers/pci/msi.c        | 4 ++--
 drivers/pci/pci-driver.c | 2 --
 3 files changed, 2 insertions(+), 8 deletions(-)

-- 
MST

[PATCH v6 1/2] PCI/MSI: Don't disable MSI/MSI-X at shutdown

[Michael S. Tsirkin]

d52877c7b1af ("pci/irq: let pci_device_shutdown to call pci_msi_shutdown
v2") disabled MSI/MSI-X at device shutdown to address a kexec problem.

The change made by the above commit is no longer necessary: it was
superceded by commit d5dea7d95c48 ("PCI: msi: Disable msi interrupts
when we initialize a pci device") which makes sure the original kexec
problem is solved in the new kernel, and commit b566a22c23 ("PCI:
disable Bus Master on PCI device shutdown") which makes sure device does
not send MSI interrupts (MSI is a memory write and so is suppressed when
bus master is cleared).

On the contrary, disabling MSI makes it *more* likely that the device
will cause mischief since unlike MSI, INT#x interrupts are not
suppressed by clearing bus mastering.

One example of such mischief is that after we disable MSI, the device
may assert INT#x (remember, cleaning bus mastering does not disable
INT#x interrupts), and if the driver hasn't registered an interrupt
handler for it, the interrupt is never deasserted which causes an "irq
%d: nobody cared" message, with irq being subsequently disabled. This is
actually not hard to observe with virtio devices.  Not a huge problem,
but ugly, and might in theory cause other problems, e.g. if the irq
being disabled is shared with another device that attempts to use it in
its shutdown callback, or if irq debugging was explicitly disabled on
the kernel command line.

Another theoretical problem is that if the driver does not flush MSI
interrupts in the shutdown callback, MSI interrupt handler will run at
the same time as the INT#x handler, which doesn't normally happen
outside the shutdown path; Depending on the driver, the two handlers
might conflict. I did not go looking for such driver bugs but this seems
plausible.

virtio specifically has another issue: register functions change between
msi enabled and disabled modes, so disabling MSI while driver is doing
things (e.g. from a kthread) will make the device confused.

Of course, some of the above specific issues can be solved by
implementing a shutdown callback in the driver: this callback would have
to suppress further activity from both the driver and the device, and
flush outstanding handlers. This is a non-trivial amount of code that
can trigger at any time, so needs careful thought to avoid race
conditions causing bugs, and that's only running on shutdown, so isn't
well tested.

Instead, stop disabling MSIs in PCI core at shutdown: it's simple, safe,
removes code instead of adding more code, and needs no driver support.

Reported-by: Fam Zheng <famz@redhat.com>
Tested-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Yinghai Lu <yhlu.kernel.send@gmail.com>
CC: Ulrich Obergfell <uobergfe@redhat.com>
CC: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/pci/pci-driver.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 3cb2210..38a602c 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -450,8 +450,6 @@ static void pci_device_shutdown(struct device *dev)
 
 	if (drv && drv->shutdown)
 		drv->shutdown(pci_dev);
-	pci_msi_shutdown(pci_dev);
-	pci_msix_shutdown(pci_dev);
 
 #ifdef CONFIG_KEXEC
 	/*
-- 
MST

Re: [PATCH v6 1/2] PCI/MSI: Don't disable MSI/MSI-X at shutdown

[Eric W. Biederman]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> d52877c7b1af ("pci/irq: let pci_device_shutdown to call pci_msi_shutdown
> v2") disabled MSI/MSI-X at device shutdown to address a kexec problem.
>
> The change made by the above commit is no longer necessary: it was
> superceded by commit d5dea7d95c48 ("PCI: msi: Disable msi interrupts
> when we initialize a pci device") which makes sure the original kexec
> problem is solved in the new kernel, and commit b566a22c23 ("PCI:
> disable Bus Master on PCI device shutdown") which makes sure device does
> not send MSI interrupts (MSI is a memory write and so is suppressed when
> bus master is cleared).
>
> On the contrary, disabling MSI makes it *more* likely that the device
> will cause mischief since unlike MSI, INT#x interrupts are not
> suppressed by clearing bus mastering.
>
> One example of such mischief is that after we disable MSI, the device
> may assert INT#x (remember, cleaning bus mastering does not disable
> INT#x interrupts), and if the driver hasn't registered an interrupt
> handler for it, the interrupt is never deasserted which causes an "irq
> %d: nobody cared" message, with irq being subsequently disabled. This is
> actually not hard to observe with virtio devices.  Not a huge problem,
> but ugly, and might in theory cause other problems, e.g. if the irq
> being disabled is shared with another device that attempts to use it in
> its shutdown callback, or if irq debugging was explicitly disabled on
> the kernel command line.

And disabling irq debugging is stupid, we should probably delete that
option.  We poll disabled irqs periodically to keep the disabled ones
from completely killing your system even if they are shared.

> Another theoretical problem is that if the driver does not flush MSI
> interrupts in the shutdown callback, MSI interrupt handler will run at
> the same time as the INT#x handler, which doesn't normally happen
> outside the shutdown path; Depending on the driver, the two handlers
> might conflict. I did not go looking for such driver bugs but this seems
> plausible.

Again a buggy driver issue.

> virtio specifically has another issue: register functions change between
> msi enabled and disabled modes, so disabling MSI while driver is doing
> things (e.g. from a kthread) will make the device confused.

Huh??? Virtio is virtual hardware.  We are talking about real hardware.
How do the two mix?  Is there a pass through going on and we let the
virtual machine get away with putting the hardware in a nasty state,
and don't clean it up in the real driver?

This sounds like a case for fixing whatever insanity is happening in the
virtio driver.

This also sounds like a case for implementing a shutdown callback and
disabling things properly.  A properly shutdown driver should have
already disabled MSI's.  A driver is responsible for enabling MSIs so it
should be responsible for disabling it.  The core disabling MSIs is
mostly to catch the handful of lazy drivers that forget.

The bottom line is that there are a few things that are standard
behavior that we can do in the generic code, but at the end of the day
it is the responsibility of the driver to shut things down and whatever
driver you are dealing with clearly has a bunch of bugs and you aren't
fixing it. 

> Of course, some of the above specific issues can be solved by
> implementing a shutdown callback in the driver: this callback would have
> to suppress further activity from both the driver and the device, and
> flush outstanding handlers. This is a non-trivial amount of code that
> can trigger at any time, so needs careful thought to avoid race
> conditions causing bugs, and that's only running on shutdown, so isn't
> well tested.

Agreed.  However the code should already exist in your drivers remove
method.  So with a little careful factoring you should be able to
use a well tested code path.

If you aren't happy with the separation between remove and shutdown
please take it up with whoever maintains the driver API these days.
I lost that fight the first time and I haven't had the energy to take it
any time since. 

Also we aren't talking about kexec-on-panic here.  So no, it can not
trigger at any time.  We are talking about kexec in as an orderly
transition to another kernel.  So it should be possible to perform an
orderly shutdown.

If you can't figure out in the driver how to create an orderly shutdown
I know we can't figure out how to do it in generic code outside of the
driver.  Especially not for every device.

> Instead, stop disabling MSIs in PCI core at shutdown: it's simple, safe,
> removes code instead of adding more code, and needs no driver support.

I can maybe almost see an argument for disabling intx as well.

I really can not buy the argument you are sending. AKA "One day I met a
bugggy driver.  The driver was so buggy that my system did not work
properly.  Instead of fixing the driver let's do crazy things, to maybe
avoid some of the bugs in the driver"

The thing is this is the same path that we use to transition to firmware
and the goal is to put the hardware in something closely resembly the
state it was in when the firmware gave us the hardware.  A state where
linux and potentially other OS's can initialize the hardware and make it
work.

The whole bus-master disable that happens on the kexec path is just
a best attempt to make things happen and it works in enough cases
it is worth doing.  Especially when the normal state of the hardware
from the boot firmware is with bus master disabled, and a good shutdown
routine will clear bus-master enable anyway.

Interrupts and intx are in a very different situation.  Typically
interrupts and intx are enabled when they come from the boot firmware.
Interrupts should not fire unless there is device activity. 

Most devices are just passive and don't have on-going activity so a
shutdown method is not particularly needed.  But clearly you have a
device that if you don't tell it to do something goes around sending
interrupts anyway.  So that driver needs to be told to shut-up and stop.

As far as leaving interrupts running and causing problems switch from
msi to intx is not particularly significant.  There is a window when
interrupts can fire and cause havoc.  By leaving msi enabled I don't see
the problem going away, just the kind of havoc changing.

I would be a lot more inclined to figure out how to get the code that
calls shutdown methods to call a drivers remove method instead.  That
would simplify things.  Unfortunately the architects of the driver
callbacks wanted something complex that would not get well tested so we
have a bit of a mess.

Eric

Re: [PATCH v6 1/2] PCI/MSI: Don't disable MSI/MSI-X at shutdown

[Michael S. Tsirkin]

On Tue, May 12, 2015 at 02:22:05PM -0500, Eric W. Biederman wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > d52877c7b1af ("pci/irq: let pci_device_shutdown to call pci_msi_shutdown
> > v2") disabled MSI/MSI-X at device shutdown to address a kexec problem.
> >
> > The change made by the above commit is no longer necessary: it was
> > superceded by commit d5dea7d95c48 ("PCI: msi: Disable msi interrupts
> > when we initialize a pci device") which makes sure the original kexec
> > problem is solved in the new kernel, and commit b566a22c23 ("PCI:
> > disable Bus Master on PCI device shutdown") which makes sure device does
> > not send MSI interrupts (MSI is a memory write and so is suppressed when
> > bus master is cleared).
> >
> > On the contrary, disabling MSI makes it *more* likely that the device
> > will cause mischief since unlike MSI, INT#x interrupts are not
> > suppressed by clearing bus mastering.
> >
> > One example of such mischief is that after we disable MSI, the device
> > may assert INT#x (remember, cleaning bus mastering does not disable
> > INT#x interrupts), and if the driver hasn't registered an interrupt
> > handler for it, the interrupt is never deasserted which causes an "irq
> > %d: nobody cared" message, with irq being subsequently disabled. This is
> > actually not hard to observe with virtio devices.  Not a huge problem,
> > but ugly, and might in theory cause other problems, e.g. if the irq
> > being disabled is shared with another device that attempts to use it in
> > its shutdown callback, or if irq debugging was explicitly disabled on
> > the kernel command line.
> 
> And disabling irq debugging is stupid, we should probably delete that
> option.  We poll disabled irqs periodically to keep the disabled ones
> from completely killing your system even if they are shared.
> 
> > Another theoretical problem is that if the driver does not flush MSI
> > interrupts in the shutdown callback, MSI interrupt handler will run at
> > the same time as the INT#x handler, which doesn't normally happen
> > outside the shutdown path; Depending on the driver, the two handlers
> > might conflict. I did not go looking for such driver bugs but this seems
> > plausible.
> 
> Again a buggy driver issue.
> 
> > virtio specifically has another issue: register functions change between
> > msi enabled and disabled modes, so disabling MSI while driver is doing
> > things (e.g. from a kthread) will make the device confused.
> 
> Huh??? Virtio is virtual hardware.  We are talking about real hardware.
> How do the two mix?  Is there a pass through going on and we let the
> virtual machine get away with putting the hardware in a nasty state,
> and don't clean it up in the real driver?

Yes pass through can mix real hardware and virtio.
We are talking about shutdown within the VM so the hypervisor
does not get a chance to run and clean up anything.

> This sounds like a case for fixing whatever insanity is happening in the
> virtio driver.
> 
> This also sounds like a case for implementing a shutdown callback and
> disabling things properly.  A properly shutdown driver should have
> already disabled MSI's.  A driver is responsible for enabling MSIs so it
> should be responsible for disabling it.  The core disabling MSIs is
> mostly to catch the handful of lazy drivers that forget.


Okay! And I am saying that if the driver did forget,
we are better off not disabling it - leave it enabled
until kexec starts and disables it.


> The bottom line is that there are a few things that are standard
> behavior that we can do in the generic code, but at the end of the day
> it is the responsibility of the driver to shut things down and whatever
> driver you are dealing with clearly has a bunch of bugs and you aren't
> fixing it. 

So please let us get on with fixing it in driver and stop
playing with device in core.


> > Of course, some of the above specific issues can be solved by
> > implementing a shutdown callback in the driver: this callback would have
> > to suppress further activity from both the driver and the device, and
> > flush outstanding handlers. This is a non-trivial amount of code that
> > can trigger at any time, so needs careful thought to avoid race
> > conditions causing bugs, and that's only running on shutdown, so isn't
> > well tested.
> 
> Agreed.  However the code should already exist in your drivers remove
> method.  So with a little careful factoring you should be able to
> use a well tested code path.
>
> If you aren't happy with the separation between remove and shutdown
> please take it up with whoever maintains the driver API these days.
> I lost that fight the first time and I haven't had the energy to take it
> any time since. 
> 
> Also we aren't talking about kexec-on-panic here.  So no, it can not
> trigger at any time.  We are talking about kexec in as an orderly
> transition to another kernel.  So it should be possible to perform an
> orderly shutdown.

Why aren't we talking about kexec-on-panic? That one doesn't
trigger shutdown callbacks?

> If you can't figure out in the driver how to create an orderly shutdown
> I know we can't figure out how to do it in generic code outside of the
> driver.  Especially not for every device.

Right! So please let's stop pretending we do, touch the
device as little as possible. This is exactly what my patches do:
don't touch msi since we don't have to.

> > Instead, stop disabling MSIs in PCI core at shutdown: it's simple, safe,
> > removes code instead of adding more code, and needs no driver support.
> 
> I can maybe almost see an argument for disabling intx as well.
> 
> I really can not buy the argument you are sending. AKA "One day I met a
> bugggy driver.  The driver was so buggy that my system did not work
> properly.  Instead of fixing the driver let's do crazy things, to maybe
> avoid some of the bugs in the driver"

I am saying it is driver's responsibility to disable msi on shutdown.
If it doesn't then it knows best. Don't second-guess it.

> The thing is this is the same path that we use to transition to firmware
> and the goal is to put the hardware in something closely resembly the
> state it was in when the firmware gave us the hardware.  A state where
> linux and potentially other OS's can initialize the hardware and make it
> work.
> 
> The whole bus-master disable that happens on the kexec path is just
> a best attempt to make things happen and it works in enough cases
> it is worth doing.  Especially when the normal state of the hardware
> from the boot firmware is with bus master disabled, and a good shutdown
> routine will clear bus-master enable anyway.
> 
> Interrupts and intx are in a very different situation.  Typically
> interrupts and intx are enabled when they come from the boot firmware.
> Interrupts should not fire unless there is device activity. 
> 
> Most devices are just passive and don't have on-going activity so a
> shutdown method is not particularly needed.  But clearly you have a
> device that if you don't tell it to do something goes around sending
> interrupts anyway.  So that driver needs to be told to shut-up and stop.

So the same argument should have been applied instead of d52877c7b1af.
Apparently for fusion io if you just disable msi and don't do anything
else, things get rosy. But it's a device specific thing.  All I am
saying doing this in pci core is wrong, pci core has no idea how to
properly tell the device to shut-up and stop, it is driver's
responsibility.


> As far as leaving interrupts running and causing problems switch from
> msi to intx is not particularly significant.  There is a window when
> interrupts can fire and cause havoc.  By leaving msi enabled I don't see
> the problem going away, just the kind of havoc changing.

How can havoc happen? We disable bus master, no msi will be sent.

> I would be a lot more inclined to figure out how to get the code that
> calls shutdown methods to call a drivers remove method instead.  That
> would simplify things.  Unfortunately the architects of the driver
> callbacks wanted something complex that would not get well tested so we
> have a bit of a mess.
> 
> Eric

Really my patch is all about doing the minimal thing in core
since we don't really know how to cleanup the device.

-- 
MST

Re: [PATCH v6 1/2] PCI/MSI: Don't disable MSI/MSI-X at shutdown

[Michael S. Tsirkin]

On Wed, May 13, 2015 at 08:41:55AM +0200, Michael S. Tsirkin wrote:
> > This also sounds like a case for implementing a shutdown callback and
> > disabling things properly.  A properly shutdown driver should have
> > already disabled MSI's.  A driver is responsible for enabling MSIs so it
> > should be responsible for disabling it.  The core disabling MSIs is
> > mostly to catch the handful of lazy drivers that forget.
> 
> 
> Okay! And I am saying that if the driver did forget,
> we are better off not disabling it - leave it enabled
> until kexec starts and disables it.
> 
> 
> > The bottom line is that there are a few things that are standard
> > behavior that we can do in the generic code, but at the end of the day
> > it is the responsibility of the driver to shut things down and whatever
> > driver you are dealing with clearly has a bunch of bugs and you aren't
> > fixing it. 
> 
> So please let us get on with fixing it in driver and stop
> playing with device in core.

Eric, does this argument make sense?  Drivers should do the right thing
in their shutdown callback, let's not try to work around their bugs in
core.

-- 
MST

Re: [PATCH v6 1/2] PCI/MSI: Don't disable MSI/MSI-X at shutdown

[Eric W. Biederman]

On May 14, 2015 1:06:00 AM CDT, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>On Wed, May 13, 2015 at 08:41:55AM +0200, Michael S. Tsirkin wrote:
>> > This also sounds like a case for implementing a shutdown callback
>and
>> > disabling things properly.  A properly shutdown driver should have
>> > already disabled MSI's.  A driver is responsible for enabling MSIs
>so it
>> > should be responsible for disabling it.  The core disabling MSIs is
>> > mostly to catch the handful of lazy drivers that forget.
>> 
>> 
>> Okay! And I am saying that if the driver did forget,
>> we are better off not disabling it - leave it enabled
>> until kexec starts and disables it.
>> 
>> 
>> > The bottom line is that there are a few things that are standard
>> > behavior that we can do in the generic code, but at the end of the
>day
>> > it is the responsibility of the driver to shut things down and
>whatever
>> > driver you are dealing with clearly has a bunch of bugs and you
>aren't
>> > fixing it. 
>> 
>> So please let us get on with fixing it in driver and stop
>> playing with device in core.
>
>Eric, does this argument make sense?  Drivers should do the right thing
>in their shutdown callback, let's not try to work around their bugs in
>core.

Not in context of this patch, as this change appears to
be to avoid fixing the driver.

Further this behavior in the core has existed for the
better part of a decade.  Who knows what weird
behavior (called regressions) this will trigger with
other drivers in other situations.

I do not see any reason to change the existing behavior here.

Especially as if you try and boot a non-linux kernel with
kexec you are almost certainly going to subject it to a
screaming MSI interrupt and there almost certainly will
not be code to disable MSIs as they are disabled by at
boot up by default.

Eric

Re: [PATCH v6 1/2] PCI/MSI: Don't disable MSI/MSI-X at shutdown

[Michael S. Tsirkin]

On Thu, May 14, 2015 at 02:58:59AM -0500, Eric W. Biederman wrote:
> 
> 
> On May 14, 2015 1:06:00 AM CDT, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >On Wed, May 13, 2015 at 08:41:55AM +0200, Michael S. Tsirkin wrote:
> >> > This also sounds like a case for implementing a shutdown callback
> >and
> >> > disabling things properly.  A properly shutdown driver should have
> >> > already disabled MSI's.  A driver is responsible for enabling MSIs
> >so it
> >> > should be responsible for disabling it.  The core disabling MSIs is
> >> > mostly to catch the handful of lazy drivers that forget.
> >> 
> >> 
> >> Okay! And I am saying that if the driver did forget,
> >> we are better off not disabling it - leave it enabled
> >> until kexec starts and disables it.
> >> 
> >> 
> >> > The bottom line is that there are a few things that are standard
> >> > behavior that we can do in the generic code, but at the end of the
> >day
> >> > it is the responsibility of the driver to shut things down and
> >whatever
> >> > driver you are dealing with clearly has a bunch of bugs and you
> >aren't
> >> > fixing it. 
> >> 
> >> So please let us get on with fixing it in driver and stop
> >> playing with device in core.
> >
> >Eric, does this argument make sense?  Drivers should do the right thing
> >in their shutdown callback, let's not try to work around their bugs in
> >core.
> 
> Not in context of this patch, as this change appears to
> be to avoid fixing the driver.

Not really. I do intend to work on adding shutdown to virtio: e.g. we
really must change avoid running config change interrupts.
but I would like the core to do the right thing, so I argue
about what's best for general core to do, and what has the least
chance to cause hangs.

> Further this behavior in the core has existed for the
> better part of a decade.
>  Who knows what weird
> behavior (called regressions) this will trigger with
> other drivers in other situations.

That is also part of the argument. It used to be so, but
now it really can not trigger any regressions because we
have since added a bigger hammer: disabling bus mastering.

Do you disagree with this? How can leaving msi on cause harm?

> I do not see any reason to change the existing behavior here.
> 
> Especially as if you try and boot a non-linux kernel with
> kexec

First time I hear about this requirement. Does anyone do this? I find
it hard to believe it works ...

> you are almost certainly going to subject it to a
> screaming MSI interrupt and there almost certainly will
> not be code to disable MSIs as they are disabled by at
> boot up by default.
> 
> Eric

OTOH if you do disable MSI but leave device functioning you will just
get screaming INT#x which is even worse because it will end up disabling
INT#x which is shared, and so breaking multiple devices and not just
this one.

-- 
MST

Re: [PATCH v6 1/2] PCI/MSI: Don't disable MSI/MSI-X at shutdown

[Michael S. Tsirkin]

On Thu, May 14, 2015 at 11:53:34AM +0200, Michael S. Tsirkin wrote:
> > you are almost certainly going to subject it to a
> > screaming MSI interrupt and there almost certainly will
> > not be code to disable MSIs as they are disabled by at
> > boot up by default.
> > 
> > Eric
> 
> OTOH if you do disable MSI but leave device functioning you will just
> get screaming INT#x which is even worse because it will end up disabling
> INT#x which is shared, and so breaking multiple devices and not just
> this one.

Eric, can you comment on this please?
To list the points again:
	1- if device is properly shut down there's no need to disable MSIs
	2- if device is not properly shut down disabling MSIs will cause
	   screaming INT#x interrupts
	3- in both cases if we leave MSI on then we can be sure we won't
	   get screaming interrupts since we now disable bus mastering
           which suppresses MSIs (but not INT#x)

Looking at the commit that added the disable, that is d52877c7b1af, you
will see that historically it preceded b566a22c23. So the screaming MSI
problem that d52877c7b1af tried to address is now gone, addressed by
b566a22c23.

What do you think?

> -- 
> MST

Re: [PATCH v6 1/2] PCI/MSI: Don't disable MSI/MSI-X at shutdown

[Michael S. Tsirkin]

On Thu, May 28, 2015 at 06:36:22PM +0200, Michael S. Tsirkin wrote:
> On Thu, May 14, 2015 at 11:53:34AM +0200, Michael S. Tsirkin wrote:
> > > you are almost certainly going to subject it to a
> > > screaming MSI interrupt and there almost certainly will
> > > not be code to disable MSIs as they are disabled by at
> > > boot up by default.
> > > 
> > > Eric
> > 
> > OTOH if you do disable MSI but leave device functioning you will just
> > get screaming INT#x which is even worse because it will end up disabling
> > INT#x which is shared, and so breaking multiple devices and not just
> > this one.
> 
> Eric, can you comment on this please?
> To list the points again:
> 	1- if device is properly shut down there's no need to disable MSIs
> 	2- if device is not properly shut down disabling MSIs will cause
> 	   screaming INT#x interrupts
> 	3- in both cases if we leave MSI on then we can be sure we won't
> 	   get screaming interrupts since we now disable bus mastering
>            which suppresses MSIs (but not INT#x)
> 
> Looking at the commit that added the disable, that is d52877c7b1af, you
> will see that historically it preceded b566a22c23. So the screaming MSI
> problem that d52877c7b1af tried to address is now gone, addressed by
> b566a22c23.
> 
> What do you think?

Eric, Bjor, could you comment?
I'm sorry if these repeating questions are annoying to you, but the
comment that not disabling msi can cause screaming interrupts seems to
be based on state of the kernel before 2012.

Ever since
commit b566a22c23327f18ce941ffad0ca907e50a53d41
    PCI: disable Bus Master on PCI device shutdown
even broken devices that don't disable msi on shutdown can
not cause screaming interrupts even if core leaves msi alone.

So I don't see how can removing that code cause regressions, and it
might improve the situation since it reduces the chance of
getting screaming INT#x.

> > -- 
> > MST

Re: [PATCH v6 1/2] PCI/MSI: Don't disable MSI/MSI-X at shutdown

[Bjorn Helgaas]

On Tue, May 12, 2015 at 03:03:32PM +0200, Michael S. Tsirkin wrote:
> d52877c7b1af ("pci/irq: let pci_device_shutdown to call pci_msi_shutdown
> v2") disabled MSI/MSI-X at device shutdown to address a kexec problem.
> ... 

I know you're trying to put all the justification in the changelog, and
that's great if it can be done.  But would you please just add the single
link here to https://bugzilla.kernel.org/show_bug.cgi?id=96571 ?

And please attach the dmesg log and instructions for reproducing the
problem to the bugzilla.  I've asked for this before, and it seems like a
simple request, but maybe there's a reason it's more complicated than it
seems to me.  It's obvious to you how all this fits together, but I'd like
it to be more concrete to the rest of us, too.

The bugzilla says Ulrich Obergfell noticed this in RHEL7.  If there's a
RHEL bugzilla, it'd be nice to have a link to it in the kernel.org
bugzilla.  Informal hints are great right now, but they'll be useless after
six months.

Bjorn

> Reported-by: Fam Zheng <famz@redhat.com>
> Tested-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: Yinghai Lu <yhlu.kernel.send@gmail.com>
> CC: Ulrich Obergfell <uobergfe@redhat.com>
> CC: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/pci/pci-driver.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3cb2210..38a602c 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -450,8 +450,6 @@ static void pci_device_shutdown(struct device *dev)
>  
>  	if (drv && drv->shutdown)
>  		drv->shutdown(pci_dev);
> -	pci_msi_shutdown(pci_dev);
> -	pci_msix_shutdown(pci_dev);
>  
>  #ifdef CONFIG_KEXEC
>  	/*
> -- 
> MST
> 

Re: [PATCH v6 1/2] PCI/MSI: Don't disable MSI/MSI-X at shutdown

[Fam Zheng]

On Tue, 05/19 09:58, Bjorn Helgaas wrote:
> On Tue, May 12, 2015 at 03:03:32PM +0200, Michael S. Tsirkin wrote:
> > d52877c7b1af ("pci/irq: let pci_device_shutdown to call pci_msi_shutdown
> > v2") disabled MSI/MSI-X at device shutdown to address a kexec problem.
> > ... 

Hi Bjorn,
> 
> I know you're trying to put all the justification in the changelog, and
> that's great if it can be done.  But would you please just add the single
> link here to https://bugzilla.kernel.org/show_bug.cgi?id=96571 ?
> 
> And please attach the dmesg log and instructions for reproducing the
> problem to the bugzilla.  I've asked for this before, and it seems like a
> simple request, but maybe there's a reason it's more complicated than it
> seems to me.  It's obvious to you how all this fits together, but I'd like
> it to be more concrete to the rest of us, too.

I've updated what I could see with current master build as much as I can, the
softlockup is not observed in my tests, but the dmesg log already shows the
spurious IRQ.

> 
> The bugzilla says Ulrich Obergfell noticed this in RHEL7.  If there's a
> RHEL bugzilla, it'd be nice to have a link to it in the kernel.org
> bugzilla.  Informal hints are great right now, but they'll be useless after
> six months.

The original bug is not public accessible, so I didn't include the link.

Fam

[PATCH v6 2/2] PCI/MSI: Make pci_msi_shutdown(), pci_msix_shutdown() static

[Michael S. Tsirkin]

pci_msi_shutdown() and pci_msix_shutdown() are now internal to
drivers/pci/msi.c; make them static.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 include/linux/pci.h | 4 ----
 drivers/pci/msi.c   | 4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 50b7c7d..bc50bb0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1213,11 +1213,9 @@ struct msix_entry {
 
 #ifdef CONFIG_PCI_MSI
 int pci_msi_vec_count(struct pci_dev *dev);
-void pci_msi_shutdown(struct pci_dev *dev);
 void pci_disable_msi(struct pci_dev *dev);
 int pci_msix_vec_count(struct pci_dev *dev);
 int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec);
-void pci_msix_shutdown(struct pci_dev *dev);
 void pci_disable_msix(struct pci_dev *dev);
 void pci_restore_msi_state(struct pci_dev *dev);
 int pci_msi_enabled(void);
@@ -1241,13 +1239,11 @@ static inline int pci_enable_msix_exact(struct pci_dev *dev,
 }
 #else
 static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; }
-static inline void pci_msi_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msi(struct pci_dev *dev) { }
 static inline int pci_msix_vec_count(struct pci_dev *dev) { return -ENOSYS; }
 static inline int pci_enable_msix(struct pci_dev *dev,
 				  struct msix_entry *entries, int nvec)
 { return -ENOSYS; }
-static inline void pci_msix_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msix(struct pci_dev *dev) { }
 static inline void pci_restore_msi_state(struct pci_dev *dev) { }
 static inline int pci_msi_enabled(void) { return 0; }
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index f66be86..ea78a07 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -887,7 +887,7 @@ int pci_msi_vec_count(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_msi_vec_count);
 
-void pci_msi_shutdown(struct pci_dev *dev)
+static void pci_msi_shutdown(struct pci_dev *dev)
 {
 	struct msi_desc *desc;
 	u32 mask;
@@ -993,7 +993,7 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
 }
 EXPORT_SYMBOL(pci_enable_msix);
 
-void pci_msix_shutdown(struct pci_dev *dev)
+static void pci_msix_shutdown(struct pci_dev *dev)
 {
 	struct msi_desc *entry;
 
-- 
MST