Re: [PATCH] PCI/AER: Enable SERR# forwarding in non ACPI flow

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
>> > ---
>> >  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



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux