| Message ID | 1533826657-24552-1-git-send-email-bharat.kumar.gogada@xilinx.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] PCI/AER: Enable SERR# forwarding in non ACPI flow | expand |
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 <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..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 <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..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 <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..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, poza@codeaurora.org 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 <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..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, poza@codeaurora.org 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 <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..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
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); }
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 <bharat.kumar.gogada@xilinx.com> --- drivers/pci/pcie/aer.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)