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