diff mbox series

[01/11] PCI: pciehp: Enable DLLSC interrupt only if supported

Message ID 20220818135140.5996-2-kabel@kernel.org (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: aardvark controller changes BATCH 6 | expand

Commit Message

Marek Behún Aug. 18, 2022, 1:51 p.m. UTC 
From: Pali Rohár <pali@kernel.org>

Don't enable Data Link Layer State Changed interrupt if it isn't
supported.

Data Link Layer Link Active Reporting Capable bit in Link Capabilities
register indicates if Data Link Layer State Changed Enable is supported.

Although Lukas Wunner says [1]
  According to PCIe r6.0, sec. 7.5.3.6, "For a hot-plug capable
  Downstream Port [...], this bit must be hardwired to 1b."
the reason we want this is because of the pci-bridge-emul driver, which
emulates a bridge, but does not support asynchronous operations (since
implementing them is unneeded and would require massive changes to the
whole driver). Therefore enabling DLLSC unconditionally makes the
corresponding bit set only in the emulated configuration space of the
pci-bridge-emul driver, which
- results in confusing information when dumping the config space (it
  says that the interrupt is not supported but enabled), which may
  confuse developers when debugging PCIe issues,
- may cause bugs in the future if someone adds code that checks whether
  DLLSC is enabled and then waits for the interrupt.

[1] https://www.spinics.net/lists/linux-pci/msg124727.html

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
Changes since batch 5:
- changed commit message, previously we wrote that the change is needed
  to fix a bug where kernel was waiting for an event which did not
  come. This turns out to be false. See
  https://lore.kernel.org/linux-pci/20220818142243.4c046c59@dellmb/T/#u
---
 drivers/pci/hotplug/pciehp_hpc.c | 30 +++++++++++++++++++++++-------
 drivers/pci/hotplug/pnv_php.c    | 13 +++++++++----
 2 files changed, 32 insertions(+), 11 deletions(-)

Comments

Lukas Wunner Aug. 21, 2022, 12:46 p.m. UTC | #1 
On Thu, Aug 18, 2022 at 03:51:30PM +0200, Marek Behún wrote:
> Don't enable Data Link Layer State Changed interrupt if it isn't
> supported.
> 
> Data Link Layer Link Active Reporting Capable bit in Link Capabilities
> register indicates if Data Link Layer State Changed Enable is supported.
> 
> Although Lukas Wunner says [1]
>   According to PCIe r6.0, sec. 7.5.3.6, "For a hot-plug capable
>   Downstream Port [...], this bit must be hardwired to 1b."
> the reason we want this is because of the pci-bridge-emul driver, which
> emulates a bridge, but does not support asynchronous operations (since
> implementing them is unneeded and would require massive changes to the
> whole driver). Therefore enabling DLLSC unconditionally makes the
> corresponding bit set only in the emulated configuration space of the
> pci-bridge-emul driver, which
> - results in confusing information when dumping the config space (it
>   says that the interrupt is not supported but enabled), which may
>   confuse developers when debugging PCIe issues,
> - may cause bugs in the future if someone adds code that checks whether
>   DLLSC is enabled and then waits for the interrupt.

Honestly I'm not sure this change adds value or is necessary:

advk_pci_bridge_emul_pcie_conf_read() unconditionally sets the DLLLARC
bit, so the change doesn't have any effect for aardvark.

Same for mvebu_pci_bridge_emul_pcie_conf_read().
There are no other drivers using pci-bridge-emul.

Apart from that, it is legal to set the DLLSCE bit even on PCIe r1.0,
which did not define Data Link Layer Link Active Reporting yet.
(It defined the bit RsvdP.)  Thus there's no reason for developers
to be confused.

We're also never depending *exclusively* on DLLSC events in pciehp,
we always react to either of PDC or DLLSC, whichever comes first.
So I don't see enabling DLLSCE on unsupporting hardware as a
potential source of error.


> [1] https://www.spinics.net/lists/linux-pci/msg124727.html

Please always use lore.kernel.org links as they will likely outlast
3rd party archives:

https://lore.kernel.org/linux-pci/20220509034216.GA26780@wunner.de/


> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
[...]
> +	pcie_capability_read_dword(ctrl_dev(ctrl), PCI_EXP_LNKCAP, &link_cap);
> +

Unfortunately this new version of the patch does not address
one of my comments on the previous version:

"The Data Link Layer Link Active Reporting Capable bit is cached
in ctrl_dev(ctrl)->link_active_reporting.  Please use that
instead of re-reading it from the register."

(Verbatim quote from the above-linked e-mail.)


> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
> @@ -841,6 +841,7 @@ static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq)
>  	struct pci_dev *pdev = php_slot->pdev;
>  	u32 broken_pdc = 0;
>  	u16 sts, ctrl;
> +	u32 link_cap;
>  	int ret;
>  
>  	/* Allocate workqueue */
> @@ -874,17 +875,21 @@ static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq)
>  		return;
>  	}
>  
> +	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
> +
>  	/* Enable the interrupts */
>  	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
>  	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC) {
>  		ctrl &= ~PCI_EXP_SLTCTL_PDCE;
> -		ctrl |= (PCI_EXP_SLTCTL_HPIE |
> -			 PCI_EXP_SLTCTL_DLLSCE);
> +		ctrl |= PCI_EXP_SLTCTL_HPIE;
>  	} else {
>  		ctrl |= (PCI_EXP_SLTCTL_HPIE |
> -			 PCI_EXP_SLTCTL_PDCE |
> -			 PCI_EXP_SLTCTL_DLLSCE);
> +			 PCI_EXP_SLTCTL_PDCE);
>  	}
> +	if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
> +		ctrl |= PCI_EXP_SLTCTL_DLLSCE;
> +	else
> +		ctrl &= ~PCI_EXP_SLTCTL_DLLSCE;
>  	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, ctrl);

