Re: [PATCH v6 1/3] PCI/DOE: Expose the DOE features via sysfs

From: Chaitanya Kulkarni
Date: Fri Aug 18 2023 - 00:56:16 EST

Next message: Alexei Starovoitov: "Re: [RFC PATCH v2 1/5] mm, oom: Introduce bpf_oom_evaluate_task"
Previous message: Kees Cook: "[PATCH v2] creds: Convert cred.usage to refcount_t"
In reply to: Alistair Francis: "[PATCH v6 3/3] PCI/DOE: Only expose the sysfs attribute group if DOE is supported"
Next in thread: kernel test robot: "Re: [PATCH v6 1/3] PCI/DOE: Expose the DOE features via sysfs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 8/17/23 16:58, Alistair Francis wrote:
> The PCIe 6 specification added support for the Data Object Exchange (DOE).
> When DOE is supported the Discovery Data Object Protocol must be
> implemented. The protocol allows a requester to obtain information about
> the other DOE features supported by the device.
>
> The kernel is already querying the DOE features supported and cacheing
> the values. This patch exposes the values via sysfs. This will allow
> userspace to determine which DOE features are supported by the PCIe
> device.
>
> By exposing the information to userspace tools like lspci can relay the
> information to users. By listing all of the supported features we can
> allow userspace to parse and support the list, which might include
> vendor specific features as well as yet to be supported features.
>
> Signed-off-by: Alistair Francis <alistair.francis@xxxxxxx>
> ---
> v6:
> - Use "feature" instead of protocol
> - Don't use any devm_* functions
> - Add two more patches to the series
> v5:
> - Return the file name as the file contents
> - Code cleanups and simplifications
> v4:
> - Fixup typos in the documentation
> - Make it clear that the file names contain the information
> - Small code cleanups
> - Remove most #ifdefs
> - Remove extra NULL assignment
> v3:
> - Expose each DOE feature as a separate file
> v2:
> - Add documentation
> - Code cleanups
>
> This patch will create a doe_features directory for all
> PCIe devices without the next two patches
>
> Documentation/ABI/testing/sysfs-bus-pci | 11 +++
> drivers/pci/doe.c | 112 ++++++++++++++++++++++++
> drivers/pci/pci-sysfs.c | 10 +++
> drivers/pci/pci.h | 3 +
> include/linux/pci-doe.h | 1 +
> 5 files changed, 137 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index ecf47559f495..199ee5d27d9d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -500,3 +500,14 @@ Description:
> console drivers from the device. Raw users of pci-sysfs
> resourceN attributes must be terminated prior to resizing.
> Success of the resizing operation is not guaranteed.
> +
> +What: /sys/bus/pci/devices/.../doe_features
> +Date: August 2023
> +Contact: Linux PCI developers <linux-pci@xxxxxxxxxxxxxxx>
> +Description:
> + This directory contains a list of the supported
> + Data Object Exchange (DOE) features. The feature values are in the

overly long line above ...

> + file name. The contents of each file are the same as the name.
> + The value comes from the device and specifies the vendor and
> + data object type supported. The lower byte is the data object
> + type and the next two bytes are the vendor ID.
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 1b97a5ab71a9..316aac60ccd5 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -56,6 +56,8 @@ struct pci_doe_mb {
> wait_queue_head_t wq;
> struct workqueue_struct *work_queue;
> unsigned long flags;
> +
> + struct device_attribute *sysfs_attrs;

above naked declaration without CONFIG_SYSFS seems really odd
especially following code is under CONFIG_SYSFS, unless it is
accessed outside of CONFIG_SYSFS (which I don't think it is
a right interface) then ignore this comment ..

> };
>
> struct pci_doe_protocol {
> @@ -92,6 +94,116 @@ struct pci_doe_task {
> struct pci_doe_mb *doe_mb;
> };
>
> +#ifdef CONFIG_SYSFS
> +static umode_t pci_doe_sysfs_attr_is_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> + unsigned long total_features = 0;
> + struct pci_doe_mb *doe_mb;
> + unsigned long index, j;
> + void *entry;
> +
> + xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> + xa_for_each(&doe_mb->prots, j, entry)
> + total_features++;
> + }
> +
> + if (total_features == 0)
> + return 0;
> +
> + return a->mode;
> +}
> +

this removes the need for extra variable to avoids unnecessary
loop iterations unless there is bug in this suggestion, why not following ?

static umode_t pci_doe_sysfs_attr_is_visible(struct kobject *kobj,
struct attribute *a, int n)
{
struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
struct pci_doe_mb *doe_mb;
unsigned long index, j;
void *entry;

xa_for_each(&pdev->doe_mbs, index, doe_mb) {
xa_for_each(&doe_mb->prots, j, entry)
return a->mode;
}

return 0;
}

