* [PATCH v3] PCI/AER: Use pci_clear_and_set_config_dword() to simplify mask updates
@ 2025-08-16 16:17 Hans Zhang
2025-08-16 20:25 ` Lukas Wunner
0 siblings, 1 reply; 3+ messages in thread
From: Hans Zhang @ 2025-08-16 16:17 UTC (permalink / raw)
To: mahesh, bhelgaas
Cc: oohall, mani, linuxppc-dev, linux-pci, linux-kernel, Hans Zhang
Replace manual read-modify-write sequences in multiple functions with
pci_clear_and_set_config_dword() helper to reduce code duplication.
Signed-off-by: Hans Zhang <18255117159@163.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
---
Changes for v3:
- Rebase to v6.17-rc1.
- The patch commit message were modified.
- Add Acked-by: Manivannan Sadhasivam <mani@kernel.org>
Changes for v2:
- The patch commit message were modified.
- New optimizations for the functions disable_ecrc_checking, aer_enable_irq, and aer_disable_irq have been added.
---
drivers/pci/pcie/aer.c | 30 +++++++++++-------------------
1 file changed, 11 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index e286c197d716..3d37e2b7e412 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -176,14 +176,13 @@ static int enable_ecrc_checking(struct pci_dev *dev)
static int disable_ecrc_checking(struct pci_dev *dev)
{
int aer = dev->aer_cap;
- u32 reg32;
if (!aer)
return -ENODEV;
- pci_read_config_dword(dev, aer + PCI_ERR_CAP, ®32);
- reg32 &= ~(PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE);
- pci_write_config_dword(dev, aer + PCI_ERR_CAP, reg32);
+ pci_clear_and_set_config_dword(dev, aer + PCI_ERR_CAP,
+ PCI_ERR_CAP_ECRC_GENE |
+ PCI_ERR_CAP_ECRC_CHKE, 0);
return 0;
}
@@ -1102,15 +1101,12 @@ static bool find_source_device(struct pci_dev *parent,
static void pci_aer_unmask_internal_errors(struct pci_dev *dev)
{
int aer = dev->aer_cap;
- u32 mask;
- pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, &mask);
- mask &= ~PCI_ERR_UNC_INTN;
- pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, mask);
+ pci_clear_and_set_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,
+ PCI_ERR_UNC_INTN, 0);
- pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK, &mask);
- mask &= ~PCI_ERR_COR_INTERNAL;
- pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask);
+ pci_clear_and_set_config_dword(dev, aer + PCI_ERR_COR_MASK,
+ PCI_ERR_COR_INTERNAL, 0);
}
static bool is_cxl_mem_dev(struct pci_dev *dev)
@@ -1556,23 +1552,19 @@ static irqreturn_t aer_irq(int irq, void *context)
static void aer_enable_irq(struct pci_dev *pdev)
{
int aer = pdev->aer_cap;
- u32 reg32;
/* Enable Root Port's interrupt in response to error messages */
- pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, ®32);
- reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
- pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+ pci_clear_and_set_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND,
+ 0, ROOT_PORT_INTR_ON_MESG_MASK);
}
static void aer_disable_irq(struct pci_dev *pdev)
{
int aer = pdev->aer_cap;
- u32 reg32;
/* Disable Root Port's interrupt in response to error messages */
- pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, ®32);
- reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
- pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+ pci_clear_and_set_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND,
+ ROOT_PORT_INTR_ON_MESG_MASK, 0);
}
/**
base-commit: 8742b2d8935f476449ef37e263bc4da3295c7b58
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] PCI/AER: Use pci_clear_and_set_config_dword() to simplify mask updates
2025-08-16 16:17 [PATCH v3] PCI/AER: Use pci_clear_and_set_config_dword() to simplify mask updates Hans Zhang
@ 2025-08-16 20:25 ` Lukas Wunner
2025-08-17 15:07 ` Hans Zhang
0 siblings, 1 reply; 3+ messages in thread
From: Lukas Wunner @ 2025-08-16 20:25 UTC (permalink / raw)
To: Hans Zhang
Cc: mahesh, bhelgaas, oohall, mani, linuxppc-dev, linux-pci,
linux-kernel
On Sun, Aug 17, 2025 at 12:17:43AM +0800, Hans Zhang wrote:
> Replace manual read-modify-write sequences in multiple functions with
> pci_clear_and_set_config_dword() helper to reduce code duplication.
None of the occurrences you're replacing is clearing *and* setting
bits at the same time. They all either clear or set bits, but not both.
For the PCIe Capability, there are pcie_capability_clear_dword()
and pcie_capability_set_dword() helpers.
It would arguably be clearer and less confusing to introduce similar
pci_clear_config_dword() and pci_set_config_dword() helpers and use
those, instead of using pci_clear_and_set_config_dword() and setting
one argument to 0.
Thanks,
Lukas
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index e286c197d716..3d37e2b7e412 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -176,14 +176,13 @@ static int enable_ecrc_checking(struct pci_dev *dev)
> static int disable_ecrc_checking(struct pci_dev *dev)
> {
> int aer = dev->aer_cap;
> - u32 reg32;
>
> if (!aer)
> return -ENODEV;
>
> - pci_read_config_dword(dev, aer + PCI_ERR_CAP, ®32);
> - reg32 &= ~(PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE);
> - pci_write_config_dword(dev, aer + PCI_ERR_CAP, reg32);
> + pci_clear_and_set_config_dword(dev, aer + PCI_ERR_CAP,
> + PCI_ERR_CAP_ECRC_GENE |
> + PCI_ERR_CAP_ECRC_CHKE, 0);
>
> return 0;
> }
> @@ -1102,15 +1101,12 @@ static bool find_source_device(struct pci_dev *parent,
> static void pci_aer_unmask_internal_errors(struct pci_dev *dev)
> {
> int aer = dev->aer_cap;
> - u32 mask;
>
> - pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, &mask);
> - mask &= ~PCI_ERR_UNC_INTN;
> - pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, mask);
> + pci_clear_and_set_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,
> + PCI_ERR_UNC_INTN, 0);
>
> - pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK, &mask);
> - mask &= ~PCI_ERR_COR_INTERNAL;
> - pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask);
> + pci_clear_and_set_config_dword(dev, aer + PCI_ERR_COR_MASK,
> + PCI_ERR_COR_INTERNAL, 0);
> }
>
> static bool is_cxl_mem_dev(struct pci_dev *dev)
> @@ -1556,23 +1552,19 @@ static irqreturn_t aer_irq(int irq, void *context)
> static void aer_enable_irq(struct pci_dev *pdev)
> {
> int aer = pdev->aer_cap;
> - u32 reg32;
>
> /* Enable Root Port's interrupt in response to error messages */
> - pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, ®32);
> - reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> - pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> + pci_clear_and_set_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND,
> + 0, ROOT_PORT_INTR_ON_MESG_MASK);
> }
>
> static void aer_disable_irq(struct pci_dev *pdev)
> {
> int aer = pdev->aer_cap;
> - u32 reg32;
>
> /* Disable Root Port's interrupt in response to error messages */
> - pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, ®32);
> - reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> - pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> + pci_clear_and_set_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND,
> + ROOT_PORT_INTR_ON_MESG_MASK, 0);
> }
>
> /**
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] PCI/AER: Use pci_clear_and_set_config_dword() to simplify mask updates
2025-08-16 20:25 ` Lukas Wunner
@ 2025-08-17 15:07 ` Hans Zhang
0 siblings, 0 replies; 3+ messages in thread
From: Hans Zhang @ 2025-08-17 15:07 UTC (permalink / raw)
To: Lukas Wunner
Cc: mahesh, bhelgaas, oohall, mani, linuxppc-dev, linux-pci,
linux-kernel
On 2025/8/17 04:25, Lukas Wunner wrote:
> On Sun, Aug 17, 2025 at 12:17:43AM +0800, Hans Zhang wrote:
>> Replace manual read-modify-write sequences in multiple functions with
>> pci_clear_and_set_config_dword() helper to reduce code duplication.
>
> None of the occurrences you're replacing is clearing *and* setting
> bits at the same time. They all either clear or set bits, but not both.
>
> For the PCIe Capability, there are pcie_capability_clear_dword()
> and pcie_capability_set_dword() helpers.
>
> It would arguably be clearer and less confusing to introduce similar
> pci_clear_config_dword() and pci_set_config_dword() helpers and use
> those, instead of using pci_clear_and_set_config_dword() and setting
> one argument to 0.
>
Dear Lukas,
Thank you very much for your reply and suggestions.
In the next version, I will introduce two helper functions,
pci_clear_config_dword() and pci_set_config_dword().
Best regards,
Hans
> Thanks,
>
> Lukas
>
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index e286c197d716..3d37e2b7e412 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -176,14 +176,13 @@ static int enable_ecrc_checking(struct pci_dev *dev)
>> static int disable_ecrc_checking(struct pci_dev *dev)
>> {
>> int aer = dev->aer_cap;
>> - u32 reg32;
>>
>> if (!aer)
>> return -ENODEV;
>>
>> - pci_read_config_dword(dev, aer + PCI_ERR_CAP, ®32);
>> - reg32 &= ~(PCI_ERR_CAP_ECRC_GENE | PCI_ERR_CAP_ECRC_CHKE);
>> - pci_write_config_dword(dev, aer + PCI_ERR_CAP, reg32);
>> + pci_clear_and_set_config_dword(dev, aer + PCI_ERR_CAP,
>> + PCI_ERR_CAP_ECRC_GENE |
>> + PCI_ERR_CAP_ECRC_CHKE, 0);
>>
>> return 0;
>> }
>> @@ -1102,15 +1101,12 @@ static bool find_source_device(struct pci_dev *parent,
>> static void pci_aer_unmask_internal_errors(struct pci_dev *dev)
>> {
>> int aer = dev->aer_cap;
>> - u32 mask;
>>
>> - pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, &mask);
>> - mask &= ~PCI_ERR_UNC_INTN;
>> - pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, mask);
>> + pci_clear_and_set_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,
>> + PCI_ERR_UNC_INTN, 0);
>>
>> - pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK, &mask);
>> - mask &= ~PCI_ERR_COR_INTERNAL;
>> - pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask);
>> + pci_clear_and_set_config_dword(dev, aer + PCI_ERR_COR_MASK,
>> + PCI_ERR_COR_INTERNAL, 0);
>> }
>>
>> static bool is_cxl_mem_dev(struct pci_dev *dev)
>> @@ -1556,23 +1552,19 @@ static irqreturn_t aer_irq(int irq, void *context)
>> static void aer_enable_irq(struct pci_dev *pdev)
>> {
>> int aer = pdev->aer_cap;
>> - u32 reg32;
>>
>> /* Enable Root Port's interrupt in response to error messages */
>> - pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, ®32);
>> - reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
>> - pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
>> + pci_clear_and_set_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND,
>> + 0, ROOT_PORT_INTR_ON_MESG_MASK);
>> }
>>
>> static void aer_disable_irq(struct pci_dev *pdev)
>> {
>> int aer = pdev->aer_cap;
>> - u32 reg32;
>>
>> /* Disable Root Port's interrupt in response to error messages */
>> - pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, ®32);
>> - reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
>> - pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND, reg32);
>> + pci_clear_and_set_config_dword(pdev, aer + PCI_ERR_ROOT_COMMAND,
>> + ROOT_PORT_INTR_ON_MESG_MASK, 0);
>> }
>>
>> /**
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-08-17 15:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-16 16:17 [PATCH v3] PCI/AER: Use pci_clear_and_set_config_dword() to simplify mask updates Hans Zhang
2025-08-16 20:25 ` Lukas Wunner
2025-08-17 15:07 ` Hans Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).