Note that the pnv_php.c driver is relying on DLLSC here if PDC
is broken (see PNV_PHP_FLAG_BROKEN_PDC).  By not enabling DLLSC,
you may break hotplug altogether.

pnv_php.c is a PowerPC-specific hotplug controller, but you're not
cc'ing the driver's maintainers, which are:

$ scripts/get_maintainer.pl drivers/pci/hotplug/pnv_php.c
Michael Ellerman <mpe@ellerman.id.au> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
Nicholas Piggin <npiggin@gmail.com> (reviewer:LINUX FOR POWERPC (32-BIT AND 64-BIT))
Christophe Leroy <christophe.leroy@csgroup.eu> (reviewer:LINUX FOR POWERPC (32-BIT AND 64-BIT))
linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC (32-BIT AND 64-BIT))

Thanks,

Lukas
Marek Behún Aug. 22, 2022, 10:37 a.m. UTC | #2 
On Sun, 21 Aug 2022 14:46:21 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> On Thu, Aug 18, 2022 at 03:51:30PM +0200, Marek Behún wrote:
> > Don't enable Data Link Layer State Changed interrupt if it isn't
> > supported.
> > 
> > Data Link Layer Link Active Reporting Capable bit in Link Capabilities
> > register indicates if Data Link Layer State Changed Enable is supported.
> > 
> > Although Lukas Wunner says [1]
> >   According to PCIe r6.0, sec. 7.5.3.6, "For a hot-plug capable
> >   Downstream Port [...], this bit must be hardwired to 1b."
> > the reason we want this is because of the pci-bridge-emul driver, which
> > emulates a bridge, but does not support asynchronous operations (since
> > implementing them is unneeded and would require massive changes to the
> > whole driver). Therefore enabling DLLSC unconditionally makes the
> > corresponding bit set only in the emulated configuration space of the
> > pci-bridge-emul driver, which
> > - results in confusing information when dumping the config space (it
> >   says that the interrupt is not supported but enabled), which may
> >   confuse developers when debugging PCIe issues,
> > - may cause bugs in the future if someone adds code that checks whether
> >   DLLSC is enabled and then waits for the interrupt.  
> 
> Honestly I'm not sure this change adds value or is necessary:
> 
> advk_pci_bridge_emul_pcie_conf_read() unconditionally sets the DLLLARC
> bit, so the change doesn't have any effect for aardvark.

This is the status now, but it wasn't always so. The support for
that was added in one of the previous batches of aardvark's changes.

> Same for mvebu_pci_bridge_emul_pcie_conf_read().
> There are no other drivers using pci-bridge-emul.

But there may be future users which won't support it and we were just
thinking this could spare some confusion to the developers, since Pali
spent nontrivial time on this when developing/debuggin aardvark last
year.

