* [PATCH 1/2] PCI/LINK: bw_notification: Clear interrupt before enabling it
2019-03-20 11:05 [PATCH 0/2] Link bandwidth notification fixes Lukas Wunner
@ 2019-03-20 11:05 ` Lukas Wunner
2019-03-25 18:50 ` Alex_Gagniuc
2019-03-20 11:05 ` [PATCH 2/2] PCI/LINK: bw_notification: Deduplicate reports for multi-function devices Lukas Wunner
2019-03-25 22:33 ` [PATCH 0/2] Link bandwidth notification fixes Bjorn Helgaas
2 siblings, 1 reply; 6+ messages in thread
From: Lukas Wunner @ 2019-03-20 11:05 UTC (permalink / raw)
To: Bjorn Helgaas, Alexandru Gagniuc
Cc: linux-pci, Austin Bolen, Alexandru Gagniuc, Keith Busch,
Shyam Iyer, Sinan Kaya
When booting a MacBookPro9,1, duplicate link downtraining messages are
logged for the devices directly attached to the two CPU-internal Root
Ports of the Core i7 3615QM: Once on device enumeration and once on
enablement of the bandwidth notification interrupt on the Root Ports.
Duplicate messages do not occur with Root Ports on the PCH and Downstream
Ports on the Thunderbolt controller: Only a single message is logged for
these, namely on device enumeration.
The reason for the duplicate messages is a stale interrupt in the Link
Status register of the 3615QM's internal Root Ports. Avoid by clearing
the interrupt before enabling it.
An alternative approach would be to clear the interrupt already on device
enumeration or to report link downtraining only if the speed has changed.
That way, link downtraining occurring between device enumeration and
enablement of the bandwidth notification interrupt could be catched.
However clearing stale interrupts before enabling them is a standard
operating procedure for any driver and keeping the two steps in one place
makes the code easier to follow.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
drivers/pci/pcie/bw_notification.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pci/pcie/bw_notification.c b/drivers/pci/pcie/bw_notification.c
index 001d6253ad48..69e6ba2558bf 100644
--- a/drivers/pci/pcie/bw_notification.c
+++ b/drivers/pci/pcie/bw_notification.c
@@ -30,6 +30,8 @@ static void pcie_enable_link_bandwidth_notification(struct pci_dev *dev)
{
u16 lnk_ctl;
+ pcie_capability_write_word(dev, PCI_EXP_LNKSTA, PCI_EXP_LNKSTA_LBMS);
+
pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnk_ctl);
lnk_ctl |= PCI_EXP_LNKCTL_LBMIE;
pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnk_ctl);
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] PCI/LINK: bw_notification: Clear interrupt before enabling it
2019-03-20 11:05 ` [PATCH 1/2] PCI/LINK: bw_notification: Clear interrupt before enabling it Lukas Wunner
@ 2019-03-25 18:50 ` Alex_Gagniuc
0 siblings, 0 replies; 6+ messages in thread
From: Alex_Gagniuc @ 2019-03-25 18:50 UTC (permalink / raw)
To: lukas, helgaas, mr.nuke.me
Cc: linux-pci, Austin.Bolen, keith.busch, Shyam.Iyer, okaya
On 3/20/19 6:14 AM, Lukas Wunner wrote:
>
> [EXTERNAL EMAIL]
>
> When booting a MacBookPro9,1, duplicate link downtraining messages are
> logged for the devices directly attached to the two CPU-internal Root
> Ports of the Core i7 3615QM: Once on device enumeration and once on
> enablement of the bandwidth notification interrupt on the Root Ports.
>
> Duplicate messages do not occur with Root Ports on the PCH and Downstream
> Ports on the Thunderbolt controller: Only a single message is logged for
> these, namely on device enumeration.
>
> The reason for the duplicate messages is a stale interrupt in the Link
> Status register of the 3615QM's internal Root Ports. Avoid by clearing
> the interrupt before enabling it.
>
> An alternative approach would be to clear the interrupt already on device
> enumeration or to report link downtraining only if the speed has changed.
> That way, link downtraining occurring between device enumeration and
> enablement of the bandwidth notification interrupt could be catched.
> However clearing stale interrupts before enabling them is a standard
> operating procedure for any driver and keeping the two steps in one place
> makes the code easier to follow.
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Alexandru Gagniuc <alex.gagniuc@dellteam.com>
> ---
> drivers/pci/pcie/bw_notification.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/pcie/bw_notification.c b/drivers/pci/pcie/bw_notification.c
> index 001d6253ad48..69e6ba2558bf 100644
> --- a/drivers/pci/pcie/bw_notification.c
> +++ b/drivers/pci/pcie/bw_notification.c
> @@ -30,6 +30,8 @@ static void pcie_enable_link_bandwidth_notification(struct pci_dev *dev)
> {
> u16 lnk_ctl;
>
> + pcie_capability_write_word(dev, PCI_EXP_LNKSTA, PCI_EXP_LNKSTA_LBMS);
> +
> pcie_capability_read_word(dev, PCI_EXP_LNKCTL, &lnk_ctl);
> lnk_ctl |= PCI_EXP_LNKCTL_LBMIE;
> pcie_capability_write_word(dev, PCI_EXP_LNKCTL, lnk_ctl);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] PCI/LINK: bw_notification: Deduplicate reports for multi-function devices
2019-03-20 11:05 [PATCH 0/2] Link bandwidth notification fixes Lukas Wunner
2019-03-20 11:05 ` [PATCH 1/2] PCI/LINK: bw_notification: Clear interrupt before enabling it Lukas Wunner
@ 2019-03-20 11:05 ` Lukas Wunner
2019-03-25 18:52 ` Alex_Gagniuc
2019-03-25 22:33 ` [PATCH 0/2] Link bandwidth notification fixes Bjorn Helgaas
2 siblings, 1 reply; 6+ messages in thread
From: Lukas Wunner @ 2019-03-20 11:05 UTC (permalink / raw)
To: Bjorn Helgaas, Alexandru Gagniuc
Cc: linux-pci, Austin Bolen, Alexandru Gagniuc, Keith Busch,
Shyam Iyer, Sinan Kaya
If a multi-function device's bandwidth is already limited when it is
enumerated, a message is logged only for function 0. By contrast, when
downtraining occurs after enumeration, a message is logged for all
functions. That's because the former uses pcie_report_downtraining(),
whereas the latter uses __pcie_print_link_status() (which doesn't filter
functions != 0). I am seeing this happen on a MacBookPro9,1 with a GPU
(function 0) and an integrated HDA controller (function 1).
Avoid this incongruence by calling pcie_report_downtraining() in both
cases.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
drivers/pci/pci.h | 1 +
drivers/pci/pcie/bw_notification.c | 2 +-
drivers/pci/probe.c | 2 +-
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 224d88634115..d994839a3e24 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -273,6 +273,7 @@ enum pcie_link_width pcie_get_width_cap(struct pci_dev *dev);
u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed *speed,
enum pcie_link_width *width);
void __pcie_print_link_status(struct pci_dev *dev, bool verbose);
+void pcie_report_downtraining(struct pci_dev *dev);
/* Single Root I/O Virtualization */
struct pci_sriov {
diff --git a/drivers/pci/pcie/bw_notification.c b/drivers/pci/pcie/bw_notification.c
index 69e6ba2558bf..c26045f2a890 100644
--- a/drivers/pci/pcie/bw_notification.c
+++ b/drivers/pci/pcie/bw_notification.c
@@ -76,7 +76,7 @@ static irqreturn_t pcie_bw_notification_handler(int irq, void *context)
*/
down_read(&pci_bus_sem);
list_for_each_entry(dev, &port->subordinate->devices, bus_list)
- __pcie_print_link_status(dev, false);
+ pcie_report_downtraining(dev);
up_read(&pci_bus_sem);
pcie_capability_read_word(port, PCI_EXP_LNKSTA, &link_status);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2ec0df04e0dc..7e12d0163863 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2388,7 +2388,7 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
return dev;
}
-static void pcie_report_downtraining(struct pci_dev *dev)
+void pcie_report_downtraining(struct pci_dev *dev)
{
if (!pci_is_pcie(dev))
return;
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] PCI/LINK: bw_notification: Deduplicate reports for multi-function devices
2019-03-20 11:05 ` [PATCH 2/2] PCI/LINK: bw_notification: Deduplicate reports for multi-function devices Lukas Wunner
@ 2019-03-25 18:52 ` Alex_Gagniuc
0 siblings, 0 replies; 6+ messages in thread
From: Alex_Gagniuc @ 2019-03-25 18:52 UTC (permalink / raw)
To: lukas, helgaas, mr.nuke.me
Cc: linux-pci, Austin.Bolen, keith.busch, Shyam.Iyer, okaya
On 3/20/19 6:15 AM, Lukas Wunner wrote:
>
> [EXTERNAL EMAIL]
>
> If a multi-function device's bandwidth is already limited when it is
> enumerated, a message is logged only for function 0. By contrast, when
> downtraining occurs after enumeration, a message is logged for all
> functions. That's because the former uses pcie_report_downtraining(),
> whereas the latter uses __pcie_print_link_status() (which doesn't filter
> functions != 0). I am seeing this happen on a MacBookPro9,1 with a GPU
> (function 0) and an integrated HDA controller (function 1).
>
> Avoid this incongruence by calling pcie_report_downtraining() in both
> cases.
Good catch! Thank you for engineering the fixes.
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Alexandru Gagniuc <alex.gagniuc@dellteam.com>
> ---
> drivers/pci/pci.h | 1 +
> drivers/pci/pcie/bw_notification.c | 2 +-
> drivers/pci/probe.c | 2 +-
> 3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 224d88634115..d994839a3e24 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -273,6 +273,7 @@ enum pcie_link_width pcie_get_width_cap(struct pci_dev *dev);
> u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed *speed,
> enum pcie_link_width *width);
> void __pcie_print_link_status(struct pci_dev *dev, bool verbose);
> +void pcie_report_downtraining(struct pci_dev *dev);
>
> /* Single Root I/O Virtualization */
> struct pci_sriov {
> diff --git a/drivers/pci/pcie/bw_notification.c b/drivers/pci/pcie/bw_notification.c
> index 69e6ba2558bf..c26045f2a890 100644
> --- a/drivers/pci/pcie/bw_notification.c
> +++ b/drivers/pci/pcie/bw_notification.c
> @@ -76,7 +76,7 @@ static irqreturn_t pcie_bw_notification_handler(int irq, void *context)
> */
> down_read(&pci_bus_sem);
> list_for_each_entry(dev, &port->subordinate->devices, bus_list)
> - __pcie_print_link_status(dev, false);
> + pcie_report_downtraining(dev);
> up_read(&pci_bus_sem);
>
> pcie_capability_read_word(port, PCI_EXP_LNKSTA, &link_status);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 2ec0df04e0dc..7e12d0163863 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2388,7 +2388,7 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
> return dev;
> }
>
> -static void pcie_report_downtraining(struct pci_dev *dev)
> +void pcie_report_downtraining(struct pci_dev *dev)
> {
> if (!pci_is_pcie(dev))
> return;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] Link bandwidth notification fixes
2019-03-20 11:05 [PATCH 0/2] Link bandwidth notification fixes Lukas Wunner
2019-03-20 11:05 ` [PATCH 1/2] PCI/LINK: bw_notification: Clear interrupt before enabling it Lukas Wunner
2019-03-20 11:05 ` [PATCH 2/2] PCI/LINK: bw_notification: Deduplicate reports for multi-function devices Lukas Wunner
@ 2019-03-25 22:33 ` Bjorn Helgaas
2 siblings, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2019-03-25 22:33 UTC (permalink / raw)
To: Lukas Wunner
Cc: Alexandru Gagniuc, linux-pci, Austin Bolen, Alexandru Gagniuc,
Keith Busch, Shyam Iyer, Sinan Kaya
On Wed, Mar 20, 2019 at 12:05:30PM +0100, Lukas Wunner wrote:
> These two small patches fix issues that showed up on my laptop when
> testing the new bandwidth notification port service. It may be worth
> applying them to 5.1 still.
>
> They're intended to be applied on top of Alexandru's patch "Do not leave
> interrupt handler NULL" (or an updated version thereof which moves the
> call to pcie_update_link_speed() to pcie_bw_notification_irq() per my
> suggestion).
>
> Thanks,
>
> Lukas
>
> Lukas Wunner (2):
> PCI/LINK: bw_notification: Clear interrupt before enabling it
> PCI/LINK: bw_notification: Deduplicate reports for multi-function
> devices
>
> drivers/pci/pci.h | 1 +
> drivers/pci/pcie/bw_notification.c | 4 +++-
> drivers/pci/probe.c | 2 +-
> 3 files changed, 5 insertions(+), 2 deletions(-)
Applied both with Alexandru's reviewed-by to for-linus for v5.1, since this
functionality is new in v5.1, thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread