| Message ID | 155605909349.3575.13433421148215616375.stgit@gimli.home (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Add link_change error handler and vfio-pci user | expand |
On 4/23/2019 5:42 PM, Alex Williamson wrote: > The PCIe bandwidth notification service generates logging any time a > link changes speed or width to a state that is considered downgraded. > Unfortunately, it cannot differentiate signal integrity related link > changes from those intentionally initiated by an endpoint driver, > including drivers that may live in userspace or VMs when making use > of vfio-pci. Therefore, allow the driver to have a say in whether > the link is indeed downgraded and worth noting in the log, or if the > change is perhaps intentional. > > For vfio-pci, we don't know the intentions of the user/guest driver > either, but we do know that GPU drivers in guests actively manage > the link state and therefore trigger the bandwidth notification for > what appear to be entirely intentional link changes. > > Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification > Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > > Changing to pci_dbg() logging is not super usable, so let's try the > previous idea of letting the driver handle link change events as they > see fit. Ideally this might be two patches, but for easier handling, > folding the pci and vfio-pci bits together. Comments? Thanks, I think this callback opens up a can of worms where drivers can ad-hoc kill a number what otherwise can be indicators of problems. But I don't have to like it to review it :). > drivers/pci/probe.c | 13 +++++++++++++ > drivers/vfio/pci/vfio_pci.c | 10 ++++++++++ > include/linux/pci.h | 3 +++ > 3 files changed, 26 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 7e12d0163863..233cd4b5b6e8 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev) I don't think you want to change pcie_report_downtraining(). You're advertising to "report" something, by nomenclature, but then go around and also call a notification callback. This is also used during probe, and you've now just killed your chance to notice you've booted with a degraded link. If what you want to do is silence the bandwidth notification, you want to modify the threaded interrupt that calls this. > if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn) > return; > > + /* > + * If driver handles link_change event, defer to driver. PCIe drivers > + * can call pcie_print_link_status() to print current link info. > + */ > + device_lock(&dev->dev); > + if (dev->driver && dev->driver->err_handler && > + dev->driver->err_handler->link_change) { > + dev->driver->err_handler->link_change(dev); > + device_unlock(&dev->dev); > + return; > + } > + device_unlock(&dev->dev); Can we write this such that there is a single lock()/unlock() pair? > + > /* Print link status only if the device is constrained by the fabric */ > __pcie_print_link_status(dev, false); > } > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index cab71da46f4a..c9ffc0ccabb3 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -1418,8 +1418,18 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, > return PCI_ERS_RESULT_CAN_RECOVER; > } > > +/* > + * Ignore link change notification, we can't differentiate signal related > + * link changes from user driver power management type operations, so do > + * nothing. Potentially this could be routed out to the user. > + */ > +static void vfio_pci_link_change(struct pci_dev *pdev) > +{ > +} > + > static const struct pci_error_handlers vfio_err_handlers = { > .error_detected = vfio_pci_aer_err_detected, > + .link_change = vfio_pci_link_change, > }; > > static struct pci_driver vfio_pci_driver = { > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 27854731afc4..e9194bc03f9e 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -763,6 +763,9 @@ struct pci_error_handlers { > > /* Device driver may resume normal operations */ > void (*resume)(struct pci_dev *dev); > + > + /* PCIe link change notification */ > + void (*link_change)(struct pci_dev *dev); > }; > > > >
On Wed, 24 Apr 2019 16:45:45 +0000 <Alex_Gagniuc@Dellteam.com> wrote: > On 4/23/2019 5:42 PM, Alex Williamson wrote: > > The PCIe bandwidth notification service generates logging any time a > > link changes speed or width to a state that is considered downgraded. > > Unfortunately, it cannot differentiate signal integrity related link > > changes from those intentionally initiated by an endpoint driver, > > including drivers that may live in userspace or VMs when making use > > of vfio-pci. Therefore, allow the driver to have a say in whether > > the link is indeed downgraded and worth noting in the log, or if the > > change is perhaps intentional. > > > > For vfio-pci, we don't know the intentions of the user/guest driver > > either, but we do know that GPU drivers in guests actively manage > > the link state and therefore trigger the bandwidth notification for > > what appear to be entirely intentional link changes. > > > > Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification > > Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > > > Changing to pci_dbg() logging is not super usable, so let's try the > > previous idea of letting the driver handle link change events as they > > see fit. Ideally this might be two patches, but for easier handling, > > folding the pci and vfio-pci bits together. Comments? Thanks, > > I think this callback opens up a can of worms where drivers can ad-hoc > kill a number what otherwise can be indicators of problems. But I don't > have to like it to review it :). > > > drivers/pci/probe.c | 13 +++++++++++++ > > drivers/vfio/pci/vfio_pci.c | 10 ++++++++++ > > include/linux/pci.h | 3 +++ > > 3 files changed, 26 insertions(+) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 7e12d0163863..233cd4b5b6e8 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev) > > I don't think you want to change pcie_report_downtraining(). You're > advertising to "report" something, by nomenclature, but then go around > and also call a notification callback. This is also used during probe, > and you've now just killed your chance to notice you've booted with a > degraded link. > If what you want to do is silence the bandwidth notification, you want > to modify the threaded interrupt that calls this. During probe, ie. discovery, a device wouldn't have a driver attached, so we'd fall through to simply printing the link status. Nothing lost afaict. The "report" verb doesn't have a subject here, report to whom? Therefore I thought it reasonable that a driver ask that it be reported to them via a callback. I don't see that as such a stretch of the interface. > > if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn) > > return; > > > > + /* > > + * If driver handles link_change event, defer to driver. PCIe drivers > > + * can call pcie_print_link_status() to print current link info. > > + */ > > + device_lock(&dev->dev); > > + if (dev->driver && dev->driver->err_handler && > > + dev->driver->err_handler->link_change) { > > + dev->driver->err_handler->link_change(dev); > > + device_unlock(&dev->dev); > > + return; > > + } > > + device_unlock(&dev->dev); > > Can we write this such that there is a single lock()/unlock() pair? Not without introducing a tracking variable, ex. bool handled = false; lock() if (stuff) { link_change() handled = true; } unlock() if (!handled) dmesg spew That's not markedly better imo, but if it's preferred I can send a v2. Thanks, Alex > > + > > /* Print link status only if the device is constrained by the fabric */ > > __pcie_print_link_status(dev, false); > > } > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > > index cab71da46f4a..c9ffc0ccabb3 100644 > > --- a/drivers/vfio/pci/vfio_pci.c > > +++ b/drivers/vfio/pci/vfio_pci.c > > @@ -1418,8 +1418,18 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, > > return PCI_ERS_RESULT_CAN_RECOVER; > > } > > > > +/* > > + * Ignore link change notification, we can't differentiate signal related > > + * link changes from user driver power management type operations, so do > > + * nothing. Potentially this could be routed out to the user. > > + */ > > +static void vfio_pci_link_change(struct pci_dev *pdev) > > +{ > > +} > > + > > static const struct pci_error_handlers vfio_err_handlers = { > > .error_detected = vfio_pci_aer_err_detected, > > + .link_change = vfio_pci_link_change, > > }; > > > > static struct pci_driver vfio_pci_driver = { > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index 27854731afc4..e9194bc03f9e 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -763,6 +763,9 @@ struct pci_error_handlers { > > > > /* Device driver may resume normal operations */ > > void (*resume)(struct pci_dev *dev); > > + > > + /* PCIe link change notification */ > > + void (*link_change)(struct pci_dev *dev); > > }; > > > > > > > > > >
On 4/24/19 12:19 PM, Alex Williamson wrote: > On Wed, 24 Apr 2019 16:45:45 +0000 > <Alex_Gagniuc@Dellteam.com> wrote: >> On 4/23/2019 5:42 PM, Alex Williamson wrote: >>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>> index 7e12d0163863..233cd4b5b6e8 100644 >>> --- a/drivers/pci/probe.c >>> +++ b/drivers/pci/probe.c >>> @@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev) >> >> I don't think you want to change pcie_report_downtraining(). You're >> advertising to "report" something, by nomenclature, but then go around >> and also call a notification callback. This is also used during probe, >> and you've now just killed your chance to notice you've booted with a >> degraded link. >> If what you want to do is silence the bandwidth notification, you want >> to modify the threaded interrupt that calls this. > > During probe, ie. discovery, a device wouldn't have a driver attached, > so we'd fall through to simply printing the link status. Nothing > lost afaict. The "report" verb doesn't have a subject here, report to > whom? Therefore I thought it reasonable that a driver ask that it be > reported to them via a callback. I don't see that as such a stretch of > the interface. That's just stretching the logic, and IMO makes the intent harder to understand. The argument relies on the state of the PCI device and logic, which is not obvious to the casual observer. If you want to bypass the bandwidth notification, then bypass the notification. >>> if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn) >>> return; >>> >>> + /* >>> + * If driver handles link_change event, defer to driver. PCIe drivers >>> + * can call pcie_print_link_status() to print current link info. >>> + */ >>> + device_lock(&dev->dev); >>> + if (dev->driver && dev->driver->err_handler && >>> + dev->driver->err_handler->link_change) { >>> + dev->driver->err_handler->link_change(dev); >>> + device_unlock(&dev->dev); >>> + return; >>> + } >>> + device_unlock(&dev->dev); >> >> Can we write this such that there is a single lock()/unlock() pair? > > Not without introducing a tracking variable, ex. [snip bad code] > That's not markedly better imo, but if it's preferred I can send a v2. How about: if (!invoke_link_changed_handler(pdev)) very_useful_downtraining_message(pdev); > Alex Alex
On Tue, Apr 23, 2019 at 04:42:28PM -0600, Alex Williamson wrote: > The PCIe bandwidth notification service generates logging any time a > link changes speed or width to a state that is considered downgraded. > Unfortunately, it cannot differentiate signal integrity related link > changes from those intentionally initiated by an endpoint driver, > including drivers that may live in userspace or VMs when making use > of vfio-pci. Therefore, allow the driver to have a say in whether > the link is indeed downgraded and worth noting in the log, or if the > change is perhaps intentional. > > For vfio-pci, we don't know the intentions of the user/guest driver > either, but we do know that GPU drivers in guests actively manage > the link state and therefore trigger the bandwidth notification for > what appear to be entirely intentional link changes. > > Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification > Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > > Changing to pci_dbg() logging is not super usable, so let's try the > previous idea of letting the driver handle link change events as they > see fit. Ideally this might be two patches, but for easier handling, > folding the pci and vfio-pci bits together. Comments? Thanks, I'm a little uneasy about the bandwidth notification logging as a whole. Messages in dmesg don't seem like a solid base for building management tools. I assume the eventual goal would be some sort of digested notification along the lines of "hey mr/ms administrator, the link to device X unexpectedly became slower, you might want to check that out." If I were building that, I don't think I would use dmesg. I might write a daemon that polls /sys/.../current_link_{speed,width}, or maybe use some sort of netlink event. Maybe it would be useful to have the admin designate devices of interest. I'm hesitant about adding a .link_change() handler. If there's something useful a driver could do with it, that's one thing. But using it merely to suppress a message doesn't really seem worth the trouble, and it seems unfriendly to ask drivers to add it when they didn't ask for it and get no benefit from it. > drivers/pci/probe.c | 13 +++++++++++++ > drivers/vfio/pci/vfio_pci.c | 10 ++++++++++ > include/linux/pci.h | 3 +++ > 3 files changed, 26 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 7e12d0163863..233cd4b5b6e8 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev) > if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn) > return; > > + /* > + * If driver handles link_change event, defer to driver. PCIe drivers > + * can call pcie_print_link_status() to print current link info. > + */ > + device_lock(&dev->dev); > + if (dev->driver && dev->driver->err_handler && > + dev->driver->err_handler->link_change) { > + dev->driver->err_handler->link_change(dev); > + device_unlock(&dev->dev); > + return; > + } > + device_unlock(&dev->dev); > + > /* Print link status only if the device is constrained by the fabric */ > __pcie_print_link_status(dev, false); > } > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index cab71da46f4a..c9ffc0ccabb3 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -1418,8 +1418,18 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, > return PCI_ERS_RESULT_CAN_RECOVER; > } > > +/* > + * Ignore link change notification, we can't differentiate signal related > + * link changes from user driver power management type operations, so do > + * nothing. Potentially this could be routed out to the user. > + */ > +static void vfio_pci_link_change(struct pci_dev *pdev) > +{ > +} > + > static const struct pci_error_handlers vfio_err_handlers = { > .error_detected = vfio_pci_aer_err_detected, > + .link_change = vfio_pci_link_change, > }; > > static struct pci_driver vfio_pci_driver = { > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 27854731afc4..e9194bc03f9e 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -763,6 +763,9 @@ struct pci_error_handlers { > > /* Device driver may resume normal operations */ > void (*resume)(struct pci_dev *dev); > + > + /* PCIe link change notification */ > + void (*link_change)(struct pci_dev *dev); > }; > > >
On Wed, 24 Apr 2019 12:57:58 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote: > On Tue, Apr 23, 2019 at 04:42:28PM -0600, Alex Williamson wrote: > > The PCIe bandwidth notification service generates logging any time a > > link changes speed or width to a state that is considered downgraded. > > Unfortunately, it cannot differentiate signal integrity related link > > changes from those intentionally initiated by an endpoint driver, > > including drivers that may live in userspace or VMs when making use > > of vfio-pci. Therefore, allow the driver to have a say in whether > > the link is indeed downgraded and worth noting in the log, or if the > > change is perhaps intentional. > > > > For vfio-pci, we don't know the intentions of the user/guest driver > > either, but we do know that GPU drivers in guests actively manage > > the link state and therefore trigger the bandwidth notification for > > what appear to be entirely intentional link changes. > > > > Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification > > Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > > > Changing to pci_dbg() logging is not super usable, so let's try the > > previous idea of letting the driver handle link change events as they > > see fit. Ideally this might be two patches, but for easier handling, > > folding the pci and vfio-pci bits together. Comments? Thanks, > > I'm a little uneasy about the bandwidth notification logging as a > whole. Messages in dmesg don't seem like a solid base for building > management tools. > > I assume the eventual goal would be some sort of digested notification > along the lines of "hey mr/ms administrator, the link to device X > unexpectedly became slower, you might want to check that out." > > If I were building that, I don't think I would use dmesg. I might > write a daemon that polls /sys/.../current_link_{speed,width}, or > maybe use some sort of netlink event. Maybe it would be useful to > have the admin designate devices of interest. > > I'm hesitant about adding a .link_change() handler. If there's > something useful a driver could do with it, that's one thing. But > using it merely to suppress a message doesn't really seem worth the > trouble, and it seems unfriendly to ask drivers to add it when they > didn't ask for it and get no benefit from it. So where do we go from here? I agree that dmesg is not necessarily a great choice for these sorts of events and if they went somewhere else, maybe I wouldn't have the same concerns about them generating user confusion or contributing to DoS vectors from userspace drivers. As it is though, we have known cases where benign events generate confusing logging messages, which seems like a regression. Drivers didn't ask for a link_change handler, but nor did they ask that the link state to their device be monitored so closely. Maybe this not only needs some sort of change to the logging mechanism, but also an opt-in by the driver if they don't expect runtime link changes. Thanks, Alex
On 4/29/2019 10:51 AM, Alex Williamson wrote: > So where do we go from here? I agree that dmesg is not necessarily a > great choice for these sorts of events and if they went somewhere else, > maybe I wouldn't have the same concerns about them generating user > confusion or contributing to DoS vectors from userspace drivers. As it > is though, we have known cases where benign events generate confusing > logging messages, which seems like a regression. Drivers didn't ask > for a link_change handler, but nor did they ask that the link state to > their device be monitored so closely. Maybe this not only needs some > sort of change to the logging mechanism, but also an opt-in by the > driver if they don't expect runtime link changes. Thanks, Is there anyway to detect autonomous hardware management support and not report link state changes in that situation? I thought there were some capability bits for these.
On Mon, 29 Apr 2019 09:45:28 -0700 Sinan Kaya <Okaya@kernel.org> wrote: > On 4/29/2019 10:51 AM, Alex Williamson wrote: > > So where do we go from here? I agree that dmesg is not necessarily a > > great choice for these sorts of events and if they went somewhere else, > > maybe I wouldn't have the same concerns about them generating user > > confusion or contributing to DoS vectors from userspace drivers. As it > > is though, we have known cases where benign events generate confusing > > logging messages, which seems like a regression. Drivers didn't ask > > for a link_change handler, but nor did they ask that the link state to > > their device be monitored so closely. Maybe this not only needs some > > sort of change to the logging mechanism, but also an opt-in by the > > driver if they don't expect runtime link changes. Thanks, > > Is there anyway to detect autonomous hardware management support and > not report link state changes in that situation? > > I thought there were some capability bits for these. Not that we can find, this doesn't trigger the separate autonomous bandwidth notification interrupt. Thanks, Alex
On Mon, Apr 29, 2019 at 08:51:04AM -0600, Alex Williamson wrote: > On Wed, 24 Apr 2019 12:57:58 -0500 > Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Tue, Apr 23, 2019 at 04:42:28PM -0600, Alex Williamson wrote: > > > The PCIe bandwidth notification service generates logging any time a > > > link changes speed or width to a state that is considered downgraded. > > > Unfortunately, it cannot differentiate signal integrity related link > > > changes from those intentionally initiated by an endpoint driver, > > > including drivers that may live in userspace or VMs when making use > > > of vfio-pci. Therefore, allow the driver to have a say in whether > > > the link is indeed downgraded and worth noting in the log, or if the > > > change is perhaps intentional. > > > > > > For vfio-pci, we don't know the intentions of the user/guest driver > > > either, but we do know that GPU drivers in guests actively manage > > > the link state and therefore trigger the bandwidth notification for > > > what appear to be entirely intentional link changes. > > > > > > Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification > > > Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > --- > > > > > > Changing to pci_dbg() logging is not super usable, so let's try the > > > previous idea of letting the driver handle link change events as they > > > see fit. Ideally this might be two patches, but for easier handling, > > > folding the pci and vfio-pci bits together. Comments? Thanks, > > > > I'm a little uneasy about the bandwidth notification logging as a > > whole. Messages in dmesg don't seem like a solid base for building > > management tools. > > > > I assume the eventual goal would be some sort of digested notification > > along the lines of "hey mr/ms administrator, the link to device X > > unexpectedly became slower, you might want to check that out." > > > > If I were building that, I don't think I would use dmesg. I might > > write a daemon that polls /sys/.../current_link_{speed,width}, or > > maybe use some sort of netlink event. Maybe it would be useful to > > have the admin designate devices of interest. > > > > I'm hesitant about adding a .link_change() handler. If there's > > something useful a driver could do with it, that's one thing. But > > using it merely to suppress a message doesn't really seem worth the > > trouble, and it seems unfriendly to ask drivers to add it when they > > didn't ask for it and get no benefit from it. > > So where do we go from here? I agree that dmesg is not necessarily a > great choice for these sorts of events and if they went somewhere else, > maybe I wouldn't have the same concerns about them generating user > confusion or contributing to DoS vectors from userspace drivers. As it > is though, we have known cases where benign events generate confusing > logging messages, which seems like a regression. Drivers didn't ask > for a link_change handler, but nor did they ask that the link state to > their device be monitored so closely. Maybe this not only needs some > sort of change to the logging mechanism, but also an opt-in by the > driver if they don't expect runtime link changes. Thanks, I think it's really too late in the cycle to rework this and get changes merged before the v5.1 release (probably on May 5), so I'll queue up a revert and we can iron out the wrinkles for v5.2. Bjorn
On Mon, Apr 29, 2019 at 10:59:26AM -0600, Alex Williamson wrote: > On Mon, 29 Apr 2019 09:45:28 -0700 > Sinan Kaya <Okaya@kernel.org> wrote: > > > On 4/29/2019 10:51 AM, Alex Williamson wrote: > > > So where do we go from here? I agree that dmesg is not necessarily a > > > great choice for these sorts of events and if they went somewhere else, > > > maybe I wouldn't have the same concerns about them generating user > > > confusion or contributing to DoS vectors from userspace drivers. As it > > > is though, we have known cases where benign events generate confusing > > > logging messages, which seems like a regression. Drivers didn't ask > > > for a link_change handler, but nor did they ask that the link state to > > > their device be monitored so closely. Maybe this not only needs some > > > sort of change to the logging mechanism, but also an opt-in by the > > > driver if they don't expect runtime link changes. Thanks, > > > > Is there anyway to detect autonomous hardware management support and > > not report link state changes in that situation? > > > > I thought there were some capability bits for these. > > Not that we can find, this doesn't trigger the separate autonomous > bandwidth notification interrupt. Thanks, I think the only control is to disable automomous lane and link rate changes. When set, any changes to either should only be in response to errors, so enabling those controls might be the right thing to do with this feature.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 7e12d0163863..233cd4b5b6e8 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev) if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn) return; + /* + * If driver handles link_change event, defer to driver. PCIe drivers + * can call pcie_print_link_status() to print current link info. + */ + device_lock(&dev->dev); + if (dev->driver && dev->driver->err_handler && + dev->driver->err_handler->link_change) { + dev->driver->err_handler->link_change(dev); + device_unlock(&dev->dev); + return; + } + device_unlock(&dev->dev); + /* Print link status only if the device is constrained by the fabric */ __pcie_print_link_status(dev, false); } diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index cab71da46f4a..c9ffc0ccabb3 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -1418,8 +1418,18 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, return PCI_ERS_RESULT_CAN_RECOVER; } +/* + * Ignore link change notification, we can't differentiate signal related + * link changes from user driver power management type operations, so do + * nothing. Potentially this could be routed out to the user. + */ +static void vfio_pci_link_change(struct pci_dev *pdev) +{ +} + static const struct pci_error_handlers vfio_err_handlers = { .error_detected = vfio_pci_aer_err_detected, + .link_change = vfio_pci_link_change, }; static struct pci_driver vfio_pci_driver = { diff --git a/include/linux/pci.h b/include/linux/pci.h index 27854731afc4..e9194bc03f9e 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -763,6 +763,9 @@ struct pci_error_handlers { /* Device driver may resume normal operations */ void (*resume)(struct pci_dev *dev); + + /* PCIe link change notification */ + void (*link_change)(struct pci_dev *dev); };
The PCIe bandwidth notification service generates logging any time a link changes speed or width to a state that is considered downgraded. Unfortunately, it cannot differentiate signal integrity related link changes from those intentionally initiated by an endpoint driver, including drivers that may live in userspace or VMs when making use of vfio-pci. Therefore, allow the driver to have a say in whether the link is indeed downgraded and worth noting in the log, or if the change is perhaps intentional. For vfio-pci, we don't know the intentions of the user/guest driver either, but we do know that GPU drivers in guests actively manage the link state and therefore trigger the bandwidth notification for what appear to be entirely intentional link changes. Fixes: e8303bb7a75c PCI/LINK: Report degraded links via link bandwidth notification Link: https://lore.kernel.org/linux-pci/155597243666.19387.1205950870601742062.stgit@gimli.home/T/#u Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- Changing to pci_dbg() logging is not super usable, so let's try the previous idea of letting the driver handle link change events as they see fit. Ideally this might be two patches, but for easier handling, folding the pci and vfio-pci bits together. Comments? Thanks, Alex drivers/pci/probe.c | 13 +++++++++++++ drivers/vfio/pci/vfio_pci.c | 10 ++++++++++ include/linux/pci.h | 3 +++ 3 files changed, 26 insertions(+)