| Message ID | TY3P286MB2754E7F2E61B266F610E8876B4C72@TY3P286MB2754.JPNP286.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Cancel compilation restrictions on function pcie_clear_device_status | expand |
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 >
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.
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 >
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
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
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
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
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.
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