linux-kernel - RE: [PATCH] PCI/AER: Enable SERR# forwarding in non ACPI flow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BL0PR02MB48498160C3F18B81DC1ED6C1A52D0@BL0PR02MB4849.namprd02.prod.outlook.com>
Date:   Wed, 1 Aug 2018 16:07:32 +0000
From:   Bharat Kumar Gogada <bharatku@...inx.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        Ravikiran Gummaluri <rgummal@...inx.com>
Subject: RE: [PATCH] PCI/AER: Enable SERR# forwarding in non ACPI flow

Hi Bjorn,

> Subject: Re: [PATCH] PCI/AER: Enable SERR# forwarding in non ACPI flow
> 
> On Thu, Jul 12, 2018 at 08:15:19PM +0530, Bharat Kumar Gogada wrote:
> > Currently PCI_BRIDGE_CTL_SERR is being enabled only in ACPI flow.
> > This bit is required for forwarding errors reported by EP devices to
> > upstream device.
> > This patch enables SERR# for Type-1 PCI device.
> 
> This does seem broken.
> 
> Figure 6-3 in PCIe r4.0, sec 6.2.6, would be a helpful reference to include in
> the commit log.
Yes, will include it.
> 
> Semi-related question: there are about 40 drivers that call
> pci_enable_pcie_error_reporting() and
> pci_disable_pcie_error_reporting().  I see that the PCI core calls
> pci_enable_pcie_error_reporting() for Root Ports and Switch Ports in this
> path:
> 
>   aer_probe                     # for root ports only
>     aer_enable_rootport
>       set_downstream_devices_error_reporting
>         set_device_error_reporting
>           if (ROOT_PORT || UPSTREAM || DOWNSTREAM)
>             pci_enable_pcie_error_reporting
>         pci_walk_bus(..., set_device_error_reporting)
> 
> But the core doesn't call pci_enable_pcie_error_reporting() for endpoints.  I
> wonder why not.  Could we?  And then remove the calls from those drivers?
Yes, enabling error reporting on endpoints by AER driver makes sense as this is 
not an optional feature like AER capability. 
> If PCI_EXP_AER_FLAGS should only be set if the driver is prepared, the
> pci_driver.err_handler would be a good hint.
> But I suspect we could do something sensible and at least report errors even
> if the driver doesn't have err_handler callbacks.
Yes, at least reporting errors irrespective of err_handler is acceptable. 
As you mentioned not all drivers are calling pci_enable_pcie_error_reporting ,
if AER service enable's this independent of err_handler,
this would be useful for old endpoint device drivers which are not calling this.
PCI_EXP_AER_FLAGS can be set if pci_dev.driver independent of err_handler.
> 
> On MIPS Octeon, it looks like pcibios_plat_dev_init() does already set
> PCI_EXP_AER_FLAGS for every device.
> 
> But this question is obviously far beyond the scope of this current patch.
> 
> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@...inx.com>
> > ---
> >  drivers/pci/pcie/aer.c |   23 +++++++++++++++++++++++
> >  1 files changed, 23 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index
> > a2e8838..943e084 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -343,6 +343,19 @@ int pci_enable_pcie_error_reporting(struct
> pci_dev *dev)
> >  	if (!dev->aer_cap)
> >  		return -EIO;
> >
> > +	if (!IS_ENABLED(CONFIG_ACPI) &&
> > +	    dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> 
> I think this test needs to be refined a little bit.  If the kernel happens to be
> built with CONFIG_ACPI=y but the current platform doesn't support ACPI, we
> still want to set PCI_BRIDGE_CTL_SERR, don't we?
Agreed, will refine it and resend.

Thanks,
Bharat