linux-kernel - Re: [PATCH RESEND v2 3/7] PCI: Add new array for keeping track of ordering of reset methods

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: <20210520152632.GC641812@rocinante.localdomain>
Date:   Thu, 20 May 2021 17:26:32 +0200
From:   Krzysztof Wilczyński <kw@...ux.com>
To:     Amey Narkhede <ameynarkhede03@...il.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>, alex.williamson@...hat.com,
        raphael.norwitz@...anix.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND v2 3/7] PCI: Add new array for keeping track of
 ordering of reset methods

Hi Amey,

[...]
> +/*
> + * The ordering for functions in pci_reset_fn_methods
> + * is required for reset_methods byte array defined
> + * in struct pci_dev
> + */

A small nitpick: missing period at the end of the sentence in the
comment above, and in other comments too.  Might add for completeness
and consistency.

[...]
> +typedef int (*pci_reset_fn_t)(struct pci_dev *, int);
> +
> +struct pci_reset_fn_method {
> +	pci_reset_fn_t reset_fn;
> +	char *name;
> +};

Question about the custom type definition above: would it be really
needed?  It there is only potentially a limited use for it, then perhaps
it would not be useful to have one?

Linus also has some preference on usage of custom types, as per:

  https://yarchive.net/comp/linux/typedefs.html

But, in the end, this really boils down to a matter of style and/or
preference.

[...]
> +#define PCI_RESET_FN_METHODS 5

Not sure if worth changing name of this constant, but what about the
following:

  #define PCI_RESET_FN_METHODS_NUM 5

Or even perhaps:

  #define PCI_RESET_METHODS_NUM 5

So it's a little bit more self-explanatory.  This would be in the
similar notion, as per:

  https://elixir.bootlin.com/linux/v5.13-rc2/source/include/linux/pci.h#L115

[...]
> +	u8 reset_methods[PCI_RESET_FN_METHODS];	/* Array for storing ordering of reset methods */

This comment reads somewhat awkward - we know that an array would be
used, most likely, for storing things, thus what about the following:

  /* Reset methods ordered by priority */

Just a suggestion, though.

Krzysztof