linux-kernel - Re: [PATCH] PCI: Fix general code style

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]

Date:   Wed, 25 Aug 2021 15:43:56 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Sergio Miguéns Iglesias <lonyelon@...il.com>
Cc:     bhelgaas@...gle.com, rjw@...ysocki.net, lenb@...nel.org,
        boris.ostrovsky@...cle.com, jgross@...e.com,
        sstabellini@...nel.org, konrad.wilk@...cle.com,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-acpi@...r.kernel.org, xen-devel@...ts.xenproject.org,
        Sergio Miguéns Iglesias <sergio@...y.xyz>
Subject: Re: [PATCH] PCI: Fix general code style

On Thu, Aug 05, 2021 at 12:28:32AM +0200, Sergio Miguéns Iglesias wrote:
> The code style for most files was fixed. This means that blank lines
> were added when needed (normally after variable declarations), spaces
> before tabs were removed, some code alignment issues were solved, block
> comment style was fixed, every instance of "unsigned var" was replaced
> with "unsigned int var"... Etc.
> 
> This commit does not change the logic of the code, it just fixes
> aesthetic problems.

I generally *like* this, and it does fix some annoying things, but I
think it's a little too much all at once.  If we're working in a file
and doing actual bug fixes or new functionality, and we want to fix
some typos or something at the end, that might be OK, but I think the
churn in the git history outweighs the benefit of this huge patch.

So I would encourage you to use some of the PCI expertise you've
gained by looking at all this code to work on something with a little
more impact.  Here are a couple ideas:

  - There are only two uses of __ref and __refdata in drivers/pci/.
    The fact that they're so rare makes me suspect that we don't need
    them.  But I haven't investigated these to see.  Somebody could
    check that out and remove them if we don't need them.  Be aware
    that I will want a clear argument for why they're not needed :)

  - Coverity complains about several issues in drivers/pci/ [1].  Most
    of the time these are false positives, but not always.  Sometimes
    there's an actual bug, and sometimes there's a way to restructure
    the code to avoid the warning (which usually means doing things
    the same way they are done elsewhere).

  - "make C=2 drivers/pci/" (sparse checker, [2]) complains about a
    few things.  Leave the pci_power_t ones alone for now, but there
    are a couple other type issues that could be cleaned up.

[1] https://docs.google.com/spreadsheets/d/19eyNDou83JACzf44j0NRzEWysva6g44G2_Z9IEXGVNk/edit?usp=sharing
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/dev-tools/sparse.rst?id=v5.13

> Signed-off-by: Sergio Miguéns Iglesias <sergio@...y.xyz>
> ---
>  drivers/pci/access.c       | 22 +++++++++++++---------
>  drivers/pci/bus.c          |  3 ++-
>  drivers/pci/msi.c          | 12 +++++++-----
>  drivers/pci/pci-acpi.c     |  3 ++-
>  drivers/pci/pci-driver.c   | 19 +++++++++++++------
>  drivers/pci/pci-sysfs.c    | 14 ++++++++++++--
>  drivers/pci/pci.c          | 16 ++++++++++++----
>  drivers/pci/proc.c         | 15 +++++++++++++++
>  drivers/pci/quirks.c       | 35 ++++++++++++++++++++++++-----------
>  drivers/pci/remove.c       |  1 +
>  drivers/pci/rom.c          |  2 +-
>  drivers/pci/setup-bus.c    |  5 ++++-
>  drivers/pci/setup-irq.c    | 12 +++++++-----
>  drivers/pci/setup-res.c    |  2 +-
>  drivers/pci/slot.c         |  5 ++++-
>  drivers/pci/syscall.c      |  5 +++--
>  drivers/pci/xen-pcifront.c | 20 ++++++++++++--------
>  17 files changed, 133 insertions(+), 58 deletions(-)