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

From: Bjorn Helgaas
Date: Fri Jun 07 2024 - 13:45:56 EST


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.