linux-kernel - Re: [PATCH V1 5/9] PCI/TPH: Introduce API functions to get/set 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]

Date: Mon, 13 May 2024 08:29:45 -0500
From: Wei Huang <wei.huang2@....com>
To: Simon Horman <horms@...nel.org>
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,
 manoj.panicker2@....com, Eric.VanTassell@....com
Subject: Re: [PATCH V1 5/9] PCI/TPH: Introduce API functions to get/set
 steering tags



On 5/11/24 15:15, Simon Horman wrote:
> 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@....com>
>> Signed-off-by: Eric Van Tassell <Eric.VanTassell@....com>
>> Signed-off-by: Wei Huang <wei.huang2@....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.

Thanks, will take care of it in the next revision.

> 
> 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.

Will fix

> 
>> +
>> +	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.

Will fix

> 
>> +
>> +	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);
>> +}
> 
> ...