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]
Date:   Wed, 3 Feb 2021 01:54:49 +0000
From:   Gustavo Pimentel <Gustavo.Pimentel@...opsys.com>
To:     Lukas Wunner <lukas@...ner.de>
CC:     "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>
Subject: RE: [PATCH v2 04/15] PCI: Add pci_find_vsec_capability() to find a
 specific VSEC

Hi Lukas,

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

> On Tue, Feb 02, 2021 at 01:40:18PM +0100, Gustavo Pimentel wrote:
> >  /**
> > + * pci_find_vsec_capability - Find a vendor-specific extended capability
> > + * @dev: PCI device to query
> > + * @cap: vendor-specific capability ID code
> > + *
> > + * Returns the address of the vendor-specific structure that matches the
> > + * 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)
> > +{
> 
> As the name implies, the capability is "vendor-specific", so it is perfectly
> possible that two vendors use the same VSEC ID for different things.
> 
> 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 
device which will be already pointing to the config space associated with 
the endpoint for instance. Because the driver is already attached to the 
endpoint through the vendor ID and device ID specified, there is no need 
to do that validation, it will be redundant.

> 
> 
> > +	u16 vsec;
> > +	u32 header;
> > +
> > +	vsec = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VNDR);
> > +	while (vsec) {
> > +		if (pci_read_config_dword(dev, vsec + PCI_VSEC_HDR,
> > +					  &header) == PCIBIOS_SUCCESSFUL &&
> > +		    PCI_VSEC_CAP_ID(header) == vsec_cap_id)
> > +			return vsec;
> > +
> > +		vsec = pci_find_next_ext_capability(dev, vsec,
> > +						    PCI_EXT_CAP_ID_VNDR);
> > +	}
> 
> FWIW, a more succinct implementation would be:
> 
> 	while ((vsec = pci_find_next_ext_capability(...))) { ... }
> 
> 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!

> 
> Thanks,
> 
> Lukas


Powered by blists - more mailing lists