linux-kernel - Re: [PATCH v11 3/4] PCI/DOE: Expose the DOE features via sysfs

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] [day] [month] [year] [list]

Message-ID: <06a795fdb2ad92bbbeb659872759158e25e9c7d8.camel@wdc.com>
Date: Tue, 6 Aug 2024 06:36:32 +0000
From: Alistair Francis <Alistair.Francis@....com>
To: "lukas@...ner.de" <lukas@...ner.de>, "alistair23@...il.com"
	<alistair23@...il.com>
CC: "chaitanyak@...dia.com" <chaitanyak@...dia.com>, "rdunlap@...radead.org"
	<rdunlap@...radead.org>, "gregkh@...uxfoundation.org"
	<gregkh@...uxfoundation.org>, "alex.williamson@...hat.com"
	<alex.williamson@...hat.com>, "Jonathan.Cameron@...wei.com"
	<Jonathan.Cameron@...wei.com>, "christian.koenig@....com"
	<christian.koenig@....com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "kch@...dia.com" <kch@...dia.com>,
	"logang@...tatee.com" <logang@...tatee.com>, "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>, "bhelgaas@...gle.com" <bhelgaas@...gle.com>
Subject: Re: [PATCH v11 3/4] PCI/DOE: Expose the DOE features via sysfs

On Sat, 2024-06-15 at 15:05 +0200, Lukas Wunner wrote:
> On Fri, Jun 14, 2024 at 10:12:43AM +1000, Alistair Francis wrote:
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
> [...]
> > +#ifdef CONFIG_SYSFS
> > +static ssize_t doe_discovery_show(struct device *dev,
> > +				  struct device_attribute *attr,
> > +				  char *buf)
> > +{
> > +	return sysfs_emit(buf, "0001:00\n");
> > +}
> > +DEVICE_ATTR_RO(doe_discovery);
> 
> If you want to use "0001:00" as filename but can't because
> "0001:00_show()" would not be a valid function name in C,
> I think there's no harm in manually expanding the macro to:
> 
> struct device_attribute dev_attr_doe_discovery = \
> 	__ATTR(0001:00, 0444, pci_doe_sysfs_feature_show, NULL);
> 
> That also avoids the need to have an extra doe_discovery_show()
> function.
> 
> Intuitively, when I saw there's a "doe_discovery" attribute,
> my first thought was: "Oh maybe I need to write something there
> to (re-)initiate DOE discovery?"

I prefer the `doe_discovery` as it's clear what the protocol is, but if
it's preferred I can change it to `0001:00`

Alistair

> 
> 
> > +static umode_t pci_doe_features_sysfs_attr_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;
> > +	unsigned long vid, type;
> > +	void *entry;
> > +
> > +	xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> > +		xa_for_each(&doe_mb->feats, j, entry) {
> > +			vid = xa_to_value(entry) >> 8;
> > +			type = xa_to_value(entry) & 0xFF;
> > +
> > +			if (vid == 0x01 && type == 0x00) {
> 
> Wherever possible, PCI_VENDOR_ID_PCI_SIG and
> PCI_DOE_PROTOCOL_DISCOVERY
> macros should be used in lieu of 0x0001 and 0x00.
> 
> > +				/*
> > +				 * This is the DOE discovery
> > protocol
> > +				 * Every DOE instance must support
> > this, so we
> > +				 * give it a useful name.
> > +				 */
> > +				return a->mode;
> > +			}
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> 
> I agree with Jonathan that at first glance one would assume that
> this function just always returns a->mode.
> 
> 
> > +static bool pci_doe_features_sysfs_group_visible(struct kobject
> > *kobj)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> > +	struct pci_doe_mb *doe_mb;
> > +	unsigned long index;
> > +
> > +	xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> > +		if (!xa_empty(&doe_mb->feats))
> > +			return true;
> > +	}
> > +
> > +	return false;
> 
> So in principle, doe_mb->feats should never be empty because the
> discovery protocol is always supported, right?  Wouldn't it then
> suffice to just check for:
> 
> +	if (!xa_empty(&pdev->doe_mbs))
> +		return true;
> 
> Or alternatively:
> 
> +	return !xa_empty(&pdev->doe_mbs);
> 
> Thanks,
> 
> Lukas