Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
To:     Lukas Wunner <lukas@wunner.de>
CC:     "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>
Subject: RE: [PATCH v2 04/15] PCI: Add pci_find_vsec_capability() to find a
 specific VSEC
Thread-Topic: [PATCH v2 04/15] PCI: Add pci_find_vsec_capability() to find a
 specific VSEC
Thread-Index: AQHW+WCk8KFoXO5U70eM77z8tv2kyapFKi6AgAB9hHA=
Date:   Wed, 3 Feb 2021 01:54:49 +0000
Message-ID: <DM5PR12MB18351689BA7312BF9DFDD6FEDAB49@DM5PR12MB1835.namprd12.prod.outlook.com>
References: <cover.1612269536.git.gustavo.pimentel@synopsys.com>
 <2ecb33dfee5dc05efc05de0731b0cb77bc246f54.1612269537.git.gustavo.pimentel@synopsys.com>
 <20210202180855.GA3571@wunner.de>
In-Reply-To: <20210202180855.GA3571@wunner.de>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: be918cc4-0f41-44e9-ae27-08d8c7e6b0b3
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Feb 2021 01:54:49.2976
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: c33c9f88-1eb7-4099-9700-16013fd9e8aa
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: m23ntKw5siVxsDRdlK2DI8uzmQ/lIBImMmSHJl0rI7yGNBqZx8tYF1knU8b3eYShVWzNdoxvaBEjcjikFYc09A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4958
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Lukas,

On Tue, Feb 2, 2021 at 18:8:55, Lukas Wunner <lukas@wunner.de> wrote:

> On Tue, Feb 02, 2021 at 01:40:18PM +0100, Gustavo Pimentel wrote:
> >  /**
> > + * pci_find_vsec_capability - Find a vendor-specific extended capabili=
ty
> > + * @dev: PCI device to query
> > + * @cap: vendor-specific capability ID code
> > + *
> > + * Returns the address of the vendor-specific structure that matches t=
he
> > + * requested capability ID code within the device's PCI configuration =
space
> > + * or 0 if it does not find a match.
> > + */
> > +u16 pci_find_vsec_capability(struct pci_dev *dev, int vsec_cap_id)
> > +{
>=20
> As the name implies, the capability is "vendor-specific", so it is perfec=
tly
> possible that two vendors use the same VSEC ID for different things.
>=20
> To make sure you're looking for the right capability, you need to pass
> a u16 vendor into this function and bail out if dev->vendor is different.

This function will be called by the driver that will pass the correct=20
device which will be already pointing to the config space associated with=20
the endpoint for instance. Because the driver is already attached to the=20
endpoint through the vendor ID and device ID specified, there is no need=20
to do that validation, it will be redundant.

>=20
>=20
> > +	u16 vsec;
> > +	u32 header;
> > +
> > +	vsec =3D pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VNDR);
> > +	while (vsec) {
> > +		if (pci_read_config_dword(dev, vsec + PCI_VSEC_HDR,
> > +					  &header) =3D=3D PCIBIOS_SUCCESSFUL &&
> > +		    PCI_VSEC_CAP_ID(header) =3D=3D vsec_cap_id)
> > +			return vsec;
> > +
> > +		vsec =3D pci_find_next_ext_capability(dev, vsec,
> > +						    PCI_EXT_CAP_ID_VNDR);
> > +	}
>=20
> FWIW, a more succinct implementation would be:
>=20
> 	while ((vsec =3D pci_find_next_ext_capability(...))) { ... }
>=20
> See set_pcie_thunderbolt() in drivers/pci/probe.c for an example.
> Please consider refactoring that function to use your new helper.

That looks more clean. I will do it. Thanks!

>=20
> Thanks,
>=20
> Lukas