netdev - Re: [PATCH V2 5/9] PCI/TPH: Introduce API functions to manage steering tags

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: <20240607174542.GA853103@bhelgaas>
Date: Fri, 7 Jun 2024 12:45:42 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Wei Huang <wei.huang2@....com>
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org, netdev@...r.kernel.org,
	bhelgaas@...gle.com, corbet@....net, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	alex.williamson@...hat.com, gospo@...adcom.com,
	michael.chan@...adcom.com, ajit.khaparde@...adcom.com,
	somnath.kotur@...adcom.com, andrew.gospodarek@...adcom.com,
	manoj.panicker2@....com, Eric.VanTassell@....com,
	vadim.fedorenko@...ux.dev, horms@...nel.org, bagasdotme@...il.com
Subject: Re: [PATCH V2 5/9] PCI/TPH: Introduce API functions to manage
 steering tags

On Fri, May 31, 2024 at 04:38:37PM -0500, Wei Huang wrote:
> This patch introduces three API functions, pcie_tph_intr_vec_supported(),
> pcie_tph_get_st() and pcie_tph_set_st(), for a driver to query, retrieve
> or configure device's steering tags. There are two possible locations for
> steering tag table and the code automatically figure out the right
> location to set the tags if pcie_tph_set_st() is called. Note the tag
> value is always zero currently and will be extended in the follow-up
> patches.

> +static int tph_get_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
> +				 u8 shift, u32 *field)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	if (!dev->tph_cap)
> +		return -EINVAL;
> +
> +	ret = pci_read_config_dword(dev, dev->tph_cap + offset, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	*field = (reg_val & mask) >> shift;
> +
> +	return 0;
> +}
> +
> +static int tph_get_table_size(struct pci_dev *dev, u16 *size_out)
> +{
> +	int ret;
> +	u32 tmp;
> +
> +	ret = tph_get_reg_field_u32(dev, PCI_TPH_CAP,
> +				    PCI_TPH_CAP_ST_MASK,
> +				    PCI_TPH_CAP_ST_SHIFT, &tmp);

Just use FIELD_GET() instead.

> +	if (ret)
> +		return ret;
> +
> +	*size_out = (u16)tmp;
> +
> +	return 0;
> +}
> +
> +/*
> + * For a given device, return a pointer to the MSI table entry at msi_index.

s/MSI/MSI-X/ to avoid any possible confusion.

> +static void __iomem *tph_msix_table_entry(struct pci_dev *dev,
> +					  u16 msi_index)

> +	ret = pcie_capability_read_dword(rp, PCI_EXP_DEVCAP2, &val);
> +	if (ret) {
> +		pr_err("cannot read device capabilities 2 of %s\n",
> +		       dev_name(&dev->dev));

Never use pr_err() when you can use pci_err() instead.  Obviously no
dev_name() needed with pci_err().  Other instances below.

> +	val &= PCI_EXP_DEVCAP2_TPH_COMP;
> +
> +	return val >> PCI_EXP_DEVCAP2_TPH_COMP_SHIFT;

FIELD_GET()

> + * The PCI Specification version 5.0 requires the "No ST Mode" mode
> + * be supported by any compatible device.

Cite r6.0 or newer and include section number.

> +	/* clear the mode select and enable fields and set new values*/

Space before closing */

> +	ctrl_reg &= ~(PCI_TPH_CTRL_REQ_EN_MASK);
> +	ctrl_reg |= (((u32)req_type << PCI_TPH_CTRL_REQ_EN_SHIFT) &
> +			PCI_TPH_CTRL_REQ_EN_MASK);

FIELD_GET()/FIELD_PREP()

> +static bool pcie_tph_write_st(struct pci_dev *dev, unsigned int msix_nr,
> +			      u8 req_type, u16 tag)

This function is not a predicate and testing for true/false gives no
indication of the sense.

For typical functions that do read/write/etc, returning 0 means
success and -errno means failure.  This is the opposite.

> +	/*
> +	 * disable TPH before updating the tag to avoid potential instability
> +	 * as cautioned about in the "ST Table Programming" of PCI-E spec

s/disable/Disable/

"PCIe r6.0, sec ..."

> +bool pcie_tph_set_st(struct pci_dev *dev, unsigned int msix_nr,
> +		     unsigned int cpu, enum tph_mem_type mem_type,
> +		     u8 req_type)

Should return 0 or -errno.