[PATCH] PCI: hotplug: Use atomic_{fetch_}andnot() where appropriate

[PATCH] PCI: hotplug: Use atomic_{fetch_}andnot() where appropriate

[Uros Bizjak]

Use atomic_{fetch_}andnot(i, v) instead of atomic_{fetch_}and(~i, v).

No functional changes intended.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 4 ++--
 drivers/pci/hotplug/pciehp_hpc.c  | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index dcdbfcf404dd..7c775d9a6599 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -121,8 +121,8 @@ static void remove_board(struct controller *ctrl, bool safe_removal)
 		msleep(1000);
 
 		/* Ignore link or presence changes caused by power off */
-		atomic_and(~(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC),
-			   &ctrl->pending_events);
+		atomic_andnot(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC,
+			      &ctrl->pending_events);
 	}
 
 	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index b1d0a1b3917d..6d192f64ea19 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -307,8 +307,8 @@ int pciehp_check_link_status(struct controller *ctrl)
 
 	/* ignore link or presence changes up to this point */
 	if (found)
-		atomic_and(~(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC),
-			   &ctrl->pending_events);
+		atomic_andnot(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC,
+			      &ctrl->pending_events);
 
 	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
 	ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
@@ -568,7 +568,7 @@ static void pciehp_ignore_dpc_link_change(struct controller *ctrl,
 	 * Could be several if DPC triggered multiple times consecutively.
 	 */
 	synchronize_hardirq(irq);
-	atomic_and(~PCI_EXP_SLTSTA_DLLSC, &ctrl->pending_events);
+	atomic_andnot(PCI_EXP_SLTSTA_DLLSC, &ctrl->pending_events);
 	if (pciehp_poll_mode)
 		pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
 					   PCI_EXP_SLTSTA_DLLSC);
@@ -702,7 +702,7 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
 	pci_config_pm_runtime_get(pdev);
 
 	/* rerun pciehp_isr() if the port was inaccessible on interrupt */
