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

["Pali Rohár" <pali@kernel.org>]

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.