As per Figure 6-3 in PCIe r4.0, sec 6.2.6, ERR_ messages
will be forwarded from the secondary interface to the primary interface,
if the SERR# Enable bit in the Bridge Control register is set.
Currently PCI_BRIDGE_CTL_SERR is being enabled only in
ACPI flow.
This patch enables PCI_BRIDGE_CTL_SERR for Type-1 PCI device.
Signed-off-by: Bharat Kumar Gogada <[email protected]>
---
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..4fb0d24 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -343,6 +343,20 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev)
if (!dev->aer_cap)
return -EIO;
+ if (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);
+ if (!(control & PCI_BRIDGE_CTL_SERR)) {
+ 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 +366,15 @@ int pci_disable_pcie_error_reporting(struct pci_dev *dev)
if (pcie_aer_get_firmware_first(dev))
return -EIO;
+ if (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-09 20:27, Bharat Kumar Gogada wrote:
> As per Figure 6-3 in PCIe r4.0, sec 6.2.6, ERR_ messages
> will be forwarded from the secondary interface to the primary
> interface,
> if the SERR# Enable bit in the Bridge Control register is set.
> Currently PCI_BRIDGE_CTL_SERR is being enabled only in
> ACPI flow.
> This patch enables PCI_BRIDGE_CTL_SERR for Type-1 PCI device.
>
> Signed-off-by: Bharat Kumar Gogada <[email protected]>
> ---
> 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..4fb0d24 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -343,6 +343,20 @@ int pci_enable_pcie_error_reporting(struct pci_dev
> *dev)
> if (!dev->aer_cap)
> return -EIO;
>
> + if (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);
> + if (!(control & PCI_BRIDGE_CTL_SERR)) {
> + 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 +366,15 @@ int pci_disable_pcie_error_reporting(struct
> pci_dev *dev)
> if (pcie_aer_get_firmware_first(dev))
> return -EIO;
>
> + if (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);
> }
Hi Bjorn,
I made some comments on patchv1., same I am putting it across here.
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 ?
> Subject: Re: [PATCH v2] PCI/AER: Enable SERR# forwarding in non ACPI flow
>
> On 2018-08-09 20:27, Bharat Kumar Gogada wrote:
> > As per Figure 6-3 in PCIe r4.0, sec 6.2.6, ERR_ messages will be
> > forwarded from the secondary interface to the primary interface, if
> > the SERR# Enable bit in the Bridge Control register is set.
> > Currently PCI_BRIDGE_CTL_SERR is being enabled only in ACPI flow.
> > This patch enables PCI_BRIDGE_CTL_SERR for Type-1 PCI device.
> >
> > Signed-off-by: Bharat Kumar Gogada <[email protected]>
> > ---
> > 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..4fb0d24 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -343,6 +343,20 @@ int pci_enable_pcie_error_reporting(struct
> > pci_dev
> > *dev)
> > if (!dev->aer_cap)
> > return -EIO;
> >
> > + if (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);
> > + if (!(control & PCI_BRIDGE_CTL_SERR)) {
> > + 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 +366,15 @@ int pci_disable_pcie_error_reporting(struct
> > pci_dev *dev)
> > if (pcie_aer_get_firmware_first(dev))
> > return -EIO;
> >
> > + if (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);
> > }
>
>
> Hi Bjorn,
>
> I made some comments on patchv1., same I am putting it across here.
>
> 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 ?
>
Hi Bjorn,
Can you please comment on this.
Bharat
On Thu, Aug 09, 2018 at 08:27:37PM +0530, Bharat Kumar Gogada wrote:
> As per Figure 6-3 in PCIe r4.0, sec 6.2.6, ERR_ messages
> will be forwarded from the secondary interface to the primary interface,
> if the SERR# Enable bit in the Bridge Control register is set.
> Currently PCI_BRIDGE_CTL_SERR is being enabled only in
> ACPI flow.
> This patch enables PCI_BRIDGE_CTL_SERR for Type-1 PCI device.
I agree this is a problem. We should not depend on how firmware set
PCI_BRIDGE_CTL_SERR.
I would prefer to set it in the pci_configure_device() path so we
always do it the same way for every device, independent of whether AER
is enabled.
Some platform firmware must already enable PCI_BRIDGE_CTL_SERR, even
though it can't know whether the OS will support AER, so it should be
safe for the OS to enable it unconditionally.
The code in program_hpp_type0() that sets PCI_BRIDGE_CTL_SERR is based
on 40abb96c51bb ("[PATCH] pciehp: Fix programming hotplug
parameters") [1], but I think it is incorrect.
ACPI v6.0, sec 6.2.9.1, (_HPX PCI Setting Record) says:
If the hot plug device includes bridge(s) in the hierarchy, the
above settings apply to the primary side (command register) of the
hot plugged bridge(s). The settings for the secondary side of the
bridge(s) (Bridge Control Register) are assumed to be provided by
the bridge driver.
Sec 6.2.8 (for the old _HPP method) doesn't contain that text, but it
has the same description of the contents:
Enable SERR When set to 1, indicates that action must be
performed to enable SERR in the command register.
Both sections talk about enabling SERR in the *command* register (not
the bridge control register) and the _HPX section is explicit about
the fact that "Enable SERR" does not apply to the bridge control
register.
So I think pci_configure_device() should unconditionally set
PCI_BRIDGE_CTL_SERR for all bridge devices. And we should remove the
corresponding code from program_hpp_type0().
[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=40abb96c51bb
> Signed-off-by: Bharat Kumar Gogada <[email protected]>
> ---
> 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..4fb0d24 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -343,6 +343,20 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev)
> if (!dev->aer_cap)
> return -EIO;
>
> + if (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);
> + if (!(control & PCI_BRIDGE_CTL_SERR)) {
> + 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 +366,15 @@ int pci_disable_pcie_error_reporting(struct pci_dev *dev)
> if (pcie_aer_get_firmware_first(dev))
> return -EIO;
>
> + if (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 Thu, Aug 09, 2018 at 08:41:27PM +0530, [email protected] wrote:
> On 2018-08-09 20:27, Bharat Kumar Gogada wrote:
> > As per Figure 6-3 in PCIe r4.0, sec 6.2.6, ERR_ messages
> > will be forwarded from the secondary interface to the primary interface,
> > if the SERR# Enable bit in the Bridge Control register is set.
> > Currently PCI_BRIDGE_CTL_SERR is being enabled only in
> > ACPI flow.
> > This patch enables PCI_BRIDGE_CTL_SERR for Type-1 PCI device.
> >
> > Signed-off-by: Bharat Kumar Gogada <[email protected]>
> > ---
> > 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..4fb0d24 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -343,6 +343,20 @@ int pci_enable_pcie_error_reporting(struct pci_dev
> > *dev)
> > if (!dev->aer_cap)
> > return -EIO;
> >
> > + if (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);
> > + if (!(control & PCI_BRIDGE_CTL_SERR)) {
> > + 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 +366,15 @@ int pci_disable_pcie_error_reporting(struct pci_dev
> > *dev)
> > if (pcie_aer_get_firmware_first(dev))
> > return -EIO;
> >
> > + if (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);
> > }
>
>
> Hi Bjorn,
>
> I made some comments on patchv1., same I am putting it across here.
>
> 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 ?
I just replied to the original patch; sorry, I forgot to add you to
the cc list. Bharat, when people respond to your v<N> patch, can you
please add them (and anybody else *they* added) to the cc list when
you post your v<N+1> patch?
If we set PCI_BRIDGE_CTL_SERR in the pci_configure_device() path,
would that address your comments?
There's still a separate question of where and how we should configure
the error bits in PCI_EXP_DEVCTL (currently done in
pci_enable_pcie_error_reporting()).
Bjorn
On 2018-09-06 01:40, Bjorn Helgaas wrote:
> On Thu, Aug 09, 2018 at 08:41:27PM +0530, [email protected] wrote:
>> On 2018-08-09 20:27, Bharat Kumar Gogada wrote:
>> > As per Figure 6-3 in PCIe r4.0, sec 6.2.6, ERR_ messages
>> > will be forwarded from the secondary interface to the primary interface,
>> > if the SERR# Enable bit in the Bridge Control register is set.
>> > Currently PCI_BRIDGE_CTL_SERR is being enabled only in
>> > ACPI flow.
>> > This patch enables PCI_BRIDGE_CTL_SERR for Type-1 PCI device.
>> >
>> > Signed-off-by: Bharat Kumar Gogada <[email protected]>
>> > ---
>> > 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..4fb0d24 100644
>> > --- a/drivers/pci/pcie/aer.c
>> > +++ b/drivers/pci/pcie/aer.c
>> > @@ -343,6 +343,20 @@ int pci_enable_pcie_error_reporting(struct pci_dev
>> > *dev)
>> > if (!dev->aer_cap)
>> > return -EIO;
>> >
>> > + if (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);
>> > + if (!(control & PCI_BRIDGE_CTL_SERR)) {
>> > + 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 +366,15 @@ int pci_disable_pcie_error_reporting(struct pci_dev
>> > *dev)
>> > if (pcie_aer_get_firmware_first(dev))
>> > return -EIO;
>> >
>> > + if (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);
>> > }
>>
>>
>> Hi Bjorn,
>>
>> I made some comments on patchv1., same I am putting it across here.
>>
>> 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 ?
>
> I just replied to the original patch; sorry, I forgot to add you to
> the cc list. Bharat, when people respond to your v<N> patch, can you
> please add them (and anybody else *they* added) to the cc list when
> you post your v<N+1> patch?
>
> If we set PCI_BRIDGE_CTL_SERR in the pci_configure_device() path,
> would that address your comments?
yes, that should do.
>
> There's still a separate question of where and how we should configure
> the error bits in PCI_EXP_DEVCTL (currently done in
> pci_enable_pcie_error_reporting()).
>
> Bjorn