| Message ID | 1531406719-18606-1-git-send-email-bharat.kumar.gogada@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 2018-07-12 20:15, 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. > > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.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) { > + u16 control; > + > + /* > + * A Type-1 PCI bridge will not forward ERR_ messages coming > + * from an endpoint if SERR# forwarding is not enabled. > + */ > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &control); > + control |= PCI_BRIDGE_CTL_SERR; > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); > + } > + > return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, > PCI_EXP_AER_FLAGS); > } > EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting); > @@ -352,6 +365,16 @@ int pci_disable_pcie_error_reporting(struct > pci_dev *dev) > if (pcie_aer_get_firmware_first(dev)) > return -EIO; > > + if (!IS_ENABLED(CONFIG_ACPI) && > + dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) { > + u16 control; > + > + /* Clear SERR Forwarding */ > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &control); > + control &= ~PCI_BRIDGE_CTL_SERR; > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); > + } > + > return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, > PCI_EXP_AER_FLAGS); > } Should this configuration no be set by Firmware ? why should Linux dictate it ? Regards, Oza.
> > 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. > > > > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.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) { > > + u16 control; > > + > > + /* > > + * A Type-1 PCI bridge will not forward ERR_ messages > coming > > + * from an endpoint if SERR# forwarding is not enabled. > > + */ > > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, > &control); > > + control |= PCI_BRIDGE_CTL_SERR; > > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); > > + } > > + > > return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, > > PCI_EXP_AER_FLAGS); } > > EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting); > > @@ -352,6 +365,16 @@ int pci_disable_pcie_error_reporting(struct > > pci_dev *dev) > > if (pcie_aer_get_firmware_first(dev)) > > return -EIO; > > > > + if (!IS_ENABLED(CONFIG_ACPI) && > > + dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) { > > + u16 control; > > + > > + /* Clear SERR Forwarding */ > > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, > &control); > > + control &= ~PCI_BRIDGE_CTL_SERR; > > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); > > + } > > + > > return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, > > PCI_EXP_AER_FLAGS); > > } > > > Should this configuration no be set by Firmware ? why should Linux dictate it > ? Hi Oza, Can you please let us know why this should be set by firmware ? Spec clearly states ERR_CORR,ERR_FATAL/NON FATAL will be forwarded only if this bit is set. If linux AER service is being enabled without checking/setting this bit, then AER service will not do anything even ERR_* is seen on link. Regards, Bharat
On 2018-07-13 19:25, 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. >> > >> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.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) { >> > + u16 control; >> > + >> > + /* >> > + * A Type-1 PCI bridge will not forward ERR_ messages >> coming >> > + * from an endpoint if SERR# forwarding is not enabled. >> > + */ >> > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, >> &control); >> > + control |= PCI_BRIDGE_CTL_SERR; >> > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); >> > + } >> > + >> > return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, >> > PCI_EXP_AER_FLAGS); } >> > EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting); >> > @@ -352,6 +365,16 @@ int pci_disable_pcie_error_reporting(struct >> > pci_dev *dev) >> > if (pcie_aer_get_firmware_first(dev)) >> > return -EIO; >> > >> > + if (!IS_ENABLED(CONFIG_ACPI) && >> > + dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) { >> > + u16 control; >> > + >> > + /* Clear SERR Forwarding */ >> > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, >> &control); >> > + control &= ~PCI_BRIDGE_CTL_SERR; >> > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); >> > + } >> > + >> > return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, >> > PCI_EXP_AER_FLAGS); >> > } >> >> >> Should this configuration no be set by Firmware ? why should Linux >> dictate it >> ? > Hi Oza, Can you please let us know why this should be set by firmware ? > Spec clearly states ERR_CORR,ERR_FATAL/NON FATAL will be forwarded > only if this bit is set. > If linux AER service is being enabled without checking/setting this > bit, then AER service will > not do anything even ERR_* is seen on link. > > Regards, > Bharat The ERR_* to be forwarded or not to be forwarded could be decision of the platform. hence I think it is best left to firmware to decide if it want to enable this for particular platform. although, There are 2 cases Hotplug capable bridge and otherwise. 1) If Firmware sets them, I do not think during enumeraion linux will loose those settings. 2) I do not see any integration of hotplug with AER currently, so if the PCIe switch is plugged into Hotplug capable RP, I am not very sure if this functions get called. Keith, Lukas and Bjorn any comments ? Regards, Oza.
> On 2018-07-13 19:25, 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. > >> > > >> > Signed-off-by: Bharat Kumar Gogada > <bharat.kumar.gogada@xilinx.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) { > >> > + u16 control; > >> > + > >> > + /* > >> > + * A Type-1 PCI bridge will not forward ERR_ messages > >> coming > >> > + * from an endpoint if SERR# forwarding is not enabled. > >> > + */ > >> > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, > >> &control); > >> > + control |= PCI_BRIDGE_CTL_SERR; > >> > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); > >> > + } > >> > + > >> > return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, > >> > PCI_EXP_AER_FLAGS); } > >> > EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting); > >> > @@ -352,6 +365,16 @@ int pci_disable_pcie_error_reporting(struct > >> > pci_dev *dev) > >> > if (pcie_aer_get_firmware_first(dev)) > >> > return -EIO; > >> > > >> > + if (!IS_ENABLED(CONFIG_ACPI) && > >> > + dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) { > >> > + u16 control; > >> > + > >> > + /* Clear SERR Forwarding */ > >> > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, > >> &control); > >> > + control &= ~PCI_BRIDGE_CTL_SERR; > >> > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); > >> > + } > >> > + > >> > return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, > >> > PCI_EXP_AER_FLAGS); > >> > } > >> > >> > >> Should this configuration no be set by Firmware ? why should Linux > >> dictate it ? > > Hi Oza, Can you please let us know why this should be set by firmware ? > > Spec clearly states ERR_CORR,ERR_FATAL/NON FATAL will be forwarded > > only if this bit is set. > > If linux AER service is being enabled without checking/setting this > > bit, then AER service will not do anything even ERR_* is seen on link. > > > > Regards, > > Bharat > > > The ERR_* to be forwarded or not to be forwarded could be decision of the > platform. > hence I think it is best left to firmware to decide if it want to enable this for > particular platform. > I'm not aware of other platforms, can you please give an example of a platform how it decides to set this in firmware ? > although, > There are 2 cases > Hotplug capable bridge and otherwise. Yes, what about an RP which supports only AER but doesn't support Hotplug ? If we have this patch we can set this bit without firmware also. > > 1) If Firmware sets them, I do not think during enumeraion linux will loose > those settings. > 2) I do not see any integration of hotplug with AER currently, so if the PCIe > switch is plugged into Hotplug capable RP, I am not very sure if this functions > get called. > > Keith, Lukas and Bjorn any comments ? Hi all, do you have any inputs on this ? Regards, Bharat
On 2018-07-18 19:04, Bharat Kumar Gogada wrote: >> On 2018-07-13 19:25, 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. >> >> > >> >> > Signed-off-by: Bharat Kumar Gogada >> <bharat.kumar.gogada@xilinx.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) { >> >> > + u16 control; >> >> > + >> >> > + /* >> >> > + * A Type-1 PCI bridge will not forward ERR_ messages >> >> coming >> >> > + * from an endpoint if SERR# forwarding is not enabled. >> >> > + */ >> >> > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, >> >> &control); >> >> > + control |= PCI_BRIDGE_CTL_SERR; >> >> > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); >> >> > + } >> >> > + >> >> > return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, >> >> > PCI_EXP_AER_FLAGS); } >> >> > EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting); >> >> > @@ -352,6 +365,16 @@ int pci_disable_pcie_error_reporting(struct >> >> > pci_dev *dev) >> >> > if (pcie_aer_get_firmware_first(dev)) >> >> > return -EIO; >> >> > >> >> > + if (!IS_ENABLED(CONFIG_ACPI) && >> >> > + dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) { >> >> > + u16 control; >> >> > + >> >> > + /* Clear SERR Forwarding */ >> >> > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, >> >> &control); >> >> > + control &= ~PCI_BRIDGE_CTL_SERR; >> >> > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); >> >> > + } >> >> > + >> >> > return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, >> >> > PCI_EXP_AER_FLAGS); >> >> > } >> >> >> >> >> >> Should this configuration no be set by Firmware ? why should Linux >> >> dictate it ? >> > Hi Oza, Can you please let us know why this should be set by firmware ? >> > Spec clearly states ERR_CORR,ERR_FATAL/NON FATAL will be forwarded >> > only if this bit is set. >> > If linux AER service is being enabled without checking/setting this >> > bit, then AER service will not do anything even ERR_* is seen on link. >> > >> > Regards, >> > Bharat >> >> >> The ERR_* to be forwarded or not to be forwarded could be decision of >> the >> platform. >> hence I think it is best left to firmware to decide if it want to >> enable this for >> particular platform. >> > I'm not aware of other platforms, can you please give an example of a > platform > how it decides to set this in firmware ? > >> although, >> There are 2 cases >> Hotplug capable bridge and otherwise. > > Yes, what about an RP which supports only AER but doesn't support > Hotplug ? > If we have this patch we can set this bit without firmware also. >> >> 1) If Firmware sets them, I do not think during enumeraion linux will >> loose >> those settings. > >> 2) I do not see any integration of hotplug with AER currently, so if >> the PCIe >> switch is plugged into Hotplug capable RP, I am not very sure if this >> functions >> get called. >> >> Keith, Lukas and Bjorn any comments ? > Hi all, do you have any inputs on this ? > I have thought more on this..but needs separate discussion hence will be forking the separate thread. If I choose to make some more patches based on this patch, this patch can remain as is and will be included with your authorship. Regards, Oza. > Regards, > Bharat
Hi Bjorn and Keith,
This discussion is to extend the idea of follwing patch.
[PATCH] PCI/AER: Enable SERR# forwarding in non ACPI flow
PCIe Spec
7.6.2.1.3 Command Register (Offset 04h)
SERR# Enable – See Section 7.6.2.1.14.
When Set, this bit enables reporting upstream of Non-fatal and Fatal
errors detected by the Function to the Root Complex. Note that errors
are reported if enabled either through this bit or through the PCI
Express specific bits in the Device Control register (see Section
7.6.3.4).
In addition, for Functions with Type 1 Configuration Space headers, this
bit controls transmission by the primary interface of ERR_NONFATAL and
ERR_FATAL error Messages forwarded from the secondary interface. This
bit does not affect the transmission of forwarded ERR_COR messages.
A Root Complex Integrated Endpoint that is not associated with a Root
Complex Event Collector is permitted to hardwire this bit to 0b.
Default value of this bit is 0b.
7.6.2.3.13 Bridge Control Register
SERR# Enable – See Section 7.6.2.1.147.5.1.8.
This bit controls forwarding of ERR_COR, ERR_NONFATAL and ERR_FATAL from
secondary to primary.
6.2.3.2.2
The transmission of these error Messages by class (correctable,
non-fatal, fatal) is enabled using the Reporting Enable bits of the
Device Control register (see Section 7.6.3.4) or the SERR# Enable bit in
the PCI Command register (see Section 7.6.2.1.3).
AER driver touches device control (and choose not to touch PCI_COMMAND)
On the hand SERR# of Bridge Control Register is not set either.
The meaning of both the SERR# for type-1 configuration space seems to me
the same.
both essentially says that ERR_NONFATAL and ERR_FATAL from secondary to
primary.
except that bridge control setting, also forwards ERR_COR messages while
Command Register settings affect only ERR_NONFATAL and ERR_FATAL.
there are 2 cases,
1)hotplug Enabled slot is inserted with type-1 configuration space
(bridge) and
2) hot plug disabled, where on our platform we typically set #SERR by
firmware
So yes it makes sense to set #SERR bit by AER driver if it fins bridge.
but we not only have do
[PATCH] PCI/AER: Enable SERR# forwarding in non ACPI flow
but also we have to cover hotplug case and hence
pci_aer_init() should call
pci_enable_pcie_error_reporting(dev);
something like below.
int pci_aer_init(struct pci_dev *dev)
{
int rc;
dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
pci_enable_pcie_error_reporting(dev);
return pci_cleanup_aer_error_status_regs(dev);
}
int pci_enable_pcie_error_reporting(struct pci_dev *dev)
{
int ret;
if (pcie_aer_get_firmware_first(dev))
return -EIO;
if (!dev->aer_cap)
return -EIO;
if (!IS_ENABLED(CONFIG_ACPI) &&
dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
u16 control;
/*
* A Type-1 PCI bridge will not forward ERR_ messages coming
* from an endpoint if SERR# forwarding is not enabled.
*/
pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &control);
control |= PCI_BRIDGE_CTL_SERR;
pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control);
}
return pcie_capability_set_word(dev, PCI_EXP_DEVCTL,
PCI_EXP_AER_FLAGS);
}
EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting);
also we have to remove pci_enable_pcie_error_reporting() call from the
drivers.
because aer_init will do it for all the devices.
although I am not very sure is it safe to detect enable error reporting
by default for all the error devices ?
e.g. setting PCI_EXP_DEVCTL.
probably drivers might want to call pci_disable_pcie_error_reporting()
who doesnt want to participate in error reporting.
Regards,
Oza.
> Subject: Re: [PATCH] PCI/AER: Enable SERR# forwarding in non ACPI flow > > On 2018-07-18 19:04, Bharat Kumar Gogada wrote: > >> On 2018-07-13 19:25, 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. > >> >> > > >> >> > Signed-off-by: Bharat Kumar Gogada > >> <bharat.kumar.gogada@xilinx.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) { > >> >> > + u16 control; > >> >> > + > >> >> > + /* > >> >> > + * A Type-1 PCI bridge will not forward ERR_ > messages > >> >> coming > >> >> > + * from an endpoint if SERR# forwarding is not > enabled. > >> >> > + */ > >> >> > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, > >> >> &control); > >> >> > + control |= PCI_BRIDGE_CTL_SERR; > >> >> > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, > control); > >> >> > + } > >> >> > + > >> >> > return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, > >> >> > PCI_EXP_AER_FLAGS); } > >> >> > EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting); > >> >> > @@ -352,6 +365,16 @@ int pci_disable_pcie_error_reporting(struct > >> >> > pci_dev *dev) > >> >> > if (pcie_aer_get_firmware_first(dev)) > >> >> > return -EIO; > >> >> > > >> >> > + if (!IS_ENABLED(CONFIG_ACPI) && > >> >> > + dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) { > >> >> > + u16 control; > >> >> > + > >> >> > + /* Clear SERR Forwarding */ > >> >> > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, > >> >> &control); > >> >> > + control &= ~PCI_BRIDGE_CTL_SERR; > >> >> > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, > control); > >> >> > + } > >> >> > + > >> >> > return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, > >> >> > PCI_EXP_AER_FLAGS); > >> >> > } > >> >> > >> >> > >> >> Should this configuration no be set by Firmware ? why should Linux > >> >> dictate it ? > >> > Hi Oza, Can you please let us know why this should be set by firmware ? > >> > Spec clearly states ERR_CORR,ERR_FATAL/NON FATAL will be forwarded > >> > only if this bit is set. > >> > If linux AER service is being enabled without checking/setting this > >> > bit, then AER service will not do anything even ERR_* is seen on link. > >> > > >> > Regards, > >> > Bharat > >> > >> > >> The ERR_* to be forwarded or not to be forwarded could be decision of > >> the platform. > >> hence I think it is best left to firmware to decide if it want to > >> enable this for particular platform. > >> > > I'm not aware of other platforms, can you please give an example of a > > platform how it decides to set this in firmware ? > > > >> although, > >> There are 2 cases > >> Hotplug capable bridge and otherwise. > > > > Yes, what about an RP which supports only AER but doesn't support > > Hotplug ? > > If we have this patch we can set this bit without firmware also. > >> > >> 1) If Firmware sets them, I do not think during enumeraion linux will > >> loose those settings. > > > >> 2) I do not see any integration of hotplug with AER currently, so if > >> the PCIe switch is plugged into Hotplug capable RP, I am not very > >> sure if this functions get called. > >> > >> Keith, Lukas and Bjorn any comments ? > > Hi all, do you have any inputs on this ? > > > > I have thought more on this..but needs separate discussion hence will be > forking the separate thread. > If I choose to make some more patches based on this patch, this patch can > remain as is and will be included with your authorship. > > Regards, > Oza. Thanks Oza.
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. 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? 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. 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@xilinx.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? > + u16 control; > + > + /* > + * A Type-1 PCI bridge will not forward ERR_ messages coming > + * from an endpoint if SERR# forwarding is not enabled. > + */ > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &control); > + control |= PCI_BRIDGE_CTL_SERR; > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); > + } > + > return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS); > } > EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting); > @@ -352,6 +365,16 @@ int pci_disable_pcie_error_reporting(struct pci_dev *dev) > if (pcie_aer_get_firmware_first(dev)) > return -EIO; > > + if (!IS_ENABLED(CONFIG_ACPI) && > + dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) { > + u16 control; > + > + /* Clear SERR Forwarding */ > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &control); > + control &= ~PCI_BRIDGE_CTL_SERR; > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); > + } > + > return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, > PCI_EXP_AER_FLAGS); > } > -- > 1.7.1 >
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@xilinx.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
On 2018-08-01 04:17, Bjorn Helgaas wrote: > 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. > > 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? 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. > what about hot-plug case ? should not aer_init() call pci_enable_pcie_error_reporting() for all the downstream pci_dev ? and remove all the calls from drivers.. > 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@xilinx.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? > >> + u16 control; >> + >> + /* >> + * A Type-1 PCI bridge will not forward ERR_ messages coming >> + * from an endpoint if SERR# forwarding is not enabled. >> + */ >> + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &control); >> + control |= PCI_BRIDGE_CTL_SERR; >> + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); >> + } >> + >> return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, >> PCI_EXP_AER_FLAGS); >> } >> EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting); >> @@ -352,6 +365,16 @@ int pci_disable_pcie_error_reporting(struct >> pci_dev *dev) >> if (pcie_aer_get_firmware_first(dev)) >> return -EIO; >> >> + if (!IS_ENABLED(CONFIG_ACPI) && >> + dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) { >> + u16 control; >> + >> + /* Clear SERR Forwarding */ >> + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &control); >> + control &= ~PCI_BRIDGE_CTL_SERR; >> + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); >> + } >> + >> return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, >> PCI_EXP_AER_FLAGS); >> } >> -- >> 1.7.1 >>
On 2018-08-02 11:53, poza@codeaurora.org wrote: > On 2018-08-01 04:17, Bjorn Helgaas wrote: >> 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. >> >> 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? 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. >> > > what about hot-plug case ? > should not aer_init() call pci_enable_pcie_error_reporting() for all > the downstream pci_dev ? > and remove all the calls from drivers.. > aer_init will be called for each device (pci_dev) while pciehp does re-enumeration. so probable we might want to call pci_enable_pcie_error_reporting() but that dictates the design where AER framework is taking decision to enable error reporting on behalf of drivers as well. but thats fine I think, if drivers do not want to participate then they have to call pci_disable_pcie_error_reporting explicitly. does this make sense ? >> 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@xilinx.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? >> >>> + u16 control; >>> + >>> + /* >>> + * A Type-1 PCI bridge will not forward ERR_ messages coming >>> + * from an endpoint if SERR# forwarding is not enabled. >>> + */ >>> + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &control); >>> + control |= PCI_BRIDGE_CTL_SERR; >>> + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); >>> + } >>> + >>> return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, >>> PCI_EXP_AER_FLAGS); >>> } >>> EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting); >>> @@ -352,6 +365,16 @@ int pci_disable_pcie_error_reporting(struct >>> pci_dev *dev) >>> if (pcie_aer_get_firmware_first(dev)) >>> return -EIO; >>> >>> + if (!IS_ENABLED(CONFIG_ACPI) && >>> + dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) { >>> + u16 control; >>> + >>> + /* Clear SERR Forwarding */ >>> + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &control); >>> + control &= ~PCI_BRIDGE_CTL_SERR; >>> + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); >>> + } >>> + >>> return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, >>> PCI_EXP_AER_FLAGS); >>> } >>> -- >>> 1.7.1 >>>
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) { + u16 control; + + /* + * A Type-1 PCI bridge will not forward ERR_ messages coming + * from an endpoint if SERR# forwarding is not enabled. + */ + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &control); + control |= PCI_BRIDGE_CTL_SERR; + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); + } + return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS); } EXPORT_SYMBOL_GPL(pci_enable_pcie_error_reporting); @@ -352,6 +365,16 @@ int pci_disable_pcie_error_reporting(struct pci_dev *dev) if (pcie_aer_get_firmware_first(dev)) return -EIO; + if (!IS_ENABLED(CONFIG_ACPI) && + dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) { + u16 control; + + /* Clear SERR Forwarding */ + pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &control); + control &= ~PCI_BRIDGE_CTL_SERR; + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, control); + } + return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS); }
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. Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com> --- drivers/pci/pcie/aer.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)