linux-kernel - Re: [PATCH v11 3/5] PCI: Move pci_clear_and_set

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]

Message-ID: <20231130183755.GA487634@bhelgaas>
Date:   Thu, 30 Nov 2023 12:37:55 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     Shuai Xue <xueshuai@...ux.alibaba.com>,
        ilkka@...amperecomputing.com, kaishen@...ux.alibaba.com,
        yangyicong@...wei.com, will@...nel.org,
        Jonathan.Cameron@...wei.com, baolin.wang@...ux.alibaba.com,
        robin.murphy@....com, chengyou@...ux.alibaba.com,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-pci@...r.kernel.org,
        rdunlap@...radead.org, mark.rutland@....com,
        zhuo.song@...ux.alibaba.com, renyu.zj@...ux.alibaba.com
Subject: Re: [PATCH v11 3/5] PCI: Move pci_clear_and_set_dword() helper to
 PCI header

On Thu, Nov 30, 2023 at 12:52:20PM +0200, Ilpo Järvinen wrote:
> On Wed, 29 Nov 2023, Bjorn Helgaas wrote:
> > On Mon, Nov 27, 2023 at 09:34:05AM +0800, Shuai Xue wrote:
> > > On 2023/11/22 21:14, Ilpo Järvinen wrote:
> > > > On Tue, 21 Nov 2023, Shuai Xue wrote:
> > > > 
> > > >> The clear and set pattern is commonly used for accessing PCI config,
> > > >> move the helper pci_clear_and_set_dword() from aspm.c into PCI header.
> > > >> In addition, rename to pci_clear_and_set_config_dword() to retain the
> > > >> "config" information and match the other accessors.
> > > >>
> > > >> No functional change intended.
> 
> > > >> +
> > > >> +void pci_clear_and_set_config_dword(const struct pci_dev *dev, int pos,
> > > >> +				    u32 clear, u32 set)
> > > > 
> > > > Just noting that annoyingly the ordering within the name is inconsistent 
> > > > between:
> > > >   pci_clear_and_set_config_dword()
> > > > and
> > > >   pcie_capability_clear_and_set_dword()
> > > > 
> > > > And if changed, it would be again annoyingly inconsistent with 
> > > > pci_read/write_config_*(), oh well... And renaming pci_read/write_config_* 
> > > > into the hierarchical pci_config_read/write_*() form for would touch only 
> > > > ~6k lines... ;-D
> > > 
> > > I think it is a good question, but I don't have a clear answer. I don't
> > > know much about the name history.  As you mentioned, the above two
> > > accessors are the foundation operation, may it comes to @Bjorn decision.
> > > 
> > > The pci_clear_and_set_config_dword() is a variant of below pci accessors:
> > > 
> > >     pci_read_config_dword()
> > >     pci_write_config_dword()
> > > 
> > > At last, they are consistent :)
> > 
> > "pcie_capability_clear_and_set_dword" is specific to the PCIe
> > Capability, doesn't work for arbitrary config space, and doesn't
> > include the word "config".
> > 
> > "pci_clear_and_set_config_dword" seems consistent with the arbitrary
> > config space accessor pattern.
> > 
> > At least "clear_and_set" is consistent across both.
> > 
> > I'm not too bothered by the difference between "clear_and_set_dword"
> > (for the PCIe capability) and "clear_and_set_config_dword" (for
> > arbitrary things).
> > 
> > Yes, "pcie_capability_clear_and_set_config_dword" would be a little
> > more consistent, but seems excessively wordy (no pun intended).
> > 
> > But maybe I'm missing your point, Ilpo.  If so, what would you
> > propose?
> 
> What I was hoping for a way to (eventually) have consistency in naming 
> like this (that is, the place where "config" or "capabilitity" appears 
> in the name):
> 
> pci_config_read_dword()
> pci_config_clear_and_set_dword()
> pcie_capability_read_dword()
> pcie_capability_clear_and_set_dword()

Ah, I see, thanks.

> But thanks to pci_read_config_dword() & friends being there since dawn of 
> time and with 6k+ instances, I guess I'm just dreaming of impossible 
> things.

Yeah, I think so.

Bjorn