Received-SPF: pass (google.com: domain of linux-kernel+bounces-123781-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <a144b96b-6ec4-4a62-b2ab-1ed631d81544@linux.intel.com> <20240328161831.GA1568357@bhelgaas>
In-Reply-To: <20240328161831.GA1568357@bhelgaas>
Reply-To: sathyanarayanan.kuppuswamy@linux.intel.com
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Date: Thu, 28 Mar 2024 15:36:07 -0700
Message-ID: <CAC41dw-wZ7eM_y5AkwW3F4dJiHYuR3YSWwap5M7FVB_fZAkg=w@mail.gmail.com>
Subject: Re: [PATCH] PCI: Update pci_find_capability() stub return values
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Bjorn Helgaas <bhelgaas@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Thu, Mar 28, 2024 at 9:18=E2=80=AFAM Bjorn Helgaas <helgaas@kernel.org> =
wrote:
>
> On Wed, Mar 27, 2024 at 02:49:34PM -0700, Kuppuswamy Sathyanarayanan wrot=
e:
> > On 3/27/24 11:02 AM, Bjorn Helgaas wrote:
> > > From: Bjorn Helgaas <bhelgaas@google.com>
> > >
> > > f646c2a0a668 ("PCI: Return u8 from pci_find_capability() and similar"=
) and
> > > ee8b1c478a9f ("PCI: Return u16 from pci_find_ext_capability() and sim=
ilar")
> > > 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@google.com>
> > > ---
> >
> > This change looks fine to me.
> >
> > Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@lin=
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:
>

Agree. Thanks for clarifying it.

>   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.o=
rg/
> [2] https://lore.kernel.org/all/20240325224931.1462051-1-helgaas@kernel.o=
rg/
>
> Bjorn