linux-kernel - Re: [PATCH v3 17/17] PCI: dwc: Add Baikal-T1 PCIe controller support

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20220628151707.GA1838891@bhelgaas>
Date:   Tue, 28 Jun 2022 10:17:07 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Serge Semin <fancer.lancer@...il.com>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Rob Herring <robh@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Frank Li <Frank.Li@....com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Rob Herring <robh+dt@...nel.org>, linux-pci@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 17/17] PCI: dwc: Add Baikal-T1 PCIe controller support

On Tue, Jun 28, 2022 at 03:23:56PM +0300, Serge Semin wrote:
> Bjorn,
> Do you have anything to say based on the notes below?

I'm focused on the first series for now.

> On Wed, Jun 22, 2022 at 08:04:37PM +0300, Serge Semin wrote:
> > On Tue, Jun 21, 2022 at 01:29:41PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Jun 20, 2022 at 08:13:47PM +0300, Serge Semin wrote:
> > > > On Wed, Jun 15, 2022 at 11:48:48AM -0500, Bjorn Helgaas wrote:
> > > > > On Fri, Jun 10, 2022 at 11:57:05AM +0300, Serge Semin wrote:

> > > > > > +struct dw_pcie_ops bt1_pcie_dw_ops = {
> > > > > > +	.read_dbi = bt1_pcie_read_dbi,
> > > > > > +	.write_dbi = bt1_pcie_write_dbi,
> > > > > > +	.write_dbi2 = bt1_pcie_write_dbi2,
> > > > > > +	.start_link = bt1_pcie_start_ltssm,
> > > > > > +	.stop_link = bt1_pcie_stop_ltssm,
> > > > > > +};
> > > 
> > > > > Please rename to "dw_pcie_ops" as most drivers use. 
> > > > 
> > > > IMO matching the structure and its instance names is not a good idea.
> > > > Other than confusing objects nature, at the very least it forces you to
> > > > violate the local namespace convention. Thus in the line of the
> > > > dw_pcie->ops initialization it looks like you use some generic
> > > > operations while in fact you just refer to the locally defined
> > > > DW PCIe ops instance with the generic variable name. Moreover AFAICS
> > > > the latest platform drivers mainly use the vendor-specific prefix in
> > > > the dw_pcie_ops structure instance including the ones acked by you,
> > > > Lorenzo and Gustavo. What makes my code any different from them?
> > 
> > > That's fair.  I suggest "bt1_pcie_ops" or "bt1_dw_pcie_ops" to match
> > > the other drivers that include the driver name:
> > > 
> > >   intel_pcie_ops
> > >   keembay_pcie_ops
> > >   kirin_dw_pcie_ops
> > >   tegra_dw_pcie_ops
> > 
> > +   ks_pcie_dw_pcie_ops
> > 
> > which is even further from the suggested names.)

As I said, they're not 100% consistent, but they almost all end in
"pcie_ops".  Yours ends in "pcie_dw_ops", which is why I suggested
renaming to be similar to the others.

I don't want to make a huge deal about this, but I do want as much
similarity across drivers as possible.  I get that it doesn't benefit
*you*, but it's a huge benefit to everybody else.

> > > They're not 100% consistent, but hopefully we can at least not make
> > > things *less* consistent.
> > 
> > I don't think we can make something less consistent if there is no real
> > consistency.) There are at most five ops descriptors can be defined in
> > the DW PCIe platform drivers: