linux-kernel - Re: [PATCH] PCI: hotplug: Use atomic_{fetch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Thu, 13 Jun 2024 18:44:36 +0200
From: Uros Bizjak <ubizjak@...il.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Lukas Wunner <lukas@...ner.de>, 
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH] PCI: hotplug: Use atomic_{fetch_}andnot() where appropriate

On Thu, Jun 13, 2024 at 5:51 PM Bjorn Helgaas <helgaas@...nel.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.