Received-SPF: pass (google.com: domain of linux-kernel+bounces-176722-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sat, 11 May 2024 21:15:54 +0100
From: Simon Horman <horms@kernel.org>
To: Wei Huang <wei.huang2@amd.com>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-doc@vger.kernel.org, netdev@vger.kernel.org,
	bhelgaas@google.com, corbet@lwn.net, davem@davemloft.net,
	edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
	alex.williamson@redhat.com, gospo@broadcom.com,
	michael.chan@broadcom.com, ajit.khaparde@broadcom.com,
	manoj.panicker2@amd.com, Eric.VanTassell@amd.com
Subject: Re: [PATCH V1 5/9] PCI/TPH: Introduce API functions to get/set
 steering tags
Message-ID: <20240511201554.GV2347895@kernel.org>
References: <20240509162741.1937586-1-wei.huang2@amd.com>
 <20240509162741.1937586-6-wei.huang2@amd.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240509162741.1937586-6-wei.huang2@amd.com>

On Thu, May 09, 2024 at 11:27:37AM -0500, Wei Huang wrote:
> This patch introduces two API functions, pcie_tph_get_st() and
> pcie_tph_set_st(), for a driver to 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.
> 
> Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>

Hi Eric and Wei,

I noticed a few minor problems flagged by Sparse
which I'd like to bring to your attention.

> ---
>  drivers/pci/pcie/tph.c  | 383 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci-tph.h |  19 ++
>  2 files changed, 402 insertions(+)
> 
> diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c

..

> +/*
> + * For a given device, return a pointer to the MSI table entry at msi_index.
> + */
> +static void __iomem *tph_msix_table_entry(struct pci_dev *dev,
> +					  __le16 msi_index)
> +{
> +	void *entry;
> +	u16 tbl_sz;
> +	int ret;
> +
> +	ret = tph_get_table_size(dev, &tbl_sz);
> +	if (ret || msi_index > tbl_sz)

While tbl_sz is a host-byte order integer value, msi_index is little endian.
So maths operations involving the latter doesn't seem right.

Flagged by Sparse.

> +		return NULL;
> +
> +	entry = dev->msix_base + msi_index * PCI_MSIX_ENTRY_SIZE;

Likewise, there seem to be endian problems here here.

Also, entry is used on the line above and below in a way
where an __iomem annotation is expected, but entry doesn't have one.

Also flagged by Sparse.

> +
> +	return entry;
> +}

..

> +/* Write the steering tag to MSI-X vector control register */
> +static void tph_write_tag_to_msix(struct pci_dev *dev, int msix_nr, u16 tag)
> +{
> +	u32 val;
> +	void __iomem *vec_ctrl;
> +	struct msi_desc *msi_desc;
> +
> +	msi_desc = tph_msix_index_to_desc(dev, msix_nr);
> +	if (!msi_desc) {
> +		pr_err("MSI-X descriptor for #%d not found\n", msix_nr);
> +		return;
> +	}
> +
> +	vec_ctrl = tph_msix_vector_control(dev, msi_desc->msi_index);

According to Sparse, the type of msi_desc->msi_index is unsigned short.
But tph_msix_vector_control expects it's second argument to be __le16.

> +
> +	val = readl(vec_ctrl);
> +	val &= 0xffff;
> +	val |= (tag << 16);
> +	writel(val, vec_ctrl);
> +
> +	/* read back to flush the update */
> +	val = readl(vec_ctrl);
> +	msi_unlock_descs(&dev->dev);
> +}

..