Received-SPF: pass (google.com: domain of linux-kernel+bounces-213627-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Thu, 13 Jun 2024 19:15:23 +0300 (EEST)
To: Bjorn Helgaas <helgaas@kernel.org>
cc: Uros Bizjak <ubizjak@gmail.com>, linux-pci@vger.kernel.org, 
    LKML <linux-kernel@vger.kernel.org>, Bjorn Helgaas <bhelgaas@google.com>, 
    Lukas Wunner <lukas@wunner.de>
Subject: Re: [PATCH] PCI: hotplug: Use atomic_{fetch_}andnot() where
 appropriate
In-Reply-To: <20240613155134.GA1062951@bhelgaas>
Message-ID: <aa9d3320-d31a-1577-cf55-9473864bd04c@linux.intel.com>
References: <20240613155134.GA1062951@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

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
> > 
>