> +static struct attribute *pci_dev_doe_feature_attrs[] = {
> + NULL,
> +};
> +
> +const struct attribute_group pci_dev_doe_feature_group = {
> + .name = "doe_features",
> + .attrs = pci_dev_doe_feature_attrs,
> + .is_visible = pci_doe_sysfs_attr_is_visible,
> +};
> +
> +static ssize_t pci_doe_sysfs_feature_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return sysfs_emit(buf, "%s\n", attr->attr.name);
> +}
> +
> +static int pci_doe_sysfs_feature_supports(struct pci_dev *pdev,
> + struct pci_doe_mb *doe_mb)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_attribute *attrs;
> + unsigned long num_features = 0;
> + unsigned long vid, type;
> + unsigned long i;
> + void *entry;
> + int ret;
> +
> + xa_for_each(&doe_mb->prots, i, entry)
> + num_features++;
> +
> + attrs = kcalloc(num_features, sizeof(*attrs), GFP_KERNEL);
> + if (!attrs)
> + return -ENOMEM;
> +
> + doe_mb->sysfs_attrs = attrs;
> + xa_for_each(&doe_mb->prots, i, entry) {
> + sysfs_attr_init(&attrs[i].attr);
> + vid = xa_to_value(entry) >> 8;
> + type = xa_to_value(entry) & 0xFF;
> + attrs[i].attr.name = kasprintf(GFP_KERNEL,
> + "0x%04lX:%02lX", vid, type);
> + if (!attrs[i].attr.name) {
> + ret = -ENOMEM;
> + goto fail;
> + }
> +
> + attrs[i].attr.mode = 0444;
> + attrs[i].show = pci_doe_sysfs_feature_show;
> +
> + ret = sysfs_add_file_to_group(&dev->kobj, &attrs[i].attr,
> + pci_dev_doe_feature_group.name);
> + if (ret)
> + goto fail;
> + }
> +
> + return 0;
> +
> +fail:
> + doe_mb->sysfs_attrs = NULL;
> + xa_for_each(&doe_mb->prots, i, entry) {
> + if (attrs[i].show)
> + sysfs_remove_file_from_group(&dev->kobj, &attrs[i].attr,
> + pci_dev_doe_feature_group.name);
> + kfree(attrs[i].attr.name);
> + }
> +
> + kfree(attrs);
> +
> + return ret;
> +}
> +
> +int doe_sysfs_init(struct pci_dev *pdev)
> +{
> + struct pci_doe_mb *doe_mb;
> + unsigned long index;
> + int ret;
> +
> + xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> + ret = pci_doe_sysfs_feature_supports(pdev, doe_mb);
> +
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
> {
> if (wait_event_timeout(doe_mb->wq,
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index ab32a91f287b..3f5104cf78b6 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -16,6 +16,7 @@
> #include <linux/kernel.h>
> #include <linux/sched.h>
> #include <linux/pci.h>
> +#include <linux/pci-doe.h>
> #include <linux/stat.h>
> #include <linux/export.h>
> #include <linux/topology.h>
> @@ -1226,6 +1227,12 @@ static int pci_create_resource_files(struct pci_dev *pdev)
> int i;
> int retval;
>
> + if (IS_ENABLED(CONFIG_PCI_DOE)) {
> + retval = doe_sysfs_init(pdev);
> + if (retval)
> + return retval;
> + }
> +
> /* Expose the PCI resources from this device as files */
> for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>
> @@ -1651,6 +1658,9 @@ static const struct attribute_group *pci_dev_attr_groups[] = {
> #endif
> #ifdef CONFIG_PCIEASPM
> &aspm_ctrl_attr_group,
> +#endif
> +#ifdef CONFIG_PCI_DOE
> + &pci_dev_doe_feature_group,
> #endif
> NULL,
> };
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index a4c397434057..139d37a0d4cd 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -180,6 +180,9 @@ extern const struct attribute_group *pci_dev_groups[];
> extern const struct attribute_group *pcibus_groups[];
> extern const struct device_type pci_dev_type;
> extern const struct attribute_group *pci_bus_groups[];
> +#ifdef CONFIG_SYSFS
> +extern const struct attribute_group pci_dev_doe_feature_group;
> +#endif
>
> extern unsigned long pci_hotplug_io_size;
> extern unsigned long pci_hotplug_mmio_size;
> diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
> index 1f14aed4354b..4cc13d9ccb50 100644
> --- a/include/linux/pci-doe.h
> +++ b/include/linux/pci-doe.h
> @@ -22,4 +22,5 @@ int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type,
> const void *request, size_t request_sz,
> void *response, size_t response_sz);
>
> +int doe_sysfs_init(struct pci_dev *pci_dev);
> #endif

Next message: Alexei Starovoitov: "Re: [RFC PATCH v2 1/5] mm, oom: Introduce bpf_oom_evaluate_task"
Previous message: Kees Cook: "[PATCH v2] creds: Convert cred.usage to refcount_t"
In reply to: Alistair Francis: "[PATCH v6 3/3] PCI/DOE: Only expose the sysfs attribute group if DOE is supported"
Next in thread: kernel test robot: "Re: [PATCH v6 1/3] PCI/DOE: Expose the DOE features via sysfs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]