[PATCH] PCI: Cancel compilation restrictions on function pcie_clear_device_status

[PATCH] PCI: Cancel compilation restrictions on function pcie_clear_device_status

[Songyang Li]

    Some PCIe devices do not have AER capabilities, but they have device
    status registers.

    Signed-off-by: Songyang Li <leesongyang@outlook.com>
---
 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

Re: [PATCH] PCI: Cancel compilation restrictions on function pcie_clear_device_status

[Bjorn Helgaas]

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 <leesongyang@outlook.com>

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
> 

Re: [PATCH] PCI: Cancel compilation restrictions on function pcie_clear_device_status

[Songyang Li]

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 <leesongyang@outlook.com>
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 <leesongyang@outlook.com>
---
 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

Re: [PATCH] PCI: Cancel compilation restrictions on function pcie_clear_device_status

[Bjorn Helgaas]

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 <leesongyang@outlook.com>
> 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 <leesongyang@outlook.com>
> ---
>  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
> 

Re: [PATCH] PCI: Cancel compilation restrictions on function pcie_clear_device_status

[Songyang Li]

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

Re: [PATCH] PCI: Cancel compilation restrictions on function pcie_clear_device_status

[Bjorn Helgaas]

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

Re: [PATCH] PCI: Cancel compilation restrictions on function pcie_clear_device_status

[Songyang Li]

On Sat, 15 Jun 2024 16:26:03 -0500, Bjorn Helgaas 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.

After careful consideration, I agree with your point of view.
I hold a viewpoint that it should be done by the PCI core,
rather than pcie drivers. I give up this patch, and then I have
gained a profound understanding of PCIe Core from this communication.
Thanks,

Songyang Li

Re: [PATCH] PCI: Cancel compilation restrictions on function pcie_clear_device_status

[Bjorn Helgaas]

On Mon, Jun 17, 2024 at 09:35:20PM +0800, Songyang Li wrote:
> On Sat, 15 Jun 2024 16:26:03 -0500, Bjorn Helgaas 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.
> 
> After careful consideration, I agree with your point of view.
> I hold a viewpoint that it should be done by the PCI core,
> rather than pcie drivers. I give up this patch, and then I have
> gained a profound understanding of PCIe Core from this communication.

I tend to think this should be done by the PCI core, but I haven't
looked at it enough to know how or where.  If you pursue it, I'd love
to see your ideas!

Thanks,
  Bjorn

Re: [PATCH] PCI: Cancel compilation restrictions on function pcie_clear_device_status

[Songyang Li]

On Mon, 17 Jun 2024 11:31:06 -0500, Bjorn Helgaas wrote:
>> On Sat, 15 Jun 2024 16:26:03 -0500, Bjorn Helgaas 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.
>> 
>> After careful consideration, I agree with your point of view.
>> I hold a viewpoint that it should be done by the PCI core,
>> rather than pcie drivers. I give up this patch, and then I have
>> gained a profound understanding of PCIe Core from this communication.
>
>I tend to think this should be done by the PCI core, but I haven't
>looked at it enough to know how or where.  If you pursue it, I'd love
>to see your ideas!

I have used the device status regs of PCIe as the basis for error detection,
which is used for PCIe devices without AER capability. But the current
solution is not universal and I look forward to submitting to the
community in the future.

Songyang Li