linux-kernel - Re: [PATCH] PCI: Update pci_find_capability() stub return values

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: <20240328161831.GA1568357@bhelgaas>
Date: Thu, 28 Mar 2024 11:18:31 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH] PCI: Update pci_find_capability() stub return values

On Wed, Mar 27, 2024 at 02:49:34PM -0700, Kuppuswamy Sathyanarayanan wrote:
> On 3/27/24 11:02 AM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@...gle.com>
> >
> > f646c2a0a668 ("PCI: Return u8 from pci_find_capability() and similar") and
> > ee8b1c478a9f ("PCI: Return u16 from pci_find_ext_capability() and similar")
> > updated the return type of the extern declarations, but neglected to update
> > the type of the stubs used CONFIG_PCI is not enabled.
> >
> > Update them to match the extern declarations.
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@...gle.com>
> > ---
> 
> This change looks fine to me.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>

Thanks for reviewing it!

> But the callers of these functions still seems to use int
> declaration to store the output. Any reason for not changing them?
> Like the usages in drivers/pci/pci.c?

This patch is just to make the extern declarations match the stubs.

No particular reason not to change users other than I didn't want to
change the users before the declarations (to avoid warnings about
assigning an int to a u8 or u16), and there's not a lot of value in
changing local variables, where an int is just on the stack and works
fine.

Changing stored values in a struct would have more benefit.  I took a
quick look and found these possibilities:

  struct controller.cap_offset (drivers/pci/hotplug/shpchp.h SHPC cap)
  struct pci_sriov.pos (drivers/pci/pci.h SR-IOV ext cap)
  struct altera_pcie_data.cap_offset (drivers/pci/controller/pcie-altera.c PCIe cap)

  struct tg3.msi_cap (broadcom/tg3.h MSI cap)
  struct tg3.pcix_cap (broadcom/tg3.h PCI-X cap)
  struct bnx2.pm_cap (broadcom/bnx2.h PM cap)
  struct bnx2.pcix_cap (broadcom/bnx2.h PCI-X cap)
  struct bnx2x_sriov.cap (broadcom/bnx2x/bnx2x_sriov.h SR-IOV ext cap)
  struct amd8111e_priv.pm_cap (amd/amd8111e.c, removed [1])
  struct pci_params.pm_cap (qlogic/qed/qed.h, removed [2])
  struct qed_hw_sriov_info.cap (qlogic/qed/qed_sriov.h SR-IOV ext cap)
  struct eeh_dev.pcix_cap (powerpc/include/asm/eeh.h PCI-X cap)
  struct eeh_dev.pcie_cap (powerpc/include/asm/eeh.h PCIe cap)
  struct eeh_dev.aer_cap (powerpc/include/asm/eeh.h AER ext cap)
  struct eeh_dev.af_cap (powerpc/include/asm/eeh.h AF cap)
  struct icm.vnd_cap (drivers/thunderbolt/icm.c VNDR ext cap)

[1] https://lore.kernel.org/all/20240325220633.1453180-1-helgaas@kernel.org/
[2] https://lore.kernel.org/all/20240325224931.1462051-1-helgaas@kernel.org/

Bjorn