Re: [PATCH V11 4/8] PCI/sysfs: Add a tags sysfs file for PCIe Endpoint devices

[Bjorn Helgaas <helgaas@kernel.org>]

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

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

   Maybe we just say "0" is invalid, since there's no obvious way to
   map this?

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

Both comments apply to the sriov files, too.

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

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

> +	 */
>  	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
>