Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On 11/8/18 2:09 PM, Bjorn Helgaas wrote: > > [EXTERNAL EMAIL] > Please report any suspicious attachments, links, or requests for sensitive > information. > > > [+cc Jonathan, Greg, Lukas, Russell, Sam, Oliver for discussion about > PCI error recovery in general] > > On Wed, Nov 07, 2018 at 05:42:57PM -0600, Bjorn Helgaas wrote: >> On Tue, Sep 18, 2018 at 05:15:00PM -0500, Alexandru Gagniuc wrote: >>> When a PCI device is gone, we don't want to send IO to it if we can >>> avoid it. We expose functionality via the irq_chip structure. As >>> users of that structure may not know about the underlying PCI device, >>> it's our responsibility to guard against removed devices. >>> >>> .irq_write_msi_msg() is already guarded inside __pci_write_msi_msg(). >>> .irq_mask/unmask() are not. Guard them for completeness. >>> >>> For example, surprise removal of a PCIe device triggers teardown. This >>> touches the irq_chips ops some point to disable the interrupts. I/O >>> generated here can crash the system on firmware-first machines. >>> Not triggering the IO in the first place greatly reduces the >>> possibility of the problem occurring. >>> >>> Signed-off-by: Alexandru Gagniuc >> >> Applied to pci/misc for v4.21, thanks! > > I'm having second thoughts about this. Do we have a verdict on this? If you don't like this approach, then I'll have to fix the problem in some other way, but the problem still needs to be fixed. Alex
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Fri, Nov 09, 2018 at 03:32:57AM -0800, Greg Kroah-Hartman wrote: > On Fri, Nov 09, 2018 at 08:29:53AM +0100, Lukas Wunner wrote: > > On Thu, Nov 08, 2018 at 02:01:17PM -0800, Greg Kroah-Hartman wrote: > > > On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote: > > > > I'm having second thoughts about this. One thing I'm uncomfortable > > > > with is that sprinkling pci_dev_is_disconnected() around feels ad hoc > > > > > > I think my stance always has been that this call is not good at all > > > because once you call it you never really know if it is still true as > > > the device could have been removed right afterward. > > > > > > So almost any code that relies on it is broken, there is no locking and > > > it can and will race and you will loose. > > > > Hm, to be honest if that's your impression I think you must have missed a > > large portion of the discussion we've been having over the past 2 years. > > > > Please consider reading this LWN article, particularly the "Surprise > > removal" section, to get up to speed: > > > > https://lwn.net/Articles/767885/ > > > > You seem to be assuming that all we care about is the *return value* of > > an mmio read. However a transaction to a surprise removed device has > > side effects beyond returning all ones, such as a Completion Timeout > > which, with thousands of transactions in flight, added up to many seconds > > to handle removal of an NVMe array and occasionally caused MCEs. > > Again, I still claim this is broken hardware/firmware :) Indeed it is, but I don't want to abandon people with hardware in hand if we can make it work despite being broken. Perfection is the enemy of good. :) > > It is not an option to just blindly carry out device accesses even though > > it is known the device is gone, Completion Timeouts be damned. > > I don't disagree with you at all, and your other email is great with > summarizing the issues here. > > What I do object to is somehow relying on that function call as knowing > that the device really is present or not. It's a good hint, yes, but > driver authors still have to be able to handle the bad data coming back > from when the call races with the device being removed. The function has always been a private interface. It is not available for drivers to rely on. The only thing we're trying to accomplish is not start a transaction if software knows it will not succeed. There are certainly times when a transaction will fail that software does not forsee, but we're not suggesting the intent handles that either.
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Fri, Nov 09, 2018 at 03:32:57AM -0800, Greg Kroah-Hartman wrote: > On Fri, Nov 09, 2018 at 08:29:53AM +0100, Lukas Wunner wrote: > > On Thu, Nov 08, 2018 at 02:01:17PM -0800, Greg Kroah-Hartman wrote: > > > On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote: > > > > I'm having second thoughts about this. One thing I'm uncomfortable > > > > with is that sprinkling pci_dev_is_disconnected() around feels ad hoc > > > > > > I think my stance always has been that this call is not good at all > > > because once you call it you never really know if it is still true as > > > the device could have been removed right afterward. > > > > > > So almost any code that relies on it is broken, there is no locking and > > > it can and will race and you will loose. > > > > Hm, to be honest if that's your impression I think you must have missed a > > large portion of the discussion we've been having over the past 2 years. > > > > Please consider reading this LWN article, particularly the "Surprise > > removal" section, to get up to speed: > > > > https://lwn.net/Articles/767885/ > > > > You seem to be assuming that all we care about is the *return value* of > > an mmio read. However a transaction to a surprise removed device has > > side effects beyond returning all ones, such as a Completion Timeout > > which, with thousands of transactions in flight, added up to many seconds > > to handle removal of an NVMe array and occasionally caused MCEs. > > Again, I still claim this is broken hardware/firmware :) Indeed it is, but I don't want to abandon people with hardware in hand if we can make it work despite being broken. Perfection is the enemy of good. :) > > It is not an option to just blindly carry out device accesses even though > > it is known the device is gone, Completion Timeouts be damned. > > I don't disagree with you at all, and your other email is great with > summarizing the issues here. > > What I do object to is somehow relying on that function call as knowing > that the device really is present or not. It's a good hint, yes, but > driver authors still have to be able to handle the bad data coming back > from when the call races with the device being removed. The function has always been a private interface. It is not available for drivers to rely on. The only thing we're trying to accomplish is not start a transaction if software knows it will not succeed. There are certainly times when a transaction will fail that software does not forsee, but we're not suggesting the intent handles that either.
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Thu, Nov 08, 2018 at 02:42:55PM -0800, Greg Kroah-Hartman wrote: > On Thu, Nov 08, 2018 at 03:32:58PM -0700, Keith Busch wrote: > > On Thu, Nov 08, 2018 at 02:01:17PM -0800, Greg Kroah-Hartman wrote: > > > On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote: > > > > I'm having second thoughts about this. One thing I'm uncomfortable > > > > with is that sprinkling pci_dev_is_disconnected() around feels ad hoc > > > > instead of systematic, in the sense that I don't know how we convince > > > > ourselves that this (and only this) is the correct place to put it. > > > > > > I think my stance always has been that this call is not good at all > > > because once you call it you never really know if it is still true as > > > the device could have been removed right afterward. > > > > > > So almost any code that relies on it is broken, there is no locking and > > > it can and will race and you will loose. > > > > AIUI, we're not trying to create code to rely on this. This more about > > reducing reliance on hardware. If the software misses the race once and > > accesses disconnected device memory, that's usually not a big deal to > > let hardware sort it out, but the point is not to push our luck. > > Then why even care about this call at all? If you need to really know > if the read worked, you have to check the value. If the value is FF > then you have a huge hint that the hardware is now gone. And you can > rely on it being gone, you can never rely on making the call to the > function to check if the hardware is there to be still valid any point > in time after the call returns. > > > Surprise hot remove is empirically more reliable the less we interact > > with hardware and firmware. That shouldn't be necessary, but is just an > > unfortunate reality. > > You are not "interacting", you are reading/writing to the hardware, as > you have to do so. So I really don't understand what you are talking > about here, sorry. We're reading hardware memory, yes, but the hardware isn't there. Something obviously needs to return FF, so we are indirectly interacting with whatever mechanism handles that. Sometimes that mechanism doesn't handle it gracefully and instead of having FF to consider, you have a machine check rebooting your system.
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Thu, Nov 08, 2018 at 02:42:55PM -0800, Greg Kroah-Hartman wrote: > On Thu, Nov 08, 2018 at 03:32:58PM -0700, Keith Busch wrote: > > On Thu, Nov 08, 2018 at 02:01:17PM -0800, Greg Kroah-Hartman wrote: > > > On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote: > > > > I'm having second thoughts about this. One thing I'm uncomfortable > > > > with is that sprinkling pci_dev_is_disconnected() around feels ad hoc > > > > instead of systematic, in the sense that I don't know how we convince > > > > ourselves that this (and only this) is the correct place to put it. > > > > > > I think my stance always has been that this call is not good at all > > > because once you call it you never really know if it is still true as > > > the device could have been removed right afterward. > > > > > > So almost any code that relies on it is broken, there is no locking and > > > it can and will race and you will loose. > > > > AIUI, we're not trying to create code to rely on this. This more about > > reducing reliance on hardware. If the software misses the race once and > > accesses disconnected device memory, that's usually not a big deal to > > let hardware sort it out, but the point is not to push our luck. > > Then why even care about this call at all? If you need to really know > if the read worked, you have to check the value. If the value is FF > then you have a huge hint that the hardware is now gone. And you can > rely on it being gone, you can never rely on making the call to the > function to check if the hardware is there to be still valid any point > in time after the call returns. > > > Surprise hot remove is empirically more reliable the less we interact > > with hardware and firmware. That shouldn't be necessary, but is just an > > unfortunate reality. > > You are not "interacting", you are reading/writing to the hardware, as > you have to do so. So I really don't understand what you are talking > about here, sorry. We're reading hardware memory, yes, but the hardware isn't there. Something obviously needs to return FF, so we are indirectly interacting with whatever mechanism handles that. Sometimes that mechanism doesn't handle it gracefully and instead of having FF to consider, you have a machine check rebooting your system.
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Thu, Nov 08, 2018 at 03:32:58PM -0700, Keith Busch wrote: > On Thu, Nov 08, 2018 at 02:01:17PM -0800, Greg Kroah-Hartman wrote: > > On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote: > > > I'm having second thoughts about this. One thing I'm uncomfortable > > > with is that sprinkling pci_dev_is_disconnected() around feels ad hoc > > > instead of systematic, in the sense that I don't know how we convince > > > ourselves that this (and only this) is the correct place to put it. > > > > I think my stance always has been that this call is not good at all > > because once you call it you never really know if it is still true as > > the device could have been removed right afterward. > > > > So almost any code that relies on it is broken, there is no locking and > > it can and will race and you will loose. > > AIUI, we're not trying to create code to rely on this. This more about > reducing reliance on hardware. If the software misses the race once and > accesses disconnected device memory, that's usually not a big deal to > let hardware sort it out, but the point is not to push our luck. Then why even care about this call at all? If you need to really know if the read worked, you have to check the value. If the value is FF then you have a huge hint that the hardware is now gone. And you can rely on it being gone, you can never rely on making the call to the function to check if the hardware is there to be still valid any point in time after the call returns. > Surprise hot remove is empirically more reliable the less we interact > with hardware and firmware. That shouldn't be necessary, but is just an > unfortunate reality. You are not "interacting", you are reading/writing to the hardware, as you have to do so. So I really don't understand what you are talking about here, sorry. greg k-h
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Thu, Nov 08, 2018 at 03:32:58PM -0700, Keith Busch wrote: > On Thu, Nov 08, 2018 at 02:01:17PM -0800, Greg Kroah-Hartman wrote: > > On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote: > > > I'm having second thoughts about this. One thing I'm uncomfortable > > > with is that sprinkling pci_dev_is_disconnected() around feels ad hoc > > > instead of systematic, in the sense that I don't know how we convince > > > ourselves that this (and only this) is the correct place to put it. > > > > I think my stance always has been that this call is not good at all > > because once you call it you never really know if it is still true as > > the device could have been removed right afterward. > > > > So almost any code that relies on it is broken, there is no locking and > > it can and will race and you will loose. > > AIUI, we're not trying to create code to rely on this. This more about > reducing reliance on hardware. If the software misses the race once and > accesses disconnected device memory, that's usually not a big deal to > let hardware sort it out, but the point is not to push our luck. Then why even care about this call at all? If you need to really know if the read worked, you have to check the value. If the value is FF then you have a huge hint that the hardware is now gone. And you can rely on it being gone, you can never rely on making the call to the function to check if the hardware is there to be still valid any point in time after the call returns. > Surprise hot remove is empirically more reliable the less we interact > with hardware and firmware. That shouldn't be necessary, but is just an > unfortunate reality. You are not "interacting", you are reading/writing to the hardware, as you have to do so. So I really don't understand what you are talking about here, sorry. greg k-h
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Thu, Nov 08, 2018 at 02:01:17PM -0800, Greg Kroah-Hartman wrote: > On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote: > > I'm having second thoughts about this. One thing I'm uncomfortable > > with is that sprinkling pci_dev_is_disconnected() around feels ad hoc > > instead of systematic, in the sense that I don't know how we convince > > ourselves that this (and only this) is the correct place to put it. > > I think my stance always has been that this call is not good at all > because once you call it you never really know if it is still true as > the device could have been removed right afterward. > > So almost any code that relies on it is broken, there is no locking and > it can and will race and you will loose. AIUI, we're not trying to create code to rely on this. This more about reducing reliance on hardware. If the software misses the race once and accesses disconnected device memory, that's usually not a big deal to let hardware sort it out, but the point is not to push our luck. Surprise hot remove is empirically more reliable the less we interact with hardware and firmware. That shouldn't be necessary, but is just an unfortunate reality.
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Thu, Nov 08, 2018 at 02:01:17PM -0800, Greg Kroah-Hartman wrote: > On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote: > > I'm having second thoughts about this. One thing I'm uncomfortable > > with is that sprinkling pci_dev_is_disconnected() around feels ad hoc > > instead of systematic, in the sense that I don't know how we convince > > ourselves that this (and only this) is the correct place to put it. > > I think my stance always has been that this call is not good at all > because once you call it you never really know if it is still true as > the device could have been removed right afterward. > > So almost any code that relies on it is broken, there is no locking and > it can and will race and you will loose. AIUI, we're not trying to create code to rely on this. This more about reducing reliance on hardware. If the software misses the race once and accesses disconnected device memory, that's usually not a big deal to let hardware sort it out, but the point is not to push our luck. Surprise hot remove is empirically more reliable the less we interact with hardware and firmware. That shouldn't be necessary, but is just an unfortunate reality.
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On 11/08/2018 02:09 PM, Bjorn Helgaas wrote: > > [EXTERNAL EMAIL] > Please report any suspicious attachments, links, or requests for sensitive > information. > > > [+cc Jonathan, Greg, Lukas, Russell, Sam, Oliver for discussion about > PCI error recovery in general] Has anyone seen seen the ECRs in the PCIe base spec and ACPI that have been floating around the past few months? -- HPX, SFI, CER. Without divulging too much before publication, I'm curious on opinions on how well (or not well) those flows would work in general, and in linux. > On Wed, Nov 07, 2018 at 05:42:57PM -0600, Bjorn Helgaas wrote: > I'm having second thoughts about this. One thing I'm uncomfortable > with is that sprinkling pci_dev_is_disconnected() around feels ad hoc > instead of systematic, in the sense that I don't know how we convince > ourselves that this (and only this) is the correct place to put it. > > Another is that the only place we call pci_dev_set_disconnected() is > in pciehp and acpiphp, so the only "disconnected" case we catch is if > hotplug happens to be involved. Every MMIO read from the device is an > opportunity to learn whether it is reachable (a read from an > unreachable device typically returns ~0 data), but we don't do > anything at all with those. > > The config accessors already check pci_dev_is_disconnected(), so this > patch is really aimed at MMIO accesses. I think it would be more > robust if we added wrappers for readl() and writel() so we could > notice read errors and avoid future reads and writes. I wouldn't expect anything less than complete scrutiny and quality control of unquestionable moral integrity :). In theory ~0 can be a great indicator that something may be wrong. Though I think it's about as ad-hoc as pci_dev_is_disconnected(). I slightly like the idea of wrapping the MMIO accessors. There's still memcpy and DMA that cause the same MemRead/Wr PCIe transactions, and the same sort of errors in PCIe land, and it would be good to have more testing on this. Since this patch is tested and confirmed to fix a known failure case, I would keep it, and the look at fixing the problem in a more generic way. BTW, a lot of the problems we're fixing here come courtesy of firmware-first error handling. Do we reach a point where we draw a line in handling new problems introduced by FFS? So, if something is a problem with FFS, but not native handling, do we commit to supporting it? Alex
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On 11/08/2018 02:09 PM, Bjorn Helgaas wrote: > > [EXTERNAL EMAIL] > Please report any suspicious attachments, links, or requests for sensitive > information. > > > [+cc Jonathan, Greg, Lukas, Russell, Sam, Oliver for discussion about > PCI error recovery in general] Has anyone seen seen the ECRs in the PCIe base spec and ACPI that have been floating around the past few months? -- HPX, SFI, CER. Without divulging too much before publication, I'm curious on opinions on how well (or not well) those flows would work in general, and in linux. > On Wed, Nov 07, 2018 at 05:42:57PM -0600, Bjorn Helgaas wrote: > I'm having second thoughts about this. One thing I'm uncomfortable > with is that sprinkling pci_dev_is_disconnected() around feels ad hoc > instead of systematic, in the sense that I don't know how we convince > ourselves that this (and only this) is the correct place to put it. > > Another is that the only place we call pci_dev_set_disconnected() is > in pciehp and acpiphp, so the only "disconnected" case we catch is if > hotplug happens to be involved. Every MMIO read from the device is an > opportunity to learn whether it is reachable (a read from an > unreachable device typically returns ~0 data), but we don't do > anything at all with those. > > The config accessors already check pci_dev_is_disconnected(), so this > patch is really aimed at MMIO accesses. I think it would be more > robust if we added wrappers for readl() and writel() so we could > notice read errors and avoid future reads and writes. I wouldn't expect anything less than complete scrutiny and quality control of unquestionable moral integrity :). In theory ~0 can be a great indicator that something may be wrong. Though I think it's about as ad-hoc as pci_dev_is_disconnected(). I slightly like the idea of wrapping the MMIO accessors. There's still memcpy and DMA that cause the same MemRead/Wr PCIe transactions, and the same sort of errors in PCIe land, and it would be good to have more testing on this. Since this patch is tested and confirmed to fix a known failure case, I would keep it, and the look at fixing the problem in a more generic way. BTW, a lot of the problems we're fixing here come courtesy of firmware-first error handling. Do we reach a point where we draw a line in handling new problems introduced by FFS? So, if something is a problem with FFS, but not native handling, do we commit to supporting it? Alex
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote:
> [+cc Jonathan, Greg, Lukas, Russell, Sam, Oliver for discussion about
> PCI error recovery in general]
>
> On Wed, Nov 07, 2018 at 05:42:57PM -0600, Bjorn Helgaas wrote:
> > On Tue, Sep 18, 2018 at 05:15:00PM -0500, Alexandru Gagniuc wrote:
> > > When a PCI device is gone, we don't want to send IO to it if we can
> > > avoid it. We expose functionality via the irq_chip structure. As
> > > users of that structure may not know about the underlying PCI device,
> > > it's our responsibility to guard against removed devices.
> > >
> > > .irq_write_msi_msg() is already guarded inside __pci_write_msi_msg().
> > > .irq_mask/unmask() are not. Guard them for completeness.
> > >
> > > For example, surprise removal of a PCIe device triggers teardown. This
> > > touches the irq_chips ops some point to disable the interrupts. I/O
> > > generated here can crash the system on firmware-first machines.
> > > Not triggering the IO in the first place greatly reduces the
> > > possibility of the problem occurring.
> > >
> > > Signed-off-by: Alexandru Gagniuc
> >
> > Applied to pci/misc for v4.21, thanks!
>
> I'm having second thoughts about this. One thing I'm uncomfortable
> with is that sprinkling pci_dev_is_disconnected() around feels ad hoc
> instead of systematic, in the sense that I don't know how we convince
> ourselves that this (and only this) is the correct place to put it.
I think my stance always has been that this call is not good at all
because once you call it you never really know if it is still true as
the device could have been removed right afterward.
So almost any code that relies on it is broken, there is no locking and
it can and will race and you will loose.
I think your patch suffers from this race:
> +static u32 mmio_readl(struct pci_dev *dev, const volatile void __iomem *addr)
> +{
> + u32 val, id;
> +
> + if (pci_dev_is_disconnected(dev))
> + return ~0;
Great, but what happens if I yank the device out right here?
> + val = readl(addr);
This value could now be all FF, if the device is gone, so what did the
check above help with?
> + /*
> + * If an MMIO read from the device returns ~0 data, that data may
> + * be valid, or it may indicate a bus error. If config space is
> + * readable, assume it's valid data; otherwise, assume a bus error.
> + */
> + if (val == ~0) {
> + pci_read_config_dword(dev, PCI_VENDOR_ID, );
> + if (id == ~0)
> + pci_dev_set_disconnected(dev, NULL);
So why do the check above for "is disconnected"? What does this buy us
here, just short-circuiting the readl()?
> + }
> +
> + return val;
> +}
> +
> +static void mmio_writel(struct pci_dev *dev, u32 val,
> + volatile void __iomem *addr)
> +{
> + if (pci_dev_is_disconnected(dev))
> + return;
> +
> + writel(val, addr);
Why even check, what's wrong with always doing the write?
I understand the wish to make this easier, but I think the only way is
that the driver themselves should be checking on their reads. And they
have to check on all reads, or at least on some subset of reads and be
able to handle 0xff for the other ones without going crazy.
I _think_ the xhci driver does this given that it is hot added/removed
all the time dynamically due to the way that modern laptops are made
where the bios adds/removed the xhci controller when a USB device is
added/removed.
thanks,
greg k-h
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Thu, Nov 08, 2018 at 02:09:17PM -0600, Bjorn Helgaas wrote:
> [+cc Jonathan, Greg, Lukas, Russell, Sam, Oliver for discussion about
> PCI error recovery in general]
>
> On Wed, Nov 07, 2018 at 05:42:57PM -0600, Bjorn Helgaas wrote:
> > On Tue, Sep 18, 2018 at 05:15:00PM -0500, Alexandru Gagniuc wrote:
> > > When a PCI device is gone, we don't want to send IO to it if we can
> > > avoid it. We expose functionality via the irq_chip structure. As
> > > users of that structure may not know about the underlying PCI device,
> > > it's our responsibility to guard against removed devices.
> > >
> > > .irq_write_msi_msg() is already guarded inside __pci_write_msi_msg().
> > > .irq_mask/unmask() are not. Guard them for completeness.
> > >
> > > For example, surprise removal of a PCIe device triggers teardown. This
> > > touches the irq_chips ops some point to disable the interrupts. I/O
> > > generated here can crash the system on firmware-first machines.
> > > Not triggering the IO in the first place greatly reduces the
> > > possibility of the problem occurring.
> > >
> > > Signed-off-by: Alexandru Gagniuc
> >
> > Applied to pci/misc for v4.21, thanks!
>
> I'm having second thoughts about this. One thing I'm uncomfortable
> with is that sprinkling pci_dev_is_disconnected() around feels ad hoc
> instead of systematic, in the sense that I don't know how we convince
> ourselves that this (and only this) is the correct place to put it.
I think my stance always has been that this call is not good at all
because once you call it you never really know if it is still true as
the device could have been removed right afterward.
So almost any code that relies on it is broken, there is no locking and
it can and will race and you will loose.
I think your patch suffers from this race:
> +static u32 mmio_readl(struct pci_dev *dev, const volatile void __iomem *addr)
> +{
> + u32 val, id;
> +
> + if (pci_dev_is_disconnected(dev))
> + return ~0;
Great, but what happens if I yank the device out right here?
> + val = readl(addr);
This value could now be all FF, if the device is gone, so what did the
check above help with?
> + /*
> + * If an MMIO read from the device returns ~0 data, that data may
> + * be valid, or it may indicate a bus error. If config space is
> + * readable, assume it's valid data; otherwise, assume a bus error.
> + */
> + if (val == ~0) {
> + pci_read_config_dword(dev, PCI_VENDOR_ID, );
> + if (id == ~0)
> + pci_dev_set_disconnected(dev, NULL);
So why do the check above for "is disconnected"? What does this buy us
here, just short-circuiting the readl()?
> + }
> +
> + return val;
> +}
> +
> +static void mmio_writel(struct pci_dev *dev, u32 val,
> + volatile void __iomem *addr)
> +{
> + if (pci_dev_is_disconnected(dev))
> + return;
> +
> + writel(val, addr);
Why even check, what's wrong with always doing the write?
I understand the wish to make this easier, but I think the only way is
that the driver themselves should be checking on their reads. And they
have to check on all reads, or at least on some subset of reads and be
able to handle 0xff for the other ones without going crazy.
I _think_ the xhci driver does this given that it is hot added/removed
all the time dynamically due to the way that modern laptops are made
where the bios adds/removed the xhci controller when a USB device is
added/removed.
thanks,
greg k-h
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Tue, Sep 18, 2018 at 05:15:00PM -0500, Alexandru Gagniuc wrote:
> When a PCI device is gone, we don't want to send IO to it if we can
> avoid it. We expose functionality via the irq_chip structure. As
> users of that structure may not know about the underlying PCI device,
> it's our responsibility to guard against removed devices.
>
> .irq_write_msi_msg() is already guarded inside __pci_write_msi_msg().
> .irq_mask/unmask() are not. Guard them for completeness.
>
> For example, surprise removal of a PCIe device triggers teardown. This
> touches the irq_chips ops some point to disable the interrupts. I/O
> generated here can crash the system on firmware-first machines.
> Not triggering the IO in the first place greatly reduces the
> possibility of the problem occurring.
>
> Signed-off-by: Alexandru Gagniuc
Applied to pci/misc for v4.21, thanks!
> ---
> drivers/pci/msi.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index f2ef896464b3..f31058fd2260 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -227,6 +227,9 @@ static void msi_set_mask_bit(struct irq_data *data, u32
> flag)
> {
> struct msi_desc *desc = irq_data_get_msi_desc(data);
>
> + if (pci_dev_is_disconnected(msi_desc_to_pci_dev(desc)))
> + return;
> +
> if (desc->msi_attrib.is_msix) {
> msix_mask_irq(desc, flag);
> readl(desc->mask_base); /* Flush write to device */
> --
> 2.17.1
>
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
On Tue, Sep 18, 2018 at 05:15:00PM -0500, Alexandru Gagniuc wrote:
> When a PCI device is gone, we don't want to send IO to it if we can
> avoid it. We expose functionality via the irq_chip structure. As
> users of that structure may not know about the underlying PCI device,
> it's our responsibility to guard against removed devices.
>
> .irq_write_msi_msg() is already guarded inside __pci_write_msi_msg().
> .irq_mask/unmask() are not. Guard them for completeness.
>
> For example, surprise removal of a PCIe device triggers teardown. This
> touches the irq_chips ops some point to disable the interrupts. I/O
> generated here can crash the system on firmware-first machines.
> Not triggering the IO in the first place greatly reduces the
> possibility of the problem occurring.
>
> Signed-off-by: Alexandru Gagniuc
Applied to pci/misc for v4.21, thanks!
> ---
> drivers/pci/msi.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index f2ef896464b3..f31058fd2260 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -227,6 +227,9 @@ static void msi_set_mask_bit(struct irq_data *data, u32
> flag)
> {
> struct msi_desc *desc = irq_data_get_msi_desc(data);
>
> + if (pci_dev_is_disconnected(msi_desc_to_pci_dev(desc)))
> + return;
> +
> if (desc->msi_attrib.is_msix) {
> msix_mask_irq(desc, flag);
> readl(desc->mask_base); /* Flush write to device */
> --
> 2.17.1
>
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
I found the same issue:
https://patchwork.ozlabs.org/patch/989272/
Tested-by: Jon Derrick
On Mon, 2018-11-05 at 18:32 -0600, Alex G. wrote:
> ping
>
> On 09/18/2018 05:15 PM, Alexandru Gagniuc wrote:
> > When a PCI device is gone, we don't want to send IO to it if we can
> > avoid it. We expose functionality via the irq_chip structure. As
> > users of that structure may not know about the underlying PCI
> > device,
> > it's our responsibility to guard against removed devices.
> >
> > .irq_write_msi_msg() is already guarded inside
> > __pci_write_msi_msg().
> > .irq_mask/unmask() are not. Guard them for completeness.
> >
> > For example, surprise removal of a PCIe device triggers teardown.
> > This
> > touches the irq_chips ops some point to disable the interrupts. I/O
> > generated here can crash the system on firmware-first machines.
> > Not triggering the IO in the first place greatly reduces the
> > possibility of the problem occurring.
> >
> > Signed-off-by: Alexandru Gagniuc
> > ---
> > drivers/pci/msi.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index f2ef896464b3..f31058fd2260 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -227,6 +227,9 @@ static void msi_set_mask_bit(struct irq_data
> > *data, u32 flag)
> > {
> > struct msi_desc *desc = irq_data_get_msi_desc(data);
> >
> > + if (pci_dev_is_disconnected(msi_desc_to_pci_dev(desc)))
> > + return;
> > +
> > if (desc->msi_attrib.is_msix) {
> > msix_mask_irq(desc, flag);
> > readl(desc->mask_base); /* Flush
> > write to device */
> >
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
I found the same issue:
https://patchwork.ozlabs.org/patch/989272/
Tested-by: Jon Derrick
On Mon, 2018-11-05 at 18:32 -0600, Alex G. wrote:
> ping
>
> On 09/18/2018 05:15 PM, Alexandru Gagniuc wrote:
> > When a PCI device is gone, we don't want to send IO to it if we can
> > avoid it. We expose functionality via the irq_chip structure. As
> > users of that structure may not know about the underlying PCI
> > device,
> > it's our responsibility to guard against removed devices.
> >
> > .irq_write_msi_msg() is already guarded inside
> > __pci_write_msi_msg().
> > .irq_mask/unmask() are not. Guard them for completeness.
> >
> > For example, surprise removal of a PCIe device triggers teardown.
> > This
> > touches the irq_chips ops some point to disable the interrupts. I/O
> > generated here can crash the system on firmware-first machines.
> > Not triggering the IO in the first place greatly reduces the
> > possibility of the problem occurring.
> >
> > Signed-off-by: Alexandru Gagniuc
> > ---
> > drivers/pci/msi.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index f2ef896464b3..f31058fd2260 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -227,6 +227,9 @@ static void msi_set_mask_bit(struct irq_data
> > *data, u32 flag)
> > {
> > struct msi_desc *desc = irq_data_get_msi_desc(data);
> >
> > + if (pci_dev_is_disconnected(msi_desc_to_pci_dev(desc)))
> > + return;
> > +
> > if (desc->msi_attrib.is_msix) {
> > msix_mask_irq(desc, flag);
> > readl(desc->mask_base); /* Flush
> > write to device */
> >
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
ping
On 09/18/2018 05:15 PM, Alexandru Gagniuc wrote:
When a PCI device is gone, we don't want to send IO to it if we can
avoid it. We expose functionality via the irq_chip structure. As
users of that structure may not know about the underlying PCI device,
it's our responsibility to guard against removed devices.
.irq_write_msi_msg() is already guarded inside __pci_write_msi_msg().
.irq_mask/unmask() are not. Guard them for completeness.
For example, surprise removal of a PCIe device triggers teardown. This
touches the irq_chips ops some point to disable the interrupts. I/O
generated here can crash the system on firmware-first machines.
Not triggering the IO in the first place greatly reduces the
possibility of the problem occurring.
Signed-off-by: Alexandru Gagniuc
---
drivers/pci/msi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index f2ef896464b3..f31058fd2260 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -227,6 +227,9 @@ static void msi_set_mask_bit(struct irq_data *data, u32
flag)
{
struct msi_desc *desc = irq_data_get_msi_desc(data);
+ if (pci_dev_is_disconnected(msi_desc_to_pci_dev(desc)))
+ return;
+
if (desc->msi_attrib.is_msix) {
msix_mask_irq(desc, flag);
readl(desc->mask_base); /* Flush write to device */
Re: [PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
ping
On 09/18/2018 05:15 PM, Alexandru Gagniuc wrote:
When a PCI device is gone, we don't want to send IO to it if we can
avoid it. We expose functionality via the irq_chip structure. As
users of that structure may not know about the underlying PCI device,
it's our responsibility to guard against removed devices.
.irq_write_msi_msg() is already guarded inside __pci_write_msi_msg().
.irq_mask/unmask() are not. Guard them for completeness.
For example, surprise removal of a PCIe device triggers teardown. This
touches the irq_chips ops some point to disable the interrupts. I/O
generated here can crash the system on firmware-first machines.
Not triggering the IO in the first place greatly reduces the
possibility of the problem occurring.
Signed-off-by: Alexandru Gagniuc
---
drivers/pci/msi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index f2ef896464b3..f31058fd2260 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -227,6 +227,9 @@ static void msi_set_mask_bit(struct irq_data *data, u32
flag)
{
struct msi_desc *desc = irq_data_get_msi_desc(data);
+ if (pci_dev_is_disconnected(msi_desc_to_pci_dev(desc)))
+ return;
+
if (desc->msi_attrib.is_msix) {
msix_mask_irq(desc, flag);
readl(desc->mask_base); /* Flush write to device */
[PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
When a PCI device is gone, we don't want to send IO to it if we can
avoid it. We expose functionality via the irq_chip structure. As
users of that structure may not know about the underlying PCI device,
it's our responsibility to guard against removed devices.
.irq_write_msi_msg() is already guarded inside __pci_write_msi_msg().
.irq_mask/unmask() are not. Guard them for completeness.
For example, surprise removal of a PCIe device triggers teardown. This
touches the irq_chips ops some point to disable the interrupts. I/O
generated here can crash the system on firmware-first machines.
Not triggering the IO in the first place greatly reduces the
possibility of the problem occurring.
Signed-off-by: Alexandru Gagniuc
---
drivers/pci/msi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index f2ef896464b3..f31058fd2260 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -227,6 +227,9 @@ static void msi_set_mask_bit(struct irq_data *data, u32
flag)
{
struct msi_desc *desc = irq_data_get_msi_desc(data);
+ if (pci_dev_is_disconnected(msi_desc_to_pci_dev(desc)))
+ return;
+
if (desc->msi_attrib.is_msix) {
msix_mask_irq(desc, flag);
readl(desc->mask_base); /* Flush write to device */
--
2.17.1
[PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected
When a PCI device is gone, we don't want to send IO to it if we can
avoid it. We expose functionality via the irq_chip structure. As
users of that structure may not know about the underlying PCI device,
it's our responsibility to guard against removed devices.
.irq_write_msi_msg() is already guarded inside __pci_write_msi_msg().
.irq_mask/unmask() are not. Guard them for completeness.
For example, surprise removal of a PCIe device triggers teardown. This
touches the irq_chips ops some point to disable the interrupts. I/O
generated here can crash the system on firmware-first machines.
Not triggering the IO in the first place greatly reduces the
possibility of the problem occurring.
Signed-off-by: Alexandru Gagniuc
---
drivers/pci/msi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index f2ef896464b3..f31058fd2260 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -227,6 +227,9 @@ static void msi_set_mask_bit(struct irq_data *data, u32
flag)
{
struct msi_desc *desc = irq_data_get_msi_desc(data);
+ if (pci_dev_is_disconnected(msi_desc_to_pci_dev(desc)))
+ return;
+
if (desc->msi_attrib.is_msix) {
msix_mask_irq(desc, flag);
readl(desc->mask_base); /* Flush write to device */
--
2.17.1
