| Message ID | 20220901181634.99591-4-chenzhuo.1@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/AER: Fix and optimize usage of status clear api | expand |
On Fri, Sep 02, 2022 at 02:16:34AM +0800, Zhuo Chen wrote: > Statements clearing AER error status in aer_enable_rootport() has the > same function as pci_aer_raw_clear_status(). So we replace them, which > has no functional changes. > > Signed-off-by: Zhuo Chen <chenzhuo.1@bytedance.com> > --- > drivers/pci/pcie/aer.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index d2996afa80f6..eb0193f279f2 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -1287,12 +1287,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc) > SYSTEM_ERROR_INTR_ON_MESG_MASK); > > /* Clear error status */ > - pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, ®32); > - pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, reg32); > - pci_read_config_dword(pdev, aer + PCI_ERR_COR_STATUS, ®32); > - pci_write_config_dword(pdev, aer + PCI_ERR_COR_STATUS, reg32); > - pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, ®32); > - pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32); > + pci_aer_raw_clear_status(pdev); It's true that this is functionally equivalent. But 20e15e673b05 ("PCI/AER: Add pci_aer_raw_clear_status() to unconditionally clear Error Status") says pci_aer_raw_clear_status() is only for use in the EDR path (this should have been included in the function comment), so I think we should preserve that property and use pci_aer_clear_status() here. pci_aer_raw_clear_status() is the same as pci_aer_clear_status() except it doesn't check pcie_aer_is_native(). And I'm pretty sure we can't get to aer_enable_rootport() *unless* pcie_aer_is_native(), because get_port_device_capability() checks the same thing, so they should be equivalent here. Bjorn
On 9/23/22 5:50 AM, Bjorn Helgaas wrote: > On Fri, Sep 02, 2022 at 02:16:34AM +0800, Zhuo Chen wrote: >> Statements clearing AER error status in aer_enable_rootport() has the >> same function as pci_aer_raw_clear_status(). So we replace them, which >> has no functional changes. >> >> Signed-off-by: Zhuo Chen <chenzhuo.1@bytedance.com> >> --- >> drivers/pci/pcie/aer.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c >> index d2996afa80f6..eb0193f279f2 100644 >> --- a/drivers/pci/pcie/aer.c >> +++ b/drivers/pci/pcie/aer.c >> @@ -1287,12 +1287,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc) >> SYSTEM_ERROR_INTR_ON_MESG_MASK); >> >> /* Clear error status */ >> - pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, ®32); >> - pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, reg32); >> - pci_read_config_dword(pdev, aer + PCI_ERR_COR_STATUS, ®32); >> - pci_write_config_dword(pdev, aer + PCI_ERR_COR_STATUS, reg32); >> - pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, ®32); >> - pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32); >> + pci_aer_raw_clear_status(pdev); > > It's true that this is functionally equivalent. > > But 20e15e673b05 ("PCI/AER: Add pci_aer_raw_clear_status() to > unconditionally clear Error Status") says pci_aer_raw_clear_status() > is only for use in the EDR path (this should have been included in the > function comment), so I think we should preserve that property and use > pci_aer_clear_status() here. > > pci_aer_raw_clear_status() is the same as pci_aer_clear_status() > except it doesn't check pcie_aer_is_native(). And I'm pretty sure we > can't get to aer_enable_rootport() *unless* pcie_aer_is_native(), > because get_port_device_capability() checks the same thing, so they > should be equivalent here. > > Bjorn Thanks Bjorn, this very detailed correction is helpful. By the way, 'only for use in the EDR path' obviously written in the function comments may be better. So far only commit log has included these. I will change to use pci_aer_clear_status() in next patch.
On Mon, Sep 26, 2022 at 10:16:23PM +0800, Zhuo Chen wrote: > On 9/23/22 5:50 AM, Bjorn Helgaas wrote: > > On Fri, Sep 02, 2022 at 02:16:34AM +0800, Zhuo Chen wrote: > > > Statements clearing AER error status in aer_enable_rootport() has the > > > same function as pci_aer_raw_clear_status(). So we replace them, which > > > has no functional changes. > > > - pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, ®32); > > > - pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, reg32); > > > - pci_read_config_dword(pdev, aer + PCI_ERR_COR_STATUS, ®32); > > > - pci_write_config_dword(pdev, aer + PCI_ERR_COR_STATUS, reg32); > > > - pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, ®32); > > > - pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32); > > > + pci_aer_raw_clear_status(pdev); > > > > It's true that this is functionally equivalent. > > > > But 20e15e673b05 ("PCI/AER: Add pci_aer_raw_clear_status() to > > unconditionally clear Error Status") says pci_aer_raw_clear_status() > > is only for use in the EDR path (this should have been included in the > > function comment), so I think we should preserve that property and use > > pci_aer_clear_status() here. > > > > pci_aer_raw_clear_status() is the same as pci_aer_clear_status() > > except it doesn't check pcie_aer_is_native(). And I'm pretty sure we > > can't get to aer_enable_rootport() *unless* pcie_aer_is_native(), > > because get_port_device_capability() checks the same thing, so they > > should be equivalent here. > > > Thanks Bjorn, this very detailed correction is helpful. By the way, 'only > for use in the EDR path' obviously written in the function comments may be > better. So far only commit log has included these. Yes, definitely! I goofed when I applied that patch without making sure there was something in the function comment. Bjorn
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index d2996afa80f6..eb0193f279f2 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -1287,12 +1287,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc) SYSTEM_ERROR_INTR_ON_MESG_MASK); /* Clear error status */ - pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, ®32); - pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, reg32); - pci_read_config_dword(pdev, aer + PCI_ERR_COR_STATUS, ®32); - pci_write_config_dword(pdev, aer + PCI_ERR_COR_STATUS, reg32); - pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, ®32); - pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32); + pci_aer_raw_clear_status(pdev); /* * Enable error reporting for the root port device and downstream port
Statements clearing AER error status in aer_enable_rootport() has the same function as pci_aer_raw_clear_status(). So we replace them, which has no functional changes. Signed-off-by: Zhuo Chen <chenzhuo.1@bytedance.com> --- drivers/pci/pcie/aer.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)