Some PCIe devices do not have AER capabilities, but they have device
status registers.
Signed-off-by: Songyang Li <[email protected]>
---
drivers/pci/pci.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
mode change 100644 => 100755 drivers/pci/pci.c
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
old mode 100644
new mode 100755
index 35fb1f17a589..e6de55be4c45
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2263,7 +2263,12 @@ int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)
}
EXPORT_SYMBOL_GPL(pci_set_pcie_reset_state);
-#ifdef CONFIG_PCIEAER
+/**
+ * pcie_clear_device_status - Clear device status.
+ * @dev: the PCI device.
+ *
+ * Clear the device status for the PCI device.
+ */
void pcie_clear_device_status(struct pci_dev *dev)
{
u16 sta;
@@ -2271,7 +2276,6 @@ void pcie_clear_device_status(struct pci_dev *dev)
pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &sta);
pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta);
}
-#endif
/**
* pcie_clear_root_pme_status - Clear root port PME interrupt status.
--
2.34.1
On Tue, Jun 11, 2024 at 11:15:10PM +0800, Songyang Li wrote:
> Some PCIe devices do not have AER capabilities, but they have device
> status registers.
>
> Signed-off-by: Songyang Li <[email protected]>
Unindent this.
Add "()" after function names.
Please explain what this patch does and why we want it. I can see
from the patch that it makes it so pcie_clear_device_status() is
always compiled, but the commit log should say that and should say why
we need that.
> ---
> drivers/pci/pci.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> mode change 100644 => 100755 drivers/pci/pci.c
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> old mode 100644
> new mode 100755
> index 35fb1f17a589..e6de55be4c45
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2263,7 +2263,12 @@ int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)
> }
> EXPORT_SYMBOL_GPL(pci_set_pcie_reset_state);
>
> -#ifdef CONFIG_PCIEAER
> +/**
> + * pcie_clear_device_status - Clear device status.
> + * @dev: the PCI device.
> + *
> + * Clear the device status for the PCI device.
> + */
> void pcie_clear_device_status(struct pci_dev *dev)
> {
> u16 sta;
> @@ -2271,7 +2276,6 @@ void pcie_clear_device_status(struct pci_dev *dev)
> pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &sta);
> pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta);
> }
> -#endif
>
> /**
> * pcie_clear_root_pme_status - Clear root port PME interrupt status.
> --
> 2.34.1
>
On Tue, 11 Jun 2024 17:34:18 -0500, Bjorn Helgaas wrote:
> Unindent this.
> Add "()" after function names.
> Please explain what this patch does and why we want it. I can see
> from the patch that it makes it so pcie_clear_device_status() is
> always compiled, but the commit log should say that and should say why
> we need that.
Thank you for reminding to add "()" after function pcie_clear_device_status.
The following is a revised patch,and it explains why we need that.
Thanks,
Songyang Li
From 3c1340522565a44ef25d2045e5bee2c0bdb72b32 Mon Sep 17 00:00:00 2001
From: Songyang Li <[email protected]>
Date: Wed, 12 Jun 2024 22:29:51 +0800
Subject: [PATCH] PCI: Cancel compilation restrictions on function
pcie_clear_device_status()
Some PCIe devices do not have AER capabilities, but they have
device status registers. The PCIe device status register and
AER register are independent PCIe capabilities, so it is
unreasonable to use CONFIG_PCIEAER for compilation control of
pcie_clear_device_status(), although which is currently only
used in the "aer.c", "edr.c", and "err.c". Some operating system
configurations do not enable the AER feature, but still expect to
use the device status register for simple RAS. In this case,
pcie_clear_device_status() can be used to clear the device status
regs, so this patch can meet the requirements of this scenario.
Signed-off-by: Songyang Li <[email protected]>
---
drivers/pci/pci.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
mode change 100644 => 100755 drivers/pci/pci.c
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
old mode 100644
new mode 100755
index 35fb1f17a589..e6de55be4c45
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2263,7 +2263,12 @@ int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)
}
EXPORT_SYMBOL_GPL(pci_set_pcie_reset_state);
-#ifdef CONFIG_PCIEAER
+/**
+ * pcie_clear_device_status - Clear device status.
+ * @dev: the PCI device.
+ *
+ * Clear the device status for the PCI device.
+ */
void pcie_clear_device_status(struct pci_dev *dev)
{
u16 sta;
@@ -2271,7 +2276,6 @@ void pcie_clear_device_status(struct pci_dev *dev)
pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &sta);
pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta);
}
-#endif
/**
* pcie_clear_root_pme_status - Clear root port PME interrupt status.
--
2.34.1
On Wed, Jun 12, 2024 at 11:15:43PM +0800, Songyang Li wrote:
> On Tue, 11 Jun 2024 17:34:18 -0500, Bjorn Helgaas wrote:
> > Unindent this.
>
> > Add "()" after function names.
>
> > Please explain what this patch does and why we want it. I can see
> > from the patch that it makes it so pcie_clear_device_status() is
> > always compiled, but the commit log should say that and should say why
> > we need that.
>
> Thank you for reminding to add "()" after function pcie_clear_device_status.
> The following is a revised patch,and it explains why we need that.
> Thanks,
>
> Songyang Li
>
> From 3c1340522565a44ef25d2045e5bee2c0bdb72b32 Mon Sep 17 00:00:00 2001
> From: Songyang Li <[email protected]>
> Date: Wed, 12 Jun 2024 22:29:51 +0800
> Subject: [PATCH] PCI: Cancel compilation restrictions on function
> pcie_clear_device_status()
>
> Some PCIe devices do not have AER capabilities, but they have
> device status registers. The PCIe device status register and
> AER register are independent PCIe capabilities, so it is
> unreasonable to use CONFIG_PCIEAER for compilation control of
> pcie_clear_device_status(), although which is currently only
> used in the "aer.c", "edr.c", and "err.c". Some operating system
> configurations do not enable the AER feature, but still expect to
> use the device status register for simple RAS. In this case,
> pcie_clear_device_status() can be used to clear the device status
> regs, so this patch can meet the requirements of this scenario.
I think all current any callers of pcie_clear_device_status() are also
under CONFIG_PCIEAER, so I don't think this fixes a current problem.
As you point out, it might make sense to use
pcie_clear_device_status() even without AER, but I think we should
include this change at the time when we add such a use.
If I'm missing a use with the current kernel, let me know.
> Signed-off-by: Songyang Li <[email protected]>
> ---
> drivers/pci/pci.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> mode change 100644 => 100755 drivers/pci/pci.c
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> old mode 100644
> new mode 100755
> index 35fb1f17a589..e6de55be4c45
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2263,7 +2263,12 @@ int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)
> }
> EXPORT_SYMBOL_GPL(pci_set_pcie_reset_state);
>
> -#ifdef CONFIG_PCIEAER
> +/**
> + * pcie_clear_device_status - Clear device status.
> + * @dev: the PCI device.
> + *
> + * Clear the device status for the PCI device.
> + */
> void pcie_clear_device_status(struct pci_dev *dev)
> {
> u16 sta;
> @@ -2271,7 +2276,6 @@ void pcie_clear_device_status(struct pci_dev *dev)
> pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &sta);
> pcie_capability_write_word(dev, PCI_EXP_DEVSTA, sta);
> }
> -#endif
>
> /**
> * pcie_clear_root_pme_status - Clear root port PME interrupt status.
> --
> 2.34.1
>
On Wed, 12 Jun 2024 15:14:32 -0500, Bjorn Helgaas wrote:
> I think all current any callers of pcie_clear_device_status() are also
> under CONFIG_PCIEAER, so I don't think this fixes a current problem.
>
> As you point out, it might make sense to use
> pcie_clear_device_status() even without AER, but I think we should
> include this change at the time when we add such a use.
>
> If I'm missing a use with the current kernel, let me know.
As far as I know, some PCIe device drivers, for example,
[net/ethernet/broadcom/tg3.c],[net/ethernet/atheros/atl1c/atl1c_main.c],
which use the following code to clear the device status register,
pcie_capability_write_word(tp->pdev, PCI_EXP_DEVSTA,
PCI_EXP_DEVSTA_CED |
PCI_EXP_DEVSTA_NFED |
PCI_EXP_DEVSTA_FED |
PCI_EXP_DEVSTA_URD);
I think it may be more suitable to export the pcie_clear_device_status()
for use in the driver code.
Thanks,
Songyang Li
On Sat, Jun 15, 2024 at 11:13:07AM +0800, Songyang Li wrote:
> On Wed, 12 Jun 2024 15:14:32 -0500, Bjorn Helgaas wrote:
> > I think all current any callers of pcie_clear_device_status() are also
> > under CONFIG_PCIEAER, so I don't think this fixes a current problem.
> >
> > As you point out, it might make sense to use
> > pcie_clear_device_status() even without AER, but I think we should
> > include this change at the time when we add such a use.
> >
> > If I'm missing a use with the current kernel, let me know.
>
> As far as I know, some PCIe device drivers, for example,
> [net/ethernet/broadcom/tg3.c],[net/ethernet/atheros/atl1c/atl1c_main.c],
> which use the following code to clear the device status register,
> pcie_capability_write_word(tp->pdev, PCI_EXP_DEVSTA,
> PCI_EXP_DEVSTA_CED |
> PCI_EXP_DEVSTA_NFED |
> PCI_EXP_DEVSTA_FED |
> PCI_EXP_DEVSTA_URD);
> I think it may be more suitable to export the pcie_clear_device_status()
> for use in the driver code.
If we want to use this from drivers, it would make sense to do
something like this patch, and this patch could be part of a series to
call it from the drivers.
But at the same time, we should ask whether drivers should be clearing
this status themselves, or whether it should be done by the PCI core.
Bjorn