-	if (atomic_fetch_and(~RERUN_ISR, &ctrl->pending_events) & RERUN_ISR) {
+	if (atomic_fetch_andnot(RERUN_ISR, &ctrl->pending_events) & RERUN_ISR) {
 		ret = pciehp_isr(irq, dev_id);
 		enable_irq(irq);
 		if (ret != IRQ_WAKE_THREAD)
-- 
2.45.2

Re: [PATCH] PCI: hotplug: Use atomic_{fetch_}andnot() where appropriate

[Bjorn Helgaas]

[+cc Lukas, Ilpo]

On Thu, Jun 13, 2024 at 10:24:24AM +0200, Uros Bizjak wrote:
> Use atomic_{fetch_}andnot(i, v) instead of atomic_{fetch_}and(~i, v).

If the purpose is to improve readability, let's mention that here.
Since this only touches pciehp, make the subject line "PCI: pciehp:
..." as was done in the past.

It looks like every use of atomic_and() uses a ~value and is hence a
candidate for a similar change, but I'm not sure that converting to
"andnot" and removing the explicit bitwise NOT is really a readability
benefit.

If it were named something like "atomic_clear_bits", I'd be totally in
favor since that's a little higher-level description, but that ship
has long since sailed.

But I don't really care and if this is the trend, I'm fine with this
if Lukas and Ilpo agree.

> No functional changes intended.
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 4 ++--
>  drivers/pci/hotplug/pciehp_hpc.c  | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index dcdbfcf404dd..7c775d9a6599 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -121,8 +121,8 @@ static void remove_board(struct controller *ctrl, bool safe_removal)
>  		msleep(1000);
>  
>  		/* Ignore link or presence changes caused by power off */
> -		atomic_and(~(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC),
> -			   &ctrl->pending_events);
> +		atomic_andnot(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC,
> +			      &ctrl->pending_events);
>  	}
>  
>  	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index b1d0a1b3917d..6d192f64ea19 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -307,8 +307,8 @@ int pciehp_check_link_status(struct controller *ctrl)
>  
>  	/* ignore link or presence changes up to this point */
>  	if (found)
> -		atomic_and(~(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC),
> -			   &ctrl->pending_events);
> +		atomic_andnot(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC,
> +			      &ctrl->pending_events);
>  
>  	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
>  	ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
> @@ -568,7 +568,7 @@ static void pciehp_ignore_dpc_link_change(struct controller *ctrl,
>  	 * Could be several if DPC triggered multiple times consecutively.
>  	 */
>  	synchronize_hardirq(irq);
> -	atomic_and(~PCI_EXP_SLTSTA_DLLSC, &ctrl->pending_events);
> +	atomic_andnot(PCI_EXP_SLTSTA_DLLSC, &ctrl->pending_events);
>  	if (pciehp_poll_mode)
>  		pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
>  					   PCI_EXP_SLTSTA_DLLSC);
> @@ -702,7 +702,7 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
>  	pci_config_pm_runtime_get(pdev);
>  
>  	/* rerun pciehp_isr() if the port was inaccessible on interrupt */
> -	if (atomic_fetch_and(~RERUN_ISR, &ctrl->pending_events) & RERUN_ISR) {
> +	if (atomic_fetch_andnot(RERUN_ISR, &ctrl->pending_events) & RERUN_ISR) {
>  		ret = pciehp_isr(irq, dev_id);
>  		enable_irq(irq);
>  		if (ret != IRQ_WAKE_THREAD)
> -- 
> 2.45.2
> 

Re: [PATCH] PCI: hotplug: Use atomic_{fetch_}andnot() where appropriate

[Ilpo Järvinen]

On Thu, 13 Jun 2024, Bjorn Helgaas wrote:

> [+cc Lukas, Ilpo]
> 
> On Thu, Jun 13, 2024 at 10:24:24AM +0200, Uros Bizjak wrote:
> > Use atomic_{fetch_}andnot(i, v) instead of atomic_{fetch_}and(~i, v).
> 
> If the purpose is to improve readability, let's mention that here.
> Since this only touches pciehp, make the subject line "PCI: pciehp:
> ..." as was done in the past.
> 
> It looks like every use of atomic_and() uses a ~value and is hence a
> candidate for a similar change, but I'm not sure that converting to
> "andnot" and removing the explicit bitwise NOT is really a readability
> benefit.
> 
> If it were named something like "atomic_clear_bits", I'd be totally in
> favor since that's a little higher-level description, but that ship
> has long since sailed.
> 
> But I don't really care and if this is the trend, I'm fine with this
> if Lukas and Ilpo agree.

I'm pretty much aligned with Bjorn, I don't find it clearer but I don't 
feel too strongly now that I've seen how to interpret this. As he pointed 
out, there would have been much better names for this operation ("andnot"
feels similar to using double negations which easily gets confusing unless
one maps it inside head into positive logic).

-- 
 i.

> > No functional changes intended.
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > ---
> >  drivers/pci/hotplug/pciehp_ctrl.c | 4 ++--
> >  drivers/pci/hotplug/pciehp_hpc.c  | 8 ++++----
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> > index dcdbfcf404dd..7c775d9a6599 100644
> > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> > @@ -121,8 +121,8 @@ static void remove_board(struct controller *ctrl, bool safe_removal)
> >  		msleep(1000);
> >  
> >  		/* Ignore link or presence changes caused by power off */
> > -		atomic_and(~(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC),
> > -			   &ctrl->pending_events);
> > +		atomic_andnot(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC,
> > +			      &ctrl->pending_events);
> >  	}
> >  
> >  	pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> > index b1d0a1b3917d..6d192f64ea19 100644
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -307,8 +307,8 @@ int pciehp_check_link_status(struct controller *ctrl)
> >  
> >  	/* ignore link or presence changes up to this point */
> >  	if (found)
> > -		atomic_and(~(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC),
> > -			   &ctrl->pending_events);
> > +		atomic_andnot(PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC,
> > +			      &ctrl->pending_events);
> >  
> >  	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
> >  	ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
> > @@ -568,7 +568,7 @@ static void pciehp_ignore_dpc_link_change(struct controller *ctrl,
> >  	 * Could be several if DPC triggered multiple times consecutively.
> >  	 */
> >  	synchronize_hardirq(irq);
> > -	atomic_and(~PCI_EXP_SLTSTA_DLLSC, &ctrl->pending_events);
> > +	atomic_andnot(PCI_EXP_SLTSTA_DLLSC, &ctrl->pending_events);
> >  	if (pciehp_poll_mode)
> >  		pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
> >  					   PCI_EXP_SLTSTA_DLLSC);
> > @@ -702,7 +702,7 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
> >  	pci_config_pm_runtime_get(pdev);
> >  
> >  	/* rerun pciehp_isr() if the port was inaccessible on interrupt */
> > -	if (atomic_fetch_and(~RERUN_ISR, &ctrl->pending_events) & RERUN_ISR) {
> > +	if (atomic_fetch_andnot(RERUN_ISR, &ctrl->pending_events) & RERUN_ISR) {
> >  		ret = pciehp_isr(irq, dev_id);
> >  		enable_irq(irq);
> >  		if (ret != IRQ_WAKE_THREAD)
> > -- 
> > 2.45.2
> > 
> 

Re: [PATCH] PCI: hotplug: Use atomic_{fetch_}andnot() where appropriate

[Uros Bizjak]

On Thu, Jun 13, 2024 at 5:51 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Lukas, Ilpo]
>
> On Thu, Jun 13, 2024 at 10:24:24AM +0200, Uros Bizjak wrote:
> > Use atomic_{fetch_}andnot(i, v) instead of atomic_{fetch_}and(~i, v).
>
> If the purpose is to improve readability, let's mention that here.
> Since this only touches pciehp, make the subject line "PCI: pciehp:
> ..." as was done in the past.
>
> It looks like every use of atomic_and() uses a ~value and is hence a
> candidate for a similar change, but I'm not sure that converting to
> "andnot" and removing the explicit bitwise NOT is really a readability
> benefit.
>
> If it were named something like "atomic_clear_bits", I'd be totally in
> favor since that's a little higher-level description, but that ship
> has long since sailed.

FYI, the set of atomic primitives and their corresponding names have
quite a long history. These are based on IA-64 psABI [1, section 7.4]
when this particular primitive was named
__sync_nand_and_fetch/__sync_fetch_and_nand. Even GCC got the and-not
part wrong (it implemented it as not-and in some ancient version), so
luckily the kernel named it atomic_{fetch_}andnot.

As far as the patch is concerned, some architectures emit atomic
andnot instruction. In the proposed patch, we have a constant argument
to atomic_and, and the compiler is smart enough to apply the bitwise
not to the argument and emits an inverted constant argument. So, in
reality, the same code is produced.

x86 with BMI1 ISA extension provides ANDN instruction, but it can't be
used with LOCK prefix.

So, if there is no readability benefit, it is OK with me to drop the patch.

[1] https://refspecs.linuxfoundation.org/elf/IA64-SysV-psABI.pdf

Thanks,
Uros.