diff mbox series

[06/18] PCI: pciehp: Enable DLLSC interrupt only if supported

Message ID 20220220193346.23789-7-kabel@kernel.org (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series PCI: aardvark controller changes BATCH 5 | expand

Commit Message

Marek Behún Feb. 20, 2022, 7:33 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.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/pci/hotplug/pciehp_hpc.c | 30 +++++++++++++++++++++++-------
 drivers/pci/hotplug/pnv_php.c    | 13 +++++++++----
 2 files changed, 32 insertions(+), 11 deletions(-)

Comments

Lukas Wunner May 9, 2022, 3:42 a.m. UTC | #1 
On Sun, Feb 20, 2022 at 08:33:34PM +0100, Marek Behún wrote:
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -788,6 +788,7 @@ static int pciehp_poll(void *data)
> @@ -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;

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.

According to PCIe r6.0, sec. 7.5.3.6, "For a hot-plug capable
Downstream Port [...], this bit must be hardwired to 1b."

That has been part of the spec since PCIe r1.1, sec. 7.8.6.

PCIe r1.0 did not contain the sentence because it did not support
DLLLARC (it defined bit 20 as RsvdP).

In other words, what you're doing here is add support for PCIe r1.0.
I'm not opposed to that, but I'd assume that aardvark supports a
more recent spec version.  More likely it doesn't comply with the spec?

What is the user-visible issue that you're experiencing without this
commit?  Does aardvark somehow misbehave if the DLLLARC bit is set to 1?
Since the bit is RsvdP, setting it shouldn't have any negative side
effects.


> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
> @@ -840,6 +840,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 */

pnv_php.c is a driver for PowerNV, yet this patch is for a series
targeting an ARM PCIe controller.  That doesn't make sense,
changes to pnv_php.c seem wrong here.

Thanks,

Lukas
Pali Rohár May 13, 2022, 4:57 p.m. UTC | #2 
On Monday 09 May 2022 05:42:16 Lukas Wunner wrote:
> On Sun, Feb 20, 2022 at 08:33:34PM +0100, Marek Behún wrote:
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -788,6 +788,7 @@ static int pciehp_poll(void *data)
> > @@ -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;
> 
> 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.
> 
> According to PCIe r6.0, sec. 7.5.3.6, "For a hot-plug capable
> Downstream Port [...], this bit must be hardwired to 1b."
> 
> That has been part of the spec since PCIe r1.1, sec. 7.8.6.
> 
> PCIe r1.0 did not contain the sentence because it did not support
> DLLLARC (it defined bit 20 as RsvdP).
> 
> In other words, what you're doing here is add support for PCIe r1.0.
> I'm not opposed to that, but I'd assume that aardvark supports a
> more recent spec version.  More likely it doesn't comply with the spec?
> 
> What is the user-visible issue that you're experiencing without this
> commit?  Does aardvark somehow misbehave if the DLLLARC bit is set to 1?
> Since the bit is RsvdP, setting it shouldn't have any negative side
> effects.

I will let fixing those issues to Marek.

To answer your questions: Config space of Aardvark Root Port does not
conform to PCIe base spec. It does not implement DLLLARC, nor DLLSCE and
lot of other bits. Plus it has Type 0 header (not Type 1). And due to
these reasons, pci-aardvark.c driver implements "emulation" of the
Root Port and implements some of the functionality via custom aardvark
registers. So Root Port would be presented to kernel and also to
userspace as PCI Bridge device with Type 1 header and with PCIe
registers required by linux kernel.

During my testing of kernel hotplug code last year, I figured out that
kernel was waiting for event which never happened. And so it was needed
to "fix" kernel to not try to enable DLLSCE because it did nothing.

I asked more times Marvell for Aardvark documentation, so I could fix
these issues, but I have never received any response for it.

> 
> > --- a/drivers/pci/hotplug/pnv_php.c
> > +++ b/drivers/pci/hotplug/pnv_php.c
> > @@ -840,6 +840,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 */
> 
> pnv_php.c is a driver for PowerNV, yet this patch is for a series
> targeting an ARM PCIe controller.  That doesn't make sense,
> changes to pnv_php.c seem wrong here.
> 
> Thanks,
> 
> Lukas

At time when I prepared this patch (year ago) I changed that DLLSCE
pattern in all places because it looked like copy+paste code which
should be fixed too.
Lukas Wunner May 14, 2022, 9:14 a.m. UTC | #3 
On Fri, May 13, 2022 at 06:57:29PM +0200, Pali Rohár wrote:
> To answer your questions: Config space of Aardvark Root Port does not
> conform to PCIe base spec. It does not implement DLLLARC, nor DLLSCE and
> lot of other bits. Plus it has Type 0 header (not Type 1). And due to
> these reasons, pci-aardvark.c driver implements "emulation" of the
> Root Port and implements some of the functionality via custom aardvark
> registers. So Root Port would be presented to kernel and also to
> userspace as PCI Bridge device with Type 1 header and with PCIe
> registers required by linux kernel.
> 
> During my testing of kernel hotplug code last year, I figured out that
> kernel was waiting for event which never happened. And so it was needed
> to "fix" kernel to not try to enable DLLSCE because it did nothing.

Could you please reproduce this and add the following on the command line:

  log_buf_len=10M pciehp.pciehp_debug=1 dyndbg="file pciehp* +p"
  ignore_loglevel

Then open a bug at bugzilla.kernel.org, attach full dmesg output
as well as full "lspci -vv" output and send the bugzilla link to me.

(Obviously, revert patches 6 and 7 when trying to reproduce it.)

So a PDC event should be sufficient to bring the slot up or down,
a DLLSC event should not be necessary.  Enabling DLLSC should not
make a difference on a controller which doesn't support it.
I just double-checked the code and I do not see where we'd wait
for a DLLSC event which never comes.

Don't worry, if we come to the conclusion that your proposed fix
is the only solution, I'm fine with it, but at this point I'd
like to get a better understanding what is really going on.
Perhaps there is some other issue in pciehp that this patch
just papers over.  Once you provide the dmesg debug output
I'll be able to analyze that.

Thanks!

Lukas
Marek Behún Aug. 18, 2022, 12:22 p.m. UTC | #4 
On Sat, 14 May 2022 11:14:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> On Fri, May 13, 2022 at 06:57:29PM +0200, Pali Rohár wrote:
> > To answer your questions: Config space of Aardvark Root Port does not
> > conform to PCIe base spec. It does not implement DLLLARC, nor DLLSCE and
> > lot of other bits. Plus it has Type 0 header (not Type 1). And due to
> > these reasons, pci-aardvark.c driver implements "emulation" of the
> > Root Port and implements some of the functionality via custom aardvark
> > registers. So Root Port would be presented to kernel and also to
> > userspace as PCI Bridge device with Type 1 header and with PCIe
> > registers required by linux kernel.
> > 
> > During my testing of kernel hotplug code last year, I figured out that
> > kernel was waiting for event which never happened. And so it was needed
> > to "fix" kernel to not try to enable DLLSCE because it did nothing.  
> 
> Could you please reproduce this and add the following on the command line:
> 
>   log_buf_len=10M pciehp.pciehp_debug=1 dyndbg="file pciehp* +p"
>   ignore_loglevel
> 
> Then open a bug at bugzilla.kernel.org, attach full dmesg output
> as well as full "lspci -vv" output and send the bugzilla link to me.
> 
> (Obviously, revert patches 6 and 7 when trying to reproduce it.)
> 
> So a PDC event should be sufficient to bring the slot up or down,
> a DLLSC event should not be necessary.  Enabling DLLSC should not
> make a difference on a controller which doesn't support it.
> I just double-checked the code and I do not see where we'd wait
> for a DLLSC event which never comes.
> 
> Don't worry, if we come to the conclusion that your proposed fix
> is the only solution, I'm fine with it, but at this point I'd
> like to get a better understanding what is really going on.
> Perhaps there is some other issue in pciehp that this patch
> just papers over.  Once you provide the dmesg debug output
> I'll be able to analyze that.

Dear Lukas,

we have tried to reproduce the bug where kernel was waiting for an
event which never happend, the bug that Pali remembered from his
work on the pciehp code.

We have concluded that it doesn't concert the DLLSC patch (06/18), only
the Command Completed Interrupt patch (07/18), and even there it seems
that the patch is not needed to trigger the bug: it seems that when
Pali was studying the bug, he did two things:
1. he made enabling Command Completed Interrupt conditional on NCCS bit
   not  set
2. he made the aardvark driver report NCCS bit via emulated bridge.

It turns out that only the second thing is needed, since the pciehp
code checks NCCS bit in pcie_wait_cmd() and does not wait for the
interrupt if NCCS is set.

Anyway we still think that both patches make sense, at least so that an
interrupt isn't reported as enabled and not supported at once when
dumping the configuration space.

So I will resend these patches with updated commit messages.

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 f4c2e6e01be0..e05e8460eb2c 100644
--- a/drivers/pci/hotplug/pnv_php.c
+++ b/drivers/pci/hotplug/pnv_php.c
@@ -840,6 +840,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 */
@@ -873,17 +874,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 */