> Apart from that, it is legal to set the DLLSCE bit even on PCIe r1.0,
> which did not define Data Link Layer Link Active Reporting yet.
> (It defined the bit RsvdP.)  Thus there's no reason for developers
> to be confused.
> 
> We're also never depending *exclusively* on DLLSC events in pciehp,
> we always react to either of PDC or DLLSC, whichever comes first.
> So I don't see enabling DLLSCE on unsupporting hardware as a
> potential source of error.
> 
> 
> > [1] https://www.spinics.net/lists/linux-pci/msg124727.html  
> 
> Please always use lore.kernel.org links as they will likely outlast
> 3rd party archives:
> 
> https://lore.kernel.org/linux-pci/20220509034216.GA26780@wunner.de/

Will do.

> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c  
> [...]
> > +	pcie_capability_read_dword(ctrl_dev(ctrl), PCI_EXP_LNKCAP, &link_cap);
> > +  
> 
> Unfortunately this new version of the patch does not address
> one of my comments on the previous version:
> 
> "The Data Link Layer Link Active Reporting Capable bit is cached
> in ctrl_dev(ctrl)->link_active_reporting.  Please use that
> instead of re-reading it from the register."
> 
> (Verbatim quote from the above-linked e-mail.)

Sorry, I forgot abbout this one.

> > --- a/drivers/pci/hotplug/pnv_php.c
> > +++ b/drivers/pci/hotplug/pnv_php.c
> > @@ -841,6 +841,7 @@ static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq)
> >  	struct pci_dev *pdev = php_slot->pdev;
> >  	u32 broken_pdc = 0;
> >  	u16 sts, ctrl;
> > +	u32 link_cap;
> >  	int ret;
> >  
> >  	/* Allocate workqueue */
> > @@ -874,17 +875,21 @@ static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq)
> >  		return;
> >  	}
> >  
> > +	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
> > +
> >  	/* Enable the interrupts */
> >  	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
> >  	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC) {
> >  		ctrl &= ~PCI_EXP_SLTCTL_PDCE;
> > -		ctrl |= (PCI_EXP_SLTCTL_HPIE |
> > -			 PCI_EXP_SLTCTL_DLLSCE);
> > +		ctrl |= PCI_EXP_SLTCTL_HPIE;
> >  	} else {
> >  		ctrl |= (PCI_EXP_SLTCTL_HPIE |
> > -			 PCI_EXP_SLTCTL_PDCE |
> > -			 PCI_EXP_SLTCTL_DLLSCE);
> > +			 PCI_EXP_SLTCTL_PDCE);
> >  	}
> > +	if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
> > +		ctrl |= PCI_EXP_SLTCTL_DLLSCE;
> > +	else
> > +		ctrl &= ~PCI_EXP_SLTCTL_DLLSCE;
> >  	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, ctrl);  
> 
> Note that the pnv_php.c driver is relying on DLLSC here if PDC
> is broken (see PNV_PHP_FLAG_BROKEN_PDC).  By not enabling DLLSC,
> you may break hotplug altogether.

But it would only break if those controllers did not report DLLLARC,
right?

> pnv_php.c is a PowerPC-specific hotplug controller, but you're not
> cc'ing the driver's maintainers, which are:
> 
> $ scripts/get_maintainer.pl drivers/pci/hotplug/pnv_php.c
> Michael Ellerman <mpe@ellerman.id.au> (supporter:LINUX FOR POWERPC (32-BIT AND 64-BIT))
> Nicholas Piggin <npiggin@gmail.com> (reviewer:LINUX FOR POWERPC (32-BIT AND 64-BIT))
> Christophe Leroy <christophe.leroy@csgroup.eu> (reviewer:LINUX FOR POWERPC (32-BIT AND 64-BIT))
> linuxppc-dev@lists.ozlabs.org (open list:LINUX FOR POWERPC (32-BIT AND 64-BIT))

Will do.

Anyway, Lorenzo, this patch can be skipped now, the other patches do
not depend on it.

Marek
diff mbox series

Patch

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 040ae076ec0e..373bb396fe22 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -788,6 +788,7 @@  static int pciehp_poll(void *data)
 static void pcie_enable_notification(struct controller *ctrl)
 {
 	u16 cmd, mask;
+	u32 link_cap;
 
 	/*
 	 * TBD: Power fault detected software notification support.
@@ -800,12 +801,17 @@  static void pcie_enable_notification(struct controller *ctrl)
 	 * next power fault detected interrupt was notified again.
 	 */
 
+	pcie_capability_read_dword(ctrl_dev(ctrl), PCI_EXP_LNKCAP, &link_cap);
+
 	/*
-	 * Always enable link events: thus link-up and link-down shall
-	 * always be treated as hotplug and unplug respectively. Enable
-	 * presence detect only if Attention Button is not present.
+	 * Enable link events if their support is indicated in Link Capability
+	 * register: thus link-up and link-down shall always be treated as
+	 * hotplug and unplug respectively. Enable presence detect only if
+	 * Attention Button is not present.
 	 */
-	cmd = PCI_EXP_SLTCTL_DLLSCE;
+	cmd = 0;
+	if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
+		cmd |= PCI_EXP_SLTCTL_DLLSCE;
 	if (ATTN_BUTTN(ctrl))
 		cmd |= PCI_EXP_SLTCTL_ABPE;
 	else
@@ -844,9 +850,14 @@  void pcie_clear_hotplug_events(struct controller *ctrl)
 
 void pcie_enable_interrupt(struct controller *ctrl)
 {
+	u32 link_cap;
 	u16 mask;
 
-	mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE;
+	pcie_capability_read_dword(ctrl_dev(ctrl), PCI_EXP_LNKCAP, &link_cap);
+
+	mask = PCI_EXP_SLTCTL_HPIE;
+	if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
+		mask |= PCI_EXP_SLTCTL_DLLSCE;
 	pcie_write_cmd(ctrl, mask, mask);
 }
 
@@ -904,19 +915,24 @@  int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, bool probe)
 	struct controller *ctrl = to_ctrl(hotplug_slot);
 	struct pci_dev *pdev = ctrl_dev(ctrl);
 	u16 stat_mask = 0, ctrl_mask = 0;
+	u32 link_cap;
 	int rc;
 
 	if (probe)
 		return 0;
 
+	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
+
 	down_write_nested(&ctrl->reset_lock, ctrl->depth);
 
 	if (!ATTN_BUTTN(ctrl)) {
 		ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
 		stat_mask |= PCI_EXP_SLTSTA_PDC;
 	}
-	ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
-	stat_mask |= PCI_EXP_SLTSTA_DLLSC;
+	if (link_cap & PCI_EXP_LNKCAP_DLLLARC) {
+		ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
+		stat_mask |= PCI_EXP_SLTSTA_DLLSC;
+	}
 
 	pcie_write_cmd(ctrl, 0, ctrl_mask);
 	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
index 881d420637bf..118b514f66b9 100644
--- a/drivers/pci/hotplug/pnv_php.c
+++ b/drivers/pci/hotplug/pnv_php.c
@@ -841,6 +841,7 @@  static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq)
 	struct pci_dev *pdev = php_slot->pdev;
 	u32 broken_pdc = 0;
 	u16 sts, ctrl;
+	u32 link_cap;
 	int ret;
 
 	/* Allocate workqueue */
@@ -874,17 +875,21 @@  static void pnv_php_init_irq(struct pnv_php_slot *php_slot, int irq)
 		return;
 	}
 
+	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
+
 	/* Enable the interrupts */
 	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &ctrl);
 	if (php_slot->flags & PNV_PHP_FLAG_BROKEN_PDC) {
 		ctrl &= ~PCI_EXP_SLTCTL_PDCE;
-		ctrl |= (PCI_EXP_SLTCTL_HPIE |
-			 PCI_EXP_SLTCTL_DLLSCE);
+		ctrl |= PCI_EXP_SLTCTL_HPIE;
 	} else {
 		ctrl |= (PCI_EXP_SLTCTL_HPIE |
-			 PCI_EXP_SLTCTL_PDCE |
-			 PCI_EXP_SLTCTL_DLLSCE);
+			 PCI_EXP_SLTCTL_PDCE);
 	}
+	if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
+		ctrl |= PCI_EXP_SLTCTL_DLLSCE;
+	else
+		ctrl &= ~PCI_EXP_SLTCTL_DLLSCE;
 	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, ctrl);
 
 	/* The interrupt is initialized successfully when @irq is valid */