Message-ID: <40e675a1-6931-8daa-d98d-7b9c64785012@huawei.com>
Date:   Tue, 2 Nov 2021 21:03:30 +0800
From:   Dongdong Liu <liudongdong3@...wei.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     <hch@...radead.org>, <kw@...ux.com>, <logang@...tatee.com>,
        <leon@...nel.org>, <linux-pci@...r.kernel.org>,
        <rajur@...lsio.com>, <hverkuil-cisco@...all.nl>,
        <linux-media@...r.kernel.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH V11 4/8] PCI/sysfs: Add a tags sysfs file for PCIe
 Endpoint devices

Hi Bjorn

Many thanks for you review.
On 2021/11/2 4:54, Bjorn Helgaas wrote:
> On Sat, Oct 30, 2021 at 09:53:44PM +0800, Dongdong Liu wrote:
>> PCIe spec 5.0 r1.0 section 2.2.6.2 says:
>>
>>   If an Endpoint supports sending Requests to other Endpoints (as
>>   opposed to host memory), the Endpoint must not send 10-Bit Tag
>>   Requests to another given Endpoint unless an implementation-specific
>>   mechanism determines that the Endpoint supports 10-Bit Tag Completer
>>   capability.
>>
>> Add a tags sysfs file, write 0 to disable 10-Bit Tag Requester
>> when the driver does not bind the device. The typical use case is for
>> p2pdma when the peer device does not support 10-Bit Tag Completer.
>> Write 10 to enable 10-Bit Tag Requester when RC supports 10-Bit Tag
>> Completer capability. The typical use case is for host memory targeted
>> by DMA Requests. The tags file content indicate current status of Tags
>> Enable.
>>
>> PCIe r5.0, sec 2.2.6.2 says:
>>
>>   Receivers/Completers must handle 8-bit Tag values correctly regardless
>>   of the setting of their Extended Tag Field Enable bit (see Section
>>   7.5.3.4).
>>
>> Add this comment in pci_configure_extended_tags(). As all PCIe completers
>> are required to support 8-bit tags, so we do not use tags sysfs file
>> to manage 8-bit tags.
>
>> +What:		/sys/bus/pci/devices/.../tags
>> +Date:		September 2021
>> +Contact:	Dongdong Liu <liudongdong3@...wei.com>
>> +Description:
>> +		The file will be visible when the device supports 10-Bit Tag
>> +		Requester. The file is readable, the value indicate current
>> +		status of Tags Enable(5-Bit, 8-Bit, 10-Bit).
>> +
>> +		The file is also writable, The values accepted are:
>> +		* > 0 - this number will be reported as tags bit to be
>> +			enabled. current only 10 is accepted
>> +		* < 0 - not valid
>> +		* = 0 - disable 10-Bit Tag, use Extended Tags(8-Bit or 5-Bit)
>> +
>> +		write 0 to disable 10-Bit Tag Requester when the driver does
>> +		not bind the device. The typical use case is for p2pdma when
>> +		the peer device does not support 10-Bit Tag Completer.
>> +
>> +		Write 10 to enable 10-Bit Tag Requester when RC supports 10-Bit
>> +		Tag Completer capability. The typical use case is for host
>> +		memory targeted by DMA Requests.
>
> 1) I think I would rename this from "tags" to "tag_bits".  A file
>    named "tags" that contains 8 suggests that we can use 8 tags, but
>    in fact, we can use 256 tags.
Looks good, Will do.
>
> 2) This controls tag size the requester will use.  The current knobs
>    in the hardware allow 5, 8, or 10 bits.
>
>    "0" to disable 10-bit tags without specifying whether we should use
>    5- or 8-bit tags doesn't seem right.  All completers are *supposed*
>    to support 8-bit, but we've tripped over a few that don't.
>
>    I don't think we currently have a run-time (or even a boot-time)
>    way to disable 8-bit tags; all we have is the quirk_no_ext_tags()
>    quirk.  But if we ever wanted to *add* that, maybe we would want:
>
>       5 - use 5-bit tags
>       8 - use 8-bit tags
>      10 - use 10-bit tags
will do.
>    Maybe we just say "0" is invalid, since there's no obvious way to
>    map this?
OK, will do.
>
>> +static ssize_t tags_show(struct device *dev,
>> +			 struct device_attribute *attr,
>> +			 char *buf)
>> +{
>> + ...
>
>> +	if (ctl & PCI_EXP_DEVCTL2_10BIT_TAG_REQ_EN)
>> +		return sysfs_emit(buf, "%s\n", "10-Bit");
>> +
>> +	ret = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &ctl);
>> +	if (ret)
>> +		return -EINVAL;
>> +
>> +	if (ctl & PCI_EXP_DEVCTL_EXT_TAG)
>> +		return sysfs_emit(buf, "%s\n", "8-Bit");
>> +
>> +	return sysfs_emit(buf, "%s\n", "5-Bit");
>
> Since I prefer the "tag_bits" name, my preference would be bare
> numbers here: "10", "8", "5".
Will do.
>
> Both comments apply to the sriov files, too.
Will do.
>
>> +static umode_t pcie_dev_tags_attrs_is_visible(struct kobject *kobj,
>> +					      struct attribute *a, int n)
>> +{
>> +	struct device *dev = kobj_to_dev(kobj);
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	if (pdev->is_virtfn)
>> +		return 0;
>> +
>> +	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_ENDPOINT)
>> +		return 0;
>> +
>> +	if (!(pdev->devcap2 & PCI_EXP_DEVCAP2_10BIT_TAG_REQ))
>> +		return 0;
>
> Makes sense for now that the file is only visible if a requester
> supports 10-bit tags.  If we ever wanted to extend this to control 5-
> vs 8-bit tags, we could make it visible in more cases then.
Will do.
>
>> +
>> +	return a->mode;
>> +}
>
>> @@ -2075,6 +2089,12 @@ int pci_configure_extended_tags(struct pci_dev *dev, void *ign)
>>  		return 0;
>>  	}
>>
>> +	/*
>> +	 * PCIe r5.0, sec 2.2.6.2 says "Receivers/Completers must handle 8-bit
>> +	 * Tag values correctly regardless of the setting of their Extended Tag
>> +	 * Field Enable bit (see Section 7.5.3.4)", so it is safe to enable
>> +	 * Extented Tags.
>
> s/Extented/Extended/
Will fix.

Thanks,
Dongdong
>
>> +	 */
>>  	if (!(ctl & PCI_EXP_DEVCTL_EXT_TAG)) {
>>  		pci_info(dev, "enabling Extended Tags\n");
>>  		pcie_capability_set_word(dev, PCI_EXP_DEVCTL,
>> --
>> 2.22.0
>>
> .
>

Powered by blists - more mailing lists