linux-kernel - Re: [PATCH 17/21] ata: ahci: Add DWC AHCI SATA 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20220411204118.tfagojpcugz4gksj@mobilestation>
Date:   Mon, 11 Apr 2022 23:41:18 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Damien Le Moal <damien.lemoal@...nsource.wdc.com>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Hans de Goede <hdegoede@...hat.com>,
        Jens Axboe <axboe@...nel.dk>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Rob Herring <robh+dt@...nel.org>, linux-ide@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 17/21] ata: ahci: Add DWC AHCI SATA controller support

On Mon, Apr 11, 2022 at 10:03:27PM +0900, Damien Le Moal wrote:
> On 4/11/22 21:41, Serge Semin wrote:
> > I beg your pardon what convention? Is it defined in someplace of the
> > subsystem docs? If it's not then how should I know about that? These
> > are the device-specific macro. The static methods below are also
> > platform-specific and the standard kernel coding style doesn't specify
> > any rule about that. Moreover the most of the AHCI glue drivers (LLDD
> > like ahci_mtk.c, ahci_ceva.c, ahci_brcm.c, ahci_st.c, ahci_tegra.c,
> > ahci_xgene.c, etc) use the same prefixing style as I do here. Finally
> > the prefix reflects the actual device name "DWC AHCI". So if there is
> > no subsystem-specific restrictions I normally define the prefix in
> > that order for the sake of the clarity.
> 

> Look at how other ahci drivers have named things. That's the "convention"
> I am talking about. Most of them name things ahci_xxx_... Same for macros.

Looked. Way not the most of them. Here are the other drivers with the
naming like I used in this patch:
ahci_mtk.c, ahci_ceva.c, ahci_brcm.c, ahci_st.c, ahci_tegra.c,
ahci_xgene.c, ahci_imx.c, etc.
That's almost half of all the AHCI drivers. Really how could I have
possibly figure out that there is a convention if so many drivers
don't have it followed?..

> 
> > 
> > Note I don't mind to convert the code to being the way you ask, but if
> > it's really the AHCI-specific codying style convention then it would be
> > very useful to have it described/advertised in some place in the
> > kernel so to know about that beforehead for the developers reference.
> > So do you insist on switching the words order in the names prefix here?
> 

> It is nice to have code consistency in the naming. That always facilitate
> grepping the code.

I am with both my hands for it, but that naming consistency must be
thoroughly documented in some place of the kernel or at least the
"convention" must be logically derivable from the existing code. It
would spare my, the other developers and actually the maintainers
time. Do you suggest to thoroughly scan each AHCI driver in the
subsystem to figure it out? Even if I did that the only way to come up
with an idea that there is a naming convention would be only if I
expected that there is one, but seeing it's a first time I've met such
LLDD requirement (really, no kernel subsystem has such naming
convention applied for the glue-drivers) and almost half of the AHCI
drivers not following the convention while the other half following it
just partly. Judging by that, neither me nor many other developer had
a chance to create an acceptable code from scratch.

-Sergey

> 
> 
> -- 
> Damien Le Moal
> Western Digital Research