linux-kernel - Re: [PATCH v2 1/8] x86/PCI: Move some logic to new function

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: <757f5fcdc14c8f22e52a34974f2e48fe2dcea4d5.camel@redhat.com>
Date: Wed, 17 Jul 2024 21:28:31 +0200
From: Philipp Stanner <pstanner@...hat.com>
To: Stewart Hildebrand <stewart.hildebrand@....com>, Bjorn Helgaas
	 <bhelgaas@...gle.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar
	 <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
	 <dave.hansen@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>, Ilpo
	Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: x86@...nel.org, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/8] x86/PCI: Move some logic to new function

On Tue, 2024-07-16 at 15:32 -0400, Stewart Hildebrand wrote:
> ... to reduce indentation level. Take the opportunity to remove
> redundant info from debug print string.

Is that intended to be the final commit message or is it still a draft?

I'd call the commit "x86/PCI: Improve code readability" (or sth like
that) since that is what the commit is about. It's not so much about
the code move per se.

and have a short message such as:

"
The indentation in pcibios_allocate_dev_resources() is unusally deep.

Improve that by moving some of its code to the new function
alloc_resource().

As we're at it, remove redundant information from dev_dbg().
"


Regards,
P.

> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@....com>
> ---
> v1->v2:
> * new patch
> ---
>  arch/x86/pci/i386.c | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/pci/i386.c b/arch/x86/pci/i386.c
> index f2f4a5d50b27..3abd55902dbc 100644
> --- a/arch/x86/pci/i386.c
> +++ b/arch/x86/pci/i386.c
> @@ -246,6 +246,25 @@ struct pci_check_idx_range {
>         int end;
>  };
>  
> +static void alloc_resource(struct pci_dev *dev, int idx, int pass)
> +{
> +       struct resource *r = &dev->resource[idx];
> +
> +       dev_dbg(&dev->dev, "BAR %d: reserving %pr (p=%d)\n", idx, r,
> pass);
> +
> +       if (pci_claim_resource(dev, idx) < 0) {
> +               if (r->flags & IORESOURCE_PCI_FIXED) {
> +                       dev_info(&dev->dev, "BAR %d %pR is
> immovable\n",
> +                                idx, r);
> +               } else {
> +                       /* We'll assign a new address later */
> +                       pcibios_save_fw_addr(dev, idx, r->start);
> +                       r->end -= r->start;
> +                       r->start = 0;
> +               }
> +       }
> +}
> +
>  static void pcibios_allocate_dev_resources(struct pci_dev *dev, int
> pass)
>  {
>         int idx, disabled, i;
> @@ -271,23 +290,8 @@ static void
> pcibios_allocate_dev_resources(struct pci_dev *dev, int pass)
>                                 disabled = !(command &
> PCI_COMMAND_IO);
>                         else
>                                 disabled = !(command &
> PCI_COMMAND_MEMORY);
> -                       if (pass == disabled) {
> -                               dev_dbg(&dev->dev,
> -                                       "BAR %d: reserving %pr (d=%d,
> p=%d)\n",
> -                                       idx, r, disabled, pass);
> -                               if (pci_claim_resource(dev, idx) < 0)
> {
> -                                       if (r->flags &
> IORESOURCE_PCI_FIXED) {
> -                                               dev_info(&dev->dev,
> "BAR %d %pR is immovable\n",
> -                                                        idx, r);
> -                                       } else {
> -                                               /* We'll assign a new
> address later */
> -
>                                                pcibios_save_fw_addr(de
> v,
> -                                                               idx,
> r->start);
> -                                               r->end -= r->start;
> -                                               r->start = 0;
> -                                       }
> -                               }
> -                       }
> +                       if (pass == disabled)
> +                               alloc_resource(dev, idx, pass);
>                 }
>         if (!pass) {
>                 r = &dev->resource[PCI_ROM_RESOURCE];