[PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c

[PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c

[Kelsey Skunberg]

The sysfs SR-IOV functions are for an optional feature and will be better
organized to keep with the feature's code. Move the sysfs SR-IOV functions
to /pci/iov.c.

Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
 drivers/pci/iov.c       | 173 +++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci-sysfs.c | 176 ----------------------------------------
 drivers/pci/pci.h       |   2 +-
 3 files changed, 174 insertions(+), 177 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 9b48818ced01..661051adf23f 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -240,6 +240,179 @@ void pci_iov_remove_virtfn(struct pci_dev *dev, int id)
 	pci_dev_put(dev);
 }
 
+static ssize_t sriov_totalvfs_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
+}
+
+static ssize_t sriov_numvfs_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->num_VFs);
+}
+
+/*
+ * num_vfs > 0; number of VFs to enable
+ * num_vfs = 0; disable all VFs
+ *
+ * Note: SRIOV spec does not allow partial VF
+ *       disable, so it's all or none.
+ */
+static ssize_t sriov_numvfs_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	int ret;
+	u16 num_vfs;
+
+	ret = kstrtou16(buf, 0, &num_vfs);
+	if (ret < 0)
+		return ret;
+
+	if (num_vfs > pci_sriov_get_totalvfs(pdev))
+		return -ERANGE;
+
+	device_lock(&pdev->dev);
+
+	if (num_vfs == pdev->sriov->num_VFs)
+		goto exit;
+
+	/* is PF driver loaded w/callback */
+	if (!pdev->driver || !pdev->driver->sriov_configure) {
+		pci_info(pdev, "Driver does not support SRIOV configuration via sysfs\n");
+		ret = -ENOENT;
+		goto exit;
+	}
+
+	if (num_vfs == 0) {
+		/* disable VFs */
+		ret = pdev->driver->sriov_configure(pdev, 0);
+		goto exit;
+	}
+
+	/* enable VFs */
+	if (pdev->sriov->num_VFs) {
+		pci_warn(pdev, "%d VFs already enabled. Disable before enabling %d VFs\n",
+			 pdev->sriov->num_VFs, num_vfs);
+		ret = -EBUSY;
+		goto exit;
+	}
+
+	ret = pdev->driver->sriov_configure(pdev, num_vfs);
+	if (ret < 0)
+		goto exit;
+
+	if (ret != num_vfs)
+		pci_warn(pdev, "%d VFs requested; only %d enabled\n",
+			 num_vfs, ret);
+
+exit:
+	device_unlock(&pdev->dev);
+
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static ssize_t sriov_offset_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->offset);
+}
+
+static ssize_t sriov_stride_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->stride);
+}
+
+static ssize_t sriov_vf_device_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%x\n", pdev->sriov->vf_device);
+}
+
+static ssize_t sriov_drivers_autoprobe_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->drivers_autoprobe);
+}
+
+static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	bool drivers_autoprobe;
+
+	if (kstrtobool(buf, &drivers_autoprobe) < 0)
+		return -EINVAL;
+
+	pdev->sriov->drivers_autoprobe = drivers_autoprobe;
+
+	return count;
+}
+
+static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
+static struct device_attribute sriov_numvfs_attr =
+		__ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
+		       sriov_numvfs_show, sriov_numvfs_store);
+static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
+static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
+static struct device_attribute sriov_vf_device_attr =
+		__ATTR_RO(sriov_vf_device);
+static struct device_attribute sriov_drivers_autoprobe_attr =
+		__ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
+		       sriov_drivers_autoprobe_show,
+		       sriov_drivers_autoprobe_store);
+
+static struct attribute *sriov_dev_attrs[] = {
+	&sriov_totalvfs_attr.attr,
+	&sriov_numvfs_attr.attr,
+	&sriov_offset_attr.attr,
+	&sriov_stride_attr.attr,
+	&sriov_vf_device_attr.attr,
+	&sriov_drivers_autoprobe_attr.attr,
+	NULL,
+};
+
+static umode_t sriov_attrs_are_visible(struct kobject *kobj,
+				       struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+
+	if (!dev_is_pf(dev))
+		return 0;
+
+	return a->mode;
+}
+
+const struct attribute_group sriov_dev_attr_group = {
+	.attrs = sriov_dev_attrs,
+	.is_visible = sriov_attrs_are_visible,
+};
+
 int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
 {
 	return 0;
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 965c72104150..d5c35434810b 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -551,154 +551,6 @@ static ssize_t devspec_show(struct device *dev,
 static DEVICE_ATTR_RO(devspec);
 #endif
 
-#ifdef CONFIG_PCI_IOV
-static ssize_t sriov_totalvfs_show(struct device *dev,
-				   struct device_attribute *attr,
-				   char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
-}
-
-
-static ssize_t sriov_numvfs_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->num_VFs);
-}
-
-/*
- * num_vfs > 0; number of VFs to enable
- * num_vfs = 0; disable all VFs
- *
- * Note: SRIOV spec doesn't allow partial VF
- *       disable, so it's all or none.
- */
-static ssize_t sriov_numvfs_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t count)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	int ret;
-	u16 num_vfs;
-
-	ret = kstrtou16(buf, 0, &num_vfs);
-	if (ret < 0)
-		return ret;
-
-	if (num_vfs > pci_sriov_get_totalvfs(pdev))
-		return -ERANGE;
-
-	device_lock(&pdev->dev);
-
-	if (num_vfs == pdev->sriov->num_VFs)
-		goto exit;
-
-	/* is PF driver loaded w/callback */
-	if (!pdev->driver || !pdev->driver->sriov_configure) {
-		pci_info(pdev, "Driver doesn't support SRIOV configuration via sysfs\n");
-		ret = -ENOENT;
-		goto exit;
-	}
-
-	if (num_vfs == 0) {
-		/* disable VFs */
-		ret = pdev->driver->sriov_configure(pdev, 0);
-		goto exit;
-	}
-
-	/* enable VFs */
-	if (pdev->sriov->num_VFs) {
-		pci_warn(pdev, "%d VFs already enabled. Disable before enabling %d VFs\n",
-			 pdev->sriov->num_VFs, num_vfs);
-		ret = -EBUSY;
-		goto exit;
-	}
-
-	ret = pdev->driver->sriov_configure(pdev, num_vfs);
-	if (ret < 0)
-		goto exit;
-
-	if (ret != num_vfs)
-		pci_warn(pdev, "%d VFs requested; only %d enabled\n",
-			 num_vfs, ret);
-
-exit:
-	device_unlock(&pdev->dev);
-
-	if (ret < 0)
-		return ret;
-
-	return count;
-}
-
-static ssize_t sriov_offset_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->offset);
-}
-
-static ssize_t sriov_stride_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->stride);
-}
-
-static ssize_t sriov_vf_device_show(struct device *dev,
-				    struct device_attribute *attr,
-				    char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%x\n", pdev->sriov->vf_device);
-}
-
-static ssize_t sriov_drivers_autoprobe_show(struct device *dev,
-					    struct device_attribute *attr,
-					    char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->drivers_autoprobe);
-}
-
-static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
-					     struct device_attribute *attr,
-					     const char *buf, size_t count)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	bool drivers_autoprobe;
-
-	if (kstrtobool(buf, &drivers_autoprobe) < 0)
-		return -EINVAL;
-
-	pdev->sriov->drivers_autoprobe = drivers_autoprobe;
-
-	return count;
-}
-
-static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
-static struct device_attribute sriov_numvfs_attr =
-		__ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
-		       sriov_numvfs_show, sriov_numvfs_store);
-static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
-static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
-static struct device_attribute sriov_vf_device_attr = __ATTR_RO(sriov_vf_device);
-static struct device_attribute sriov_drivers_autoprobe_attr =
-		__ATTR(sriov_drivers_autoprobe, (S_IRUGO|S_IWUSR|S_IWGRP),
-		       sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
-#endif /* CONFIG_PCI_IOV */
-
 static ssize_t driver_override_store(struct device *dev,
 				     struct device_attribute *attr,
 				     const char *buf, size_t count)
@@ -1697,34 +1549,6 @@ static const struct attribute_group pci_dev_hp_attr_group = {
 	.is_visible = pci_dev_hp_attrs_are_visible,
 };
 
-#ifdef CONFIG_PCI_IOV
-static struct attribute *sriov_dev_attrs[] = {
-	&sriov_totalvfs_attr.attr,
-	&sriov_numvfs_attr.attr,
-	&sriov_offset_attr.attr,
-	&sriov_stride_attr.attr,
-	&sriov_vf_device_attr.attr,
-	&sriov_drivers_autoprobe_attr.attr,
-	NULL,
-};
-
-static umode_t sriov_attrs_are_visible(struct kobject *kobj,
-				       struct attribute *a, int n)
-{
-	struct device *dev = kobj_to_dev(kobj);
-
-	if (!dev_is_pf(dev))
-		return 0;
-
-	return a->mode;
-}
-
-static const struct attribute_group sriov_dev_attr_group = {
-	.attrs = sriov_dev_attrs,
-	.is_visible = sriov_attrs_are_visible,
-};
-#endif /* CONFIG_PCI_IOV */
-
 static const struct attribute_group pci_dev_attr_group = {
 	.attrs = pci_dev_dev_attrs,
 	.is_visible = pci_dev_attrs_are_visible,
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 61bbfd611140..7e3c6c8ae6f9 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -455,7 +455,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno);
 resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
 void pci_restore_iov_state(struct pci_dev *dev);
 int pci_iov_bus_range(struct pci_bus *bus);
-
+extern const struct attribute_group sriov_dev_attr_group;
 #else
 static inline int pci_iov_init(struct pci_dev *dev)
 {
-- 
2.20.1

Re: [Linux-kernel-mentees] [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c

[Greg KH]

On Fri, Aug 09, 2019 at 01:57:21PM -0600, Kelsey Skunberg wrote:
> +static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);

DEVICE_ATTR_RO() please.  This is a device attribute, not a "raw"
kobject attribute.

> +static struct device_attribute sriov_numvfs_attr =
> +		__ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> +		       sriov_numvfs_show, sriov_numvfs_store);
> +static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
> +static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
> +static struct device_attribute sriov_vf_device_attr =
> +		__ATTR_RO(sriov_vf_device);
> +static struct device_attribute sriov_drivers_autoprobe_attr =
> +		__ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> +		       sriov_drivers_autoprobe_show,
> +		       sriov_drivers_autoprobe_store);

Same for all of these, they should use DEVICE_ATTR* macros.

And why the odd permissions on 2 of these files?  Are you sure about
that?

thanks,

greg k-h

Re: [Linux-kernel-mentees] [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c

[Bjorn Helgaas]

On Sat, Aug 10, 2019 at 09:17:19AM +0200, Greg KH wrote:
> On Fri, Aug 09, 2019 at 01:57:21PM -0600, Kelsey Skunberg wrote:
> > +static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> 
> DEVICE_ATTR_RO() please.  This is a device attribute, not a "raw"
> kobject attribute.

This patch is just a move; here's the source of the line above:

> > -static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);

I certainly support using DEVICE_ATTR_RO() instead of __ATTR_RO(), but
that should be down with a separate patch so it's not buried in what
is otherwise a simple move.

> > +static struct device_attribute sriov_numvfs_attr =
> > +		__ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> > +		       sriov_numvfs_show, sriov_numvfs_store);
> > +static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
> > +static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
> > +static struct device_attribute sriov_vf_device_attr =
> > +		__ATTR_RO(sriov_vf_device);
> > +static struct device_attribute sriov_drivers_autoprobe_attr =
> > +		__ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> > +		       sriov_drivers_autoprobe_show,
> > +		       sriov_drivers_autoprobe_store);
> 
> Same for all of these, they should use DEVICE_ATTR* macros.
> 
> And why the odd permissions on 2 of these files?  Are you sure about
> that?

Same for these.  It'd be nice to fix them (and similar cases in
pci-sysfs.c, rpadlpar_sysfs.c, sgi_hotplug.c, slot.c) but in a
separate patch.

I think Kelsey did the right thing here by not mixing unrelated fixes
in with the code move.  A couple additional patches to change the
__ATTR() uses and the permissions (git grep "\<S_" finds several
possibilities) would be icing on the cake, but getting the SR-IOV
code all together is an improvement by itself.

Bjorn

Re: [Linux-kernel-mentees] [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c

[Greg KH]

On Sat, Aug 10, 2019 at 12:15:25PM -0500, Bjorn Helgaas wrote:
> On Sat, Aug 10, 2019 at 09:17:19AM +0200, Greg KH wrote:
> > On Fri, Aug 09, 2019 at 01:57:21PM -0600, Kelsey Skunberg wrote:
> > > +static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> > 
> > DEVICE_ATTR_RO() please.  This is a device attribute, not a "raw"
> > kobject attribute.
> 
> This patch is just a move; here's the source of the line above:
> 
> > > -static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> 
> I certainly support using DEVICE_ATTR_RO() instead of __ATTR_RO(), but
> that should be down with a separate patch so it's not buried in what
> is otherwise a simple move.
> 
> > > +static struct device_attribute sriov_numvfs_attr =
> > > +		__ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> > > +		       sriov_numvfs_show, sriov_numvfs_store);
> > > +static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
> > > +static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
> > > +static struct device_attribute sriov_vf_device_attr =
> > > +		__ATTR_RO(sriov_vf_device);
> > > +static struct device_attribute sriov_drivers_autoprobe_attr =
> > > +		__ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> > > +		       sriov_drivers_autoprobe_show,
> > > +		       sriov_drivers_autoprobe_store);
> > 
> > Same for all of these, they should use DEVICE_ATTR* macros.
> > 
> > And why the odd permissions on 2 of these files?  Are you sure about
> > that?
> 
> Same for these.  It'd be nice to fix them (and similar cases in
> pci-sysfs.c, rpadlpar_sysfs.c, sgi_hotplug.c, slot.c) but in a
> separate patch.
> 
> I think Kelsey did the right thing here by not mixing unrelated fixes
> in with the code move.  A couple additional patches to change the
> __ATTR() uses and the permissions (git grep "\<S_" finds several
> possibilities) would be icing on the cake, but getting the SR-IOV
> code all together is an improvement by itself.

Ah, ok, that makes more sense.  As long as this is patch 1/X, I'm fine
with it :)

thanks,

greg k-h

Re: [Linux-kernel-mentees] [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c

[Kelsey Skunberg]

On Sat, Aug 10, 2019 at 07:24:09PM +0200, Greg KH wrote:
> On Sat, Aug 10, 2019 at 12:15:25PM -0500, Bjorn Helgaas wrote:
> > On Sat, Aug 10, 2019 at 09:17:19AM +0200, Greg KH wrote:
> > > On Fri, Aug 09, 2019 at 01:57:21PM -0600, Kelsey Skunberg wrote:
> > > > +static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> > > 
> > > DEVICE_ATTR_RO() please.  This is a device attribute, not a "raw"
> > > kobject attribute.
> > 
> > This patch is just a move; here's the source of the line above:
> > 
> > > > -static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> > 
> > I certainly support using DEVICE_ATTR_RO() instead of __ATTR_RO(), but
> > that should be down with a separate patch so it's not buried in what
> > is otherwise a simple move.
> > 
> > > > +static struct device_attribute sriov_numvfs_attr =
> > > > +		__ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> > > > +		       sriov_numvfs_show, sriov_numvfs_store);
> > > > +static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
> > > > +static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
> > > > +static struct device_attribute sriov_vf_device_attr =
> > > > +		__ATTR_RO(sriov_vf_device);
> > > > +static struct device_attribute sriov_drivers_autoprobe_attr =
> > > > +		__ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> > > > +		       sriov_drivers_autoprobe_show,
> > > > +		       sriov_drivers_autoprobe_store);
> > > 
> > > Same for all of these, they should use DEVICE_ATTR* macros.
> > > 
> > > And why the odd permissions on 2 of these files?  Are you sure about
> > > that?
> > 
> > Same for these.  It'd be nice to fix them (and similar cases in
> > pci-sysfs.c, rpadlpar_sysfs.c, sgi_hotplug.c, slot.c) but in a
> > separate patch.
> > 
> > I think Kelsey did the right thing here by not mixing unrelated fixes
> > in with the code move.  A couple additional patches to change the
> > __ATTR() uses and the permissions (git grep "\<S_" finds several
> > possibilities) would be icing on the cake, but getting the SR-IOV
> > code all together is an improvement by itself.
> 
> Ah, ok, that makes more sense.  As long as this is patch 1/X, I'm fine
> with it :)
> 
> thanks,
> 
> greg k-h

I'll set up a series to cover these changes and submit it as a v2. Thank
you for reviewing Greg and Bjorn!

Cheers,
Kelsey

[PATCH v2 0/3] PCI: pci-sysfs.c cleanup

[Kelsey Skunberg]

This series is designed to clean up device attributes and permissions in
pci-sysfs.c. Then move the sysfs SR-IOV functions from pci-sysfs.c to
iov.c for better organization. Patches build off of each other.

Patch 1: Define device attributes with DEVICE_ATTR*() instead of __ATTR*().

Patch 2: Change permissions from symbolic to the preferred octal.

Patch 3: Move sysfs SR-IOV functions to iov.c to keep the feature's code
together.

Changes since v1:
        Add patch 1 and 2 to fix the way device attributes are defined
        and change permissions from symbolic to octal. Patch 3 which moves
        sysfs SR-IOV functions to iov.c will then apply cleaner.


Kelsey Skunberg (3):
  PCI: sysfs: Define device attributes with DEVICE_ATTR*()
  PCI: sysfs: Change permissions from symbolic to octal
  PCI/IOV: Move sysfs SR-IOV functions to iov.c

 drivers/pci/iov.c       | 168 +++++++++++++++++++++++++++++++
 drivers/pci/pci-sysfs.c | 217 ++++------------------------------------
 drivers/pci/pci.h       |   2 +-
 3 files changed, 188 insertions(+), 199 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*()

[Kelsey Skunberg]

Defining device attributes should be done through the helper
DEVICE_ATTR*(_name, _mode, _show, _store). Change all instances using
__ATTR*() to now use DEVICE_ATTR*().

Example of old:

struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show,
						  _store)

Example of new:

static DEVICE_ATTR(foo, S_IWUSR | S_IRUGO, show_foo, store_foo)

Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
 drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 965c72104150..8af7944fdccb 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static struct device_attribute dev_rescan_attr = __ATTR(rescan,
-							(S_IWUSR|S_IWGRP),
-							NULL, dev_rescan_store);
+static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
 
 static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
@@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
 	return count;
 }
-static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
-							(S_IWUSR|S_IWGRP),
-							NULL, remove_store);
+static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
+				  remove_store);
 
 static ssize_t dev_bus_rescan_store(struct device *dev,
 				    struct device_attribute *attr,
@@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
+static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
 
 #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
 static ssize_t d3cold_allowed_store(struct device *dev,
@@ -687,16 +684,14 @@ static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
 	return count;
 }
 
-static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
-static struct device_attribute sriov_numvfs_attr =
-		__ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
-		       sriov_numvfs_show, sriov_numvfs_store);
-static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
-static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
-static struct device_attribute sriov_vf_device_attr = __ATTR_RO(sriov_vf_device);
-static struct device_attribute sriov_drivers_autoprobe_attr =
-		__ATTR(sriov_drivers_autoprobe, (S_IRUGO|S_IWUSR|S_IWGRP),
-		       sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
+static DEVICE_ATTR_RO(sriov_totalvfs);
+static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
+				  sriov_numvfs_show, sriov_numvfs_store);
+static DEVICE_ATTR_RO(sriov_offset);
+static DEVICE_ATTR_RO(sriov_stride);
+static DEVICE_ATTR_RO(sriov_vf_device);
+static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
+		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
 #endif /* CONFIG_PCI_IOV */
 
 static ssize_t driver_override_store(struct device *dev,
@@ -792,7 +787,7 @@ static struct attribute *pcie_dev_attrs[] = {
 };
 
 static struct attribute *pcibus_attrs[] = {
-	&dev_attr_rescan.attr,
+	&dev_attr_bus_rescan.attr,
 	&dev_attr_cpuaffinity.attr,
 	&dev_attr_cpulistaffinity.attr,
 	NULL,
@@ -820,7 +815,7 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
 		!!(pdev->resource[PCI_ROM_RESOURCE].flags &
 		   IORESOURCE_ROM_SHADOW));
 }
-static struct device_attribute vga_attr = __ATTR_RO(boot_vga);
+static DEVICE_ATTR_RO(boot_vga);
 
 static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
 			       struct bin_attribute *bin_attr, char *buf,
@@ -1458,7 +1453,7 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-static struct device_attribute reset_attr = __ATTR(reset, 0200, NULL, reset_store);
+static DEVICE_ATTR(reset, 0200, NULL, reset_store);
 
 static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 {
@@ -1468,7 +1463,7 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 	pcie_aspm_create_sysfs_dev_files(dev);
 
 	if (dev->reset_fn) {
-		retval = device_create_file(&dev->dev, &reset_attr);
+		retval = device_create_file(&dev->dev, &dev_attr_reset);
 		if (retval)
 			goto error;
 	}
@@ -1553,7 +1548,7 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
 	pcie_vpd_remove_sysfs_dev_files(dev);
 	pcie_aspm_remove_sysfs_dev_files(dev);
 	if (dev->reset_fn) {
-		device_remove_file(&dev->dev, &reset_attr);
+		device_remove_file(&dev->dev, &dev_attr_reset);
 		dev->reset_fn = 0;
 	}
 }
@@ -1606,7 +1601,7 @@ static int __init pci_sysfs_init(void)
 late_initcall(pci_sysfs_init);
 
 static struct attribute *pci_dev_dev_attrs[] = {
-	&vga_attr.attr,
+	&dev_attr_boot_vga.attr,
 	NULL,
 };
 
@@ -1616,7 +1611,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	if (a == &vga_attr.attr)
+	if (a == &dev_attr_boot_vga.attr)
 		if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
 			return 0;
 
@@ -1624,8 +1619,8 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
 }
 
 static struct attribute *pci_dev_hp_attrs[] = {
-	&dev_remove_attr.attr,
-	&dev_rescan_attr.attr,
+	&dev_attr_remove.attr,
+	&dev_attr_rescan.attr,
 	NULL,
 };
 
@@ -1699,12 +1694,12 @@ static const struct attribute_group pci_dev_hp_attr_group = {
 
 #ifdef CONFIG_PCI_IOV
 static struct attribute *sriov_dev_attrs[] = {
-	&sriov_totalvfs_attr.attr,
-	&sriov_numvfs_attr.attr,
-	&sriov_offset_attr.attr,
-	&sriov_stride_attr.attr,
-	&sriov_vf_device_attr.attr,
-	&sriov_drivers_autoprobe_attr.attr,
+	&dev_attr_sriov_totalvfs.attr,
+	&dev_attr_sriov_numvfs.attr,
+	&dev_attr_sriov_offset.attr,
+	&dev_attr_sriov_stride.attr,
+	&dev_attr_sriov_vf_device.attr,
+	&dev_attr_sriov_drivers_autoprobe.attr,
 	NULL,
 };
 
-- 
2.20.1

Re: [Linux-kernel-mentees] [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*()

[Greg KH]

On Tue, Aug 13, 2019 at 02:45:11PM -0600, Kelsey Skunberg wrote:
> Defining device attributes should be done through the helper
> DEVICE_ATTR*(_name, _mode, _show, _store). Change all instances using
> __ATTR*() to now use DEVICE_ATTR*().
> 
> Example of old:
> 
> struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show,
> 						  _store)
> 
> Example of new:
> 
> static DEVICE_ATTR(foo, S_IWUSR | S_IRUGO, show_foo, store_foo)

Why not DEVICE_ATTR_RO() and DEVICE_ATTR_RW() and friends?  "Raw"
DEVICE_ATTR() should almost never be used unless the files have a very
strange mode setting.  And if that is true, they should be audited to
find out why their permissions are so strange from the rest of the
kernel defaults.

> 
> Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
> ---
>  drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
>  1 file changed, 27 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 965c72104150..8af7944fdccb 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
>  	}
>  	return count;
>  }
> -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> -							(S_IWUSR|S_IWGRP),
> -							NULL, dev_rescan_store);
> +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);

DEVICE_ATTR_WO()?

>  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
>  			    const char *buf, size_t count)
> @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
>  		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
>  	return count;
>  }
> -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> -							(S_IWUSR|S_IWGRP),
> -							NULL, remove_store);
> +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> +				  remove_store);

DEVICE_ATTR_WO()?

Ugh, no lockdep?  ick, ok, leave this as-is, crazy "remove" files...

>  
>  static ssize_t dev_bus_rescan_store(struct device *dev,
>  				    struct device_attribute *attr,
> @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
>  	}
>  	return count;
>  }
> -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);

DEVICE_ATTR_WO()?

>  
>  #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
>  static ssize_t d3cold_allowed_store(struct device *dev,
> @@ -687,16 +684,14 @@ static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
>  	return count;
>  }
>  
> -static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> -static struct device_attribute sriov_numvfs_attr =
> -		__ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
> -		       sriov_numvfs_show, sriov_numvfs_store);
> -static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
> -static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
> -static struct device_attribute sriov_vf_device_attr = __ATTR_RO(sriov_vf_device);
> -static struct device_attribute sriov_drivers_autoprobe_attr =
> -		__ATTR(sriov_drivers_autoprobe, (S_IRUGO|S_IWUSR|S_IWGRP),
> -		       sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> +static DEVICE_ATTR_RO(sriov_totalvfs);
> +static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> +				  sriov_numvfs_show, sriov_numvfs_store);

DEVICE_ATTR_RW()?

> +static DEVICE_ATTR_RO(sriov_offset);
> +static DEVICE_ATTR_RO(sriov_stride);
> +static DEVICE_ATTR_RO(sriov_vf_device);
> +static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> +		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);

DEVICE_ATTR_RW()?

>  #endif /* CONFIG_PCI_IOV */
>  
>  static ssize_t driver_override_store(struct device *dev,
> @@ -792,7 +787,7 @@ static struct attribute *pcie_dev_attrs[] = {
>  };
>  
>  static struct attribute *pcibus_attrs[] = {
> -	&dev_attr_rescan.attr,
> +	&dev_attr_bus_rescan.attr,
>  	&dev_attr_cpuaffinity.attr,
>  	&dev_attr_cpulistaffinity.attr,
>  	NULL,
> @@ -820,7 +815,7 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
>  		!!(pdev->resource[PCI_ROM_RESOURCE].flags &
>  		   IORESOURCE_ROM_SHADOW));
>  }
> -static struct device_attribute vga_attr = __ATTR_RO(boot_vga);
> +static DEVICE_ATTR_RO(boot_vga);
>  
>  static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
>  			       struct bin_attribute *bin_attr, char *buf,
> @@ -1458,7 +1453,7 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
>  	return count;
>  }
>  
> -static struct device_attribute reset_attr = __ATTR(reset, 0200, NULL, reset_store);
> +static DEVICE_ATTR(reset, 0200, NULL, reset_store);

DEVICE_ATTR_WO()?  Hm, root only, maybe not :(

>  
>  static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>  {
> @@ -1468,7 +1463,7 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>  	pcie_aspm_create_sysfs_dev_files(dev);
>  
>  	if (dev->reset_fn) {
> -		retval = device_create_file(&dev->dev, &reset_attr);
> +		retval = device_create_file(&dev->dev, &dev_attr_reset);

odds are this needs to be fixed up later to use attribute groups
properly.  But that's better left for another patch.

>  		if (retval)
>  			goto error;
>  	}
> @@ -1553,7 +1548,7 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
>  	pcie_vpd_remove_sysfs_dev_files(dev);
>  	pcie_aspm_remove_sysfs_dev_files(dev);
>  	if (dev->reset_fn) {
> -		device_remove_file(&dev->dev, &reset_attr);
> +		device_remove_file(&dev->dev, &dev_attr_reset);

Same here, attribute groups will handle this.

thanks,

greg k-h

Re: [Linux-kernel-mentees] [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*()

[Kelsey Skunberg]

On Wed, Aug 14, 2019 at 09:52:20AM +0200, Greg KH wrote:
> On Tue, Aug 13, 2019 at 02:45:11PM -0600, Kelsey Skunberg wrote:
> > Defining device attributes should be done through the helper
> > DEVICE_ATTR*(_name, _mode, _show, _store). Change all instances using
> > __ATTR*() to now use DEVICE_ATTR*().
> > 
> > Example of old:
> > 
> > struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show,
> > 						  _store)
> > 
> > Example of new:
> > 
> > static DEVICE_ATTR(foo, S_IWUSR | S_IRUGO, show_foo, store_foo)
> 
> Why not DEVICE_ATTR_RO() and DEVICE_ATTR_RW() and friends?  "Raw"
> DEVICE_ATTR() should almost never be used unless the files have a very
> strange mode setting.  And if that is true, they should be audited to
> find out why their permissions are so strange from the rest of the
> kernel defaults.
>

This makes sense. I'll put together a patch to change the DEVICE_ATTR()
applicable to be changed. Thank you, Greg!

-Kelsey

> > 
> > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
> > ---
> >  drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> >  1 file changed, 27 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 965c72104150..8af7944fdccb 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> >  	}
> >  	return count;
> >  }
> > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > -							(S_IWUSR|S_IWGRP),
> > -							NULL, dev_rescan_store);
> > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> 
> DEVICE_ATTR_WO()?
> 
> >  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> >  			    const char *buf, size_t count)
> > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> >  		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> >  	return count;
> >  }
> > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > -							(S_IWUSR|S_IWGRP),
> > -							NULL, remove_store);
> > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > +				  remove_store);
> 
> DEVICE_ATTR_WO()?
> 
> Ugh, no lockdep?  ick, ok, leave this as-is, crazy "remove" files...
> 
> >  
> >  static ssize_t dev_bus_rescan_store(struct device *dev,
> >  				    struct device_attribute *attr,
> > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> >  	}
> >  	return count;
> >  }
> > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> 
> DEVICE_ATTR_WO()?
> 
> >  
> >  #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
> >  static ssize_t d3cold_allowed_store(struct device *dev,
> > @@ -687,16 +684,14 @@ static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
> >  	return count;
> >  }
> >  
> > -static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> > -static struct device_attribute sriov_numvfs_attr =
> > -		__ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
> > -		       sriov_numvfs_show, sriov_numvfs_store);
> > -static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
> > -static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
> > -static struct device_attribute sriov_vf_device_attr = __ATTR_RO(sriov_vf_device);
> > -static struct device_attribute sriov_drivers_autoprobe_attr =
> > -		__ATTR(sriov_drivers_autoprobe, (S_IRUGO|S_IWUSR|S_IWGRP),
> > -		       sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> > +static DEVICE_ATTR_RO(sriov_totalvfs);
> > +static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> > +				  sriov_numvfs_show, sriov_numvfs_store);
> 
> DEVICE_ATTR_RW()?
> 
> > +static DEVICE_ATTR_RO(sriov_offset);
> > +static DEVICE_ATTR_RO(sriov_stride);
> > +static DEVICE_ATTR_RO(sriov_vf_device);
> > +static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> > +		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> 
> DEVICE_ATTR_RW()?
> 
> >  #endif /* CONFIG_PCI_IOV */
> >  
> >  static ssize_t driver_override_store(struct device *dev,
> > @@ -792,7 +787,7 @@ static struct attribute *pcie_dev_attrs[] = {
> >  };
> >  
> >  static struct attribute *pcibus_attrs[] = {
> > -	&dev_attr_rescan.attr,
> > +	&dev_attr_bus_rescan.attr,
> >  	&dev_attr_cpuaffinity.attr,
> >  	&dev_attr_cpulistaffinity.attr,
> >  	NULL,
> > @@ -820,7 +815,7 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
> >  		!!(pdev->resource[PCI_ROM_RESOURCE].flags &
> >  		   IORESOURCE_ROM_SHADOW));
> >  }
> > -static struct device_attribute vga_attr = __ATTR_RO(boot_vga);
> > +static DEVICE_ATTR_RO(boot_vga);
> >  
> >  static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
> >  			       struct bin_attribute *bin_attr, char *buf,
> > @@ -1458,7 +1453,7 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> >  	return count;
> >  }
> >  
> > -static struct device_attribute reset_attr = __ATTR(reset, 0200, NULL, reset_store);
> > +static DEVICE_ATTR(reset, 0200, NULL, reset_store);
> 
> DEVICE_ATTR_WO()?  Hm, root only, maybe not :(
> 
> >  
> >  static int pci_create_capabilities_sysfs(struct pci_dev *dev)
> >  {
> > @@ -1468,7 +1463,7 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
> >  	pcie_aspm_create_sysfs_dev_files(dev);
> >  
> >  	if (dev->reset_fn) {
> > -		retval = device_create_file(&dev->dev, &reset_attr);
> > +		retval = device_create_file(&dev->dev, &dev_attr_reset);
> 
> odds are this needs to be fixed up later to use attribute groups
> properly.  But that's better left for another patch.
> 
> >  		if (retval)
> >  			goto error;
> >  	}
> > @@ -1553,7 +1548,7 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
> >  	pcie_vpd_remove_sysfs_dev_files(dev);
> >  	pcie_aspm_remove_sysfs_dev_files(dev);
> >  	if (dev->reset_fn) {
> > -		device_remove_file(&dev->dev, &reset_attr);
> > +		device_remove_file(&dev->dev, &dev_attr_reset);
> 
> Same here, attribute groups will handle this.
> 
> thanks,
> 
> greg k-h

Re: [Linux-kernel-mentees] [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*()

[Kelsey Skunberg]

On Wed, Aug 14, 2019 at 09:52:20AM +0200, Greg KH wrote:
> On Tue, Aug 13, 2019 at 02:45:11PM -0600, Kelsey Skunberg wrote:
> > Defining device attributes should be done through the helper
> > DEVICE_ATTR*(_name, _mode, _show, _store). Change all instances using
> > __ATTR*() to now use DEVICE_ATTR*().
> > 
> > Example of old:
> > 
> > struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show,
> > 						  _store)
> > 
> > Example of new:
> > 
> > static DEVICE_ATTR(foo, S_IWUSR | S_IRUGO, show_foo, store_foo)
> 
> Why not DEVICE_ATTR_RO() and DEVICE_ATTR_RW() and friends?  "Raw"
> DEVICE_ATTR() should almost never be used unless the files have a very
> strange mode setting.  And if that is true, they should be audited to
> find out why their permissions are so strange from the rest of the
> kernel defaults.
>

I updated the commit log to show a more encouraged example and a couple
of the DEVICE_ATTR() into DEVICE_ATTR_WO() in a new patch while adding
that patch to the series.

> > 
> > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
> > ---
> >  drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> >  1 file changed, 27 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 965c72104150..8af7944fdccb 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> >  	}
> >  	return count;
> >  }
> > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > -							(S_IWUSR|S_IWGRP),
> > -							NULL, dev_rescan_store);
> > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> 
> DEVICE_ATTR_WO()?
>

This was changed.

> >  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> >  			    const char *buf, size_t count)
> > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> >  		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> >  	return count;
> >  }
> > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > -							(S_IWUSR|S_IWGRP),
> > -							NULL, remove_store);
> > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > +				  remove_store);
> 
> DEVICE_ATTR_WO()?
> 
> Ugh, no lockdep?  ick, ok, leave this as-is, crazy "remove" files...
> 
> >  
> >  static ssize_t dev_bus_rescan_store(struct device *dev,
> >  				    struct device_attribute *attr,
> > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> >  	}
> >  	return count;
> >  }
> > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> 
> DEVICE_ATTR_WO()?
>

As well as this one.

> >  
> >  #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
> >  static ssize_t d3cold_allowed_store(struct device *dev,
> > @@ -687,16 +684,14 @@ static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
> >  	return count;
> >  }
> >  
> > -static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> > -static struct device_attribute sriov_numvfs_attr =
> > -		__ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
> > -		       sriov_numvfs_show, sriov_numvfs_store);
> > -static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
> > -static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
> > -static struct device_attribute sriov_vf_device_attr = __ATTR_RO(sriov_vf_device);
> > -static struct device_attribute sriov_drivers_autoprobe_attr =
> > -		__ATTR(sriov_drivers_autoprobe, (S_IRUGO|S_IWUSR|S_IWGRP),
> > -		       sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> > +static DEVICE_ATTR_RO(sriov_totalvfs);
> > +static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> > +				  sriov_numvfs_show, sriov_numvfs_store);
> 
> DEVICE_ATTR_RW()?
> 
> > +static DEVICE_ATTR_RO(sriov_offset);
> > +static DEVICE_ATTR_RO(sriov_stride);
> > +static DEVICE_ATTR_RO(sriov_vf_device);
> > +static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> > +		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> 
> DEVICE_ATTR_RW()?
>

Since the DEVICE_ATTR_RW() would change the permissions to '0644', I left
these alone for now. Once confirmed which direction to go, it is my
intention to either add a comment stating the permission '0664' is okay to
use here, or changing the permissions accordingly. This will be
accomplished in a separate patch.


> >  #endif /* CONFIG_PCI_IOV */
> >  
> >  static ssize_t driver_override_store(struct device *dev,
> > @@ -792,7 +787,7 @@ static struct attribute *pcie_dev_attrs[] = {
> >  };
> >  
> >  static struct attribute *pcibus_attrs[] = {
> > -	&dev_attr_rescan.attr,
> > +	&dev_attr_bus_rescan.attr,
> >  	&dev_attr_cpuaffinity.attr,
> >  	&dev_attr_cpulistaffinity.attr,
> >  	NULL,
> > @@ -820,7 +815,7 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
> >  		!!(pdev->resource[PCI_ROM_RESOURCE].flags &
> >  		   IORESOURCE_ROM_SHADOW));
> >  }
> > -static struct device_attribute vga_attr = __ATTR_RO(boot_vga);
> > +static DEVICE_ATTR_RO(boot_vga);
> >  
> >  static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
> >  			       struct bin_attribute *bin_attr, char *buf,
> > @@ -1458,7 +1453,7 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> >  	return count;
> >  }
> >  
> > -static struct device_attribute reset_attr = __ATTR(reset, 0200, NULL, reset_store);
> > +static DEVICE_ATTR(reset, 0200, NULL, reset_store);
> 
> DEVICE_ATTR_WO()?  Hm, root only, maybe not :(
> 
> >  
> >  static int pci_create_capabilities_sysfs(struct pci_dev *dev)
> >  {
> > @@ -1468,7 +1463,7 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
> >  	pcie_aspm_create_sysfs_dev_files(dev);
> >  
> >  	if (dev->reset_fn) {
> > -		retval = device_create_file(&dev->dev, &reset_attr);
> > +		retval = device_create_file(&dev->dev, &dev_attr_reset);
> 
> odds are this needs to be fixed up later to use attribute groups
> properly.  But that's better left for another patch.
> 
> >  		if (retval)
> >  			goto error;
> >  	}
> > @@ -1553,7 +1548,7 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
> >  	pcie_vpd_remove_sysfs_dev_files(dev);
> >  	pcie_aspm_remove_sysfs_dev_files(dev);
> >  	if (dev->reset_fn) {
> > -		device_remove_file(&dev->dev, &reset_attr);
> > +		device_remove_file(&dev->dev, &dev_attr_reset);
> 
> Same here, attribute groups will handle this.
> 
> thanks,
> 
> greg k-h

Thank you for pointing these others out, too. I appreciate all your help
with this, Greg. Let me know if there's anything else you spot you'd like
changed.

-Kelsey

[PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal

[Kelsey Skunberg]

Symbolic permissions such as "(S_IWUSR | S_IWGRP)" are not
preferred and octal permissions should be used instead. Change all
symbolic permissions to octal permissions.

Example of old:

"(S_IWUSR | S_IWGRP)"

Example of new:

"0220"

Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
 drivers/pci/pci-sysfs.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 8af7944fdccb..346193ca4826 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -464,7 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
+static DEVICE_ATTR(rescan, 0220, NULL, dev_rescan_store);
 
 static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
@@ -478,7 +478,7 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
 	return count;
 }
-static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
+static DEVICE_ATTR_IGNORE_LOCKDEP(remove, 0220, NULL,
 				  remove_store);
 
 static ssize_t dev_bus_rescan_store(struct device *dev,
@@ -501,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
+static DEVICE_ATTR(bus_rescan, 0220, NULL, dev_bus_rescan_store);
 
 #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
 static ssize_t d3cold_allowed_store(struct device *dev,
@@ -685,13 +685,12 @@ static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
 }
 
 static DEVICE_ATTR_RO(sriov_totalvfs);
-static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
-				  sriov_numvfs_show, sriov_numvfs_store);
+static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
 static DEVICE_ATTR_RO(sriov_offset);
 static DEVICE_ATTR_RO(sriov_stride);
 static DEVICE_ATTR_RO(sriov_vf_device);
-static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
-		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
+static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
+		   sriov_drivers_autoprobe_store);
 #endif /* CONFIG_PCI_IOV */
 
 static ssize_t driver_override_store(struct device *dev,
@@ -1080,7 +1079,7 @@ void pci_create_legacy_files(struct pci_bus *b)
 	sysfs_bin_attr_init(b->legacy_io);
 	b->legacy_io->attr.name = "legacy_io";
 	b->legacy_io->size = 0xffff;
-	b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
+	b->legacy_io->attr.mode = 0600;
 	b->legacy_io->read = pci_read_legacy_io;
 	b->legacy_io->write = pci_write_legacy_io;
 	b->legacy_io->mmap = pci_mmap_legacy_io;
@@ -1094,7 +1093,7 @@ void pci_create_legacy_files(struct pci_bus *b)
 	sysfs_bin_attr_init(b->legacy_mem);
 	b->legacy_mem->attr.name = "legacy_mem";
 	b->legacy_mem->size = 1024*1024;
-	b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
+	b->legacy_mem->attr.mode = 0600;
 	b->legacy_mem->mmap = pci_mmap_legacy_mem;
 	pci_adjust_legacy_attr(b, pci_mmap_mem);
 	error = device_create_bin_file(&b->dev, b->legacy_mem);
@@ -1301,7 +1300,7 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
 		}
 	}
 	res_attr->attr.name = res_attr_name;
-	res_attr->attr.mode = S_IRUSR | S_IWUSR;
+	res_attr->attr.mode = 0600;
 	res_attr->size = pci_resource_len(pdev, num);
 	res_attr->private = (void *)(unsigned long)num;
 	retval = sysfs_create_bin_file(&pdev->dev.kobj, res_attr);
@@ -1414,7 +1413,7 @@ static ssize_t pci_read_rom(struct file *filp, struct kobject *kobj,
 static const struct bin_attribute pci_config_attr = {
 	.attr =	{
 		.name = "config",
-		.mode = S_IRUGO | S_IWUSR,
+		.mode = 0644,
 	},
 	.size = PCI_CFG_SPACE_SIZE,
 	.read = pci_read_config,
@@ -1424,7 +1423,7 @@ static const struct bin_attribute pci_config_attr = {
 static const struct bin_attribute pcie_config_attr = {
 	.attr =	{
 		.name = "config",
-		.mode = S_IRUGO | S_IWUSR,
+		.mode = 0644,
 	},
 	.size = PCI_CFG_SPACE_EXP_SIZE,
 	.read = pci_read_config,
@@ -1506,7 +1505,7 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
 		sysfs_bin_attr_init(attr);
 		attr->size = rom_size;
 		attr->attr.name = "rom";
-		attr->attr.mode = S_IRUSR | S_IWUSR;
+		attr->attr.mode = 0600;
 		attr->read = pci_read_rom;
 		attr->write = pci_write_rom;
 		retval = sysfs_create_bin_file(&pdev->dev.kobj, attr);
-- 
2.20.1

Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal

[Bjorn Helgaas]

[+cc Bodong, Don, Greg for permission question]

On Tue, Aug 13, 2019 at 02:45:12PM -0600, Kelsey Skunberg wrote:
> Symbolic permissions such as "(S_IWUSR | S_IWGRP)" are not
> preferred and octal permissions should be used instead. Change all
> symbolic permissions to octal permissions.
> 
> Example of old:
> 
> "(S_IWUSR | S_IWGRP)"
> 
> Example of new:
> 
> "0220"


>  static DEVICE_ATTR_RO(sriov_totalvfs);
> -static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> -				  sriov_numvfs_show, sriov_numvfs_store);
> +static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
>  static DEVICE_ATTR_RO(sriov_offset);
>  static DEVICE_ATTR_RO(sriov_stride);
>  static DEVICE_ATTR_RO(sriov_vf_device);
> -static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> -		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> +static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
> +		   sriov_drivers_autoprobe_store);

Greg noticed that sriov_numvfs and sriov_drivers_autoprobe have
"unusual" permissions.  These were added by:

  0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
  1789382a72a5 ("PCI: SRIOV control and status via sysfs")

Kelsey's patch correctly preserves the existing permissions, but we
should double-check that they are the permissions they want, and
possibly add a comment about why they're different from the rest.

Bjorn

Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal

[Greg Kroah-Hartman]

On Wed, Aug 14, 2019 at 12:38:46AM -0500, Bjorn Helgaas wrote:
> [+cc Bodong, Don, Greg for permission question]
> 
> On Tue, Aug 13, 2019 at 02:45:12PM -0600, Kelsey Skunberg wrote:
> > Symbolic permissions such as "(S_IWUSR | S_IWGRP)" are not
> > preferred and octal permissions should be used instead. Change all
> > symbolic permissions to octal permissions.
> > 
> > Example of old:
> > 
> > "(S_IWUSR | S_IWGRP)"
> > 
> > Example of new:
> > 
> > "0220"
> 
> 
> >  static DEVICE_ATTR_RO(sriov_totalvfs);
> > -static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> > -				  sriov_numvfs_show, sriov_numvfs_store);
> > +static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
> >  static DEVICE_ATTR_RO(sriov_offset);
> >  static DEVICE_ATTR_RO(sriov_stride);
> >  static DEVICE_ATTR_RO(sriov_vf_device);
> > -static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> > -		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> > +static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
> > +		   sriov_drivers_autoprobe_store);
> 
> Greg noticed that sriov_numvfs and sriov_drivers_autoprobe have
> "unusual" permissions.  These were added by:
> 
>   0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
>   1789382a72a5 ("PCI: SRIOV control and status via sysfs")
> 
> Kelsey's patch correctly preserves the existing permissions, but we
> should double-check that they are the permissions they want, and
> possibly add a comment about why they're different from the rest.

I agree.  And if those permissions are ok, please put a HUGE comment in
here saying why they are what they are and why they need to stay that
way so we don't have this conversation again in a few years :)

thanks,

greg k-h

Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal

[Don Dutile]

On 08/14/2019 01:38 AM, Bjorn Helgaas wrote:
> [+cc Bodong, Don, Greg for permission question]
> 
> On Tue, Aug 13, 2019 at 02:45:12PM -0600, Kelsey Skunberg wrote:
>> Symbolic permissions such as "(S_IWUSR | S_IWGRP)" are not
>> preferred and octal permissions should be used instead. Change all
>> symbolic permissions to octal permissions.
>>
>> Example of old:
>>
>> "(S_IWUSR | S_IWGRP)"
>>
>> Example of new:
>>
>> "0220"
> 
> 
>>   static DEVICE_ATTR_RO(sriov_totalvfs);
>> -static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
>> -				  sriov_numvfs_show, sriov_numvfs_store);
>> +static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
>>   static DEVICE_ATTR_RO(sriov_offset);
>>   static DEVICE_ATTR_RO(sriov_stride);
>>   static DEVICE_ATTR_RO(sriov_vf_device);
>> -static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
>> -		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
>> +static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
>> +		   sriov_drivers_autoprobe_store);
> 
> Greg noticed that sriov_numvfs and sriov_drivers_autoprobe have
> "unusual" permissions.  These were added by:
> 
>    0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
>    1789382a72a5 ("PCI: SRIOV control and status via sysfs")
> 
> Kelsey's patch correctly preserves the existing permissions, but we
> should double-check that they are the permissions they want, and
> possibly add a comment about why they're different from the rest.
> 
> Bjorn
> 
The rest being? ... 0644 vs 0664 ?
The file is read & written, thus the (first) 6; I'll have to dig through very old (7 yr) notes to see if the second 6 is needed for libvirt (so it doesn't have to be root to enable).

-dd

Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal

[Kelsey Skunberg]

On Thu, Aug 15, 2019 at 10:37:13AM -0400, Don Dutile wrote:
> On 08/14/2019 01:38 AM, Bjorn Helgaas wrote:
> > [+cc Bodong, Don, Greg for permission question]
> > 
> > On Tue, Aug 13, 2019 at 02:45:12PM -0600, Kelsey Skunberg wrote:
> > > Symbolic permissions such as "(S_IWUSR | S_IWGRP)" are not
> > > preferred and octal permissions should be used instead. Change all
> > > symbolic permissions to octal permissions.
> > > 
> > > Example of old:
> > > 
> > > "(S_IWUSR | S_IWGRP)"
> > > 
> > > Example of new:
> > > 
> > > "0220"
> > 
> > 
> > >   static DEVICE_ATTR_RO(sriov_totalvfs);
> > > -static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> > > -				  sriov_numvfs_show, sriov_numvfs_store);
> > > +static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
> > >   static DEVICE_ATTR_RO(sriov_offset);
> > >   static DEVICE_ATTR_RO(sriov_stride);
> > >   static DEVICE_ATTR_RO(sriov_vf_device);
> > > -static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> > > -		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> > > +static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
> > > +		   sriov_drivers_autoprobe_store);
> > 
> > Greg noticed that sriov_numvfs and sriov_drivers_autoprobe have
> > "unusual" permissions.  These were added by:
> > 
> >    0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
> >    1789382a72a5 ("PCI: SRIOV control and status via sysfs")
> > 
> > Kelsey's patch correctly preserves the existing permissions, but we
> > should double-check that they are the permissions they want, and
> > possibly add a comment about why they're different from the rest.
> > 
> > Bjorn
> > 

Hi Don,

> The rest being? ... 0644 vs 0664 ?
> The file is read & written, thus the (first) 6; I'll have to dig through very old (7 yr) notes to see if the second 6 is needed for libvirt (so it doesn't have to be root to enable).
> 
> -dd
>

Were you able to see if the unusual permissions (0664) are needed for
libvirt? I appreciate your help!

-Kelsey

Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal

[Don Dutile]

On 09/04/2019 02:22 AM, Kelsey Skunberg wrote:
> On Thu, Aug 15, 2019 at 10:37:13AM -0400, Don Dutile wrote:
>> On 08/14/2019 01:38 AM, Bjorn Helgaas wrote:
>>> [+cc Bodong, Don, Greg for permission question]
>>>
>>> On Tue, Aug 13, 2019 at 02:45:12PM -0600, Kelsey Skunberg wrote:
>>>> Symbolic permissions such as "(S_IWUSR | S_IWGRP)" are not
>>>> preferred and octal permissions should be used instead. Change all
>>>> symbolic permissions to octal permissions.
>>>>
>>>> Example of old:
>>>>
>>>> "(S_IWUSR | S_IWGRP)"
>>>>
>>>> Example of new:
>>>>
>>>> "0220"
>>>
>>>
>>>>    static DEVICE_ATTR_RO(sriov_totalvfs);
>>>> -static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
>>>> -				  sriov_numvfs_show, sriov_numvfs_store);
>>>> +static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
>>>>    static DEVICE_ATTR_RO(sriov_offset);
>>>>    static DEVICE_ATTR_RO(sriov_stride);
>>>>    static DEVICE_ATTR_RO(sriov_vf_device);
>>>> -static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
>>>> -		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
>>>> +static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
>>>> +		   sriov_drivers_autoprobe_store);
>>>
>>> Greg noticed that sriov_numvfs and sriov_drivers_autoprobe have
>>> "unusual" permissions.  These were added by:
>>>
>>>     0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
>>>     1789382a72a5 ("PCI: SRIOV control and status via sysfs")
>>>
>>> Kelsey's patch correctly preserves the existing permissions, but we
>>> should double-check that they are the permissions they want, and
>>> possibly add a comment about why they're different from the rest.
>>>
>>> Bjorn
>>>
> 
> Hi Don,
> 
>> The rest being? ... 0644 vs 0664 ?
>> The file is read & written, thus the (first) 6; I'll have to dig through very old (7 yr) notes to see if the second 6 is needed for libvirt (so it doesn't have to be root to enable).
>>
>> -dd
>>
> 
> Were you able to see if the unusual permissions (0664) are needed for
> libvirt? I appreciate your help!
> 
> -Kelsey
> 
Asking libvirt team in RH; will get back as soon as I hear back.
LPC time sink may delay the response.

-dd

Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal

[Don Dutile]

On 09/04/2019 02:22 AM, Kelsey Skunberg wrote:
> On Thu, Aug 15, 2019 at 10:37:13AM -0400, Don Dutile wrote:
>> On 08/14/2019 01:38 AM, Bjorn Helgaas wrote:
>>> [+cc Bodong, Don, Greg for permission question]
>>>
>>> On Tue, Aug 13, 2019 at 02:45:12PM -0600, Kelsey Skunberg wrote:
>>>> Symbolic permissions such as "(S_IWUSR | S_IWGRP)" are not
>>>> preferred and octal permissions should be used instead. Change all
>>>> symbolic permissions to octal permissions.
>>>>
>>>> Example of old:
>>>>
>>>> "(S_IWUSR | S_IWGRP)"
>>>>
>>>> Example of new:
>>>>
>>>> "0220"
>>>
>>>
>>>>    static DEVICE_ATTR_RO(sriov_totalvfs);
>>>> -static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
>>>> -				  sriov_numvfs_show, sriov_numvfs_store);
>>>> +static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
>>>>    static DEVICE_ATTR_RO(sriov_offset);
>>>>    static DEVICE_ATTR_RO(sriov_stride);
>>>>    static DEVICE_ATTR_RO(sriov_vf_device);
>>>> -static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
>>>> -		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
>>>> +static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
>>>> +		   sriov_drivers_autoprobe_store);
>>>
>>> Greg noticed that sriov_numvfs and sriov_drivers_autoprobe have
>>> "unusual" permissions.  These were added by:
>>>
>>>     0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
>>>     1789382a72a5 ("PCI: SRIOV control and status via sysfs")
>>>
>>> Kelsey's patch correctly preserves the existing permissions, but we
>>> should double-check that they are the permissions they want, and
>>> possibly add a comment about why they're different from the rest.
>>>
>>> Bjorn
>>>
> 
> Hi Don,
> 
>> The rest being? ... 0644 vs 0664 ?
>> The file is read & written, thus the (first) 6; I'll have to dig through very old (7 yr) notes to see if the second 6 is needed for libvirt (so it doesn't have to be root to enable).
>>
>> -dd
>>
> 
> Were you able to see if the unusual permissions (0664) are needed for
> libvirt? I appreciate your help!
> 
> -Kelsey
> 
Daniel Berrangé reported that libvirt runs as root when dealing with anything PCI, and chowns files for qemu needs, so there is no need for the 664 permission.
For all I know, it's a simple typo that was allowed to creep in. :-/

Feel free to modify to 644.

-dd

Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal

[Kelsey Skunberg]

On Wed, Sep 04, 2019 at 02:33:44PM -0400, Don Dutile wrote:
> On 09/04/2019 02:22 AM, Kelsey Skunberg wrote:
> > On Thu, Aug 15, 2019 at 10:37:13AM -0400, Don Dutile wrote:
> > > On 08/14/2019 01:38 AM, Bjorn Helgaas wrote:
> > > > [+cc Bodong, Don, Greg for permission question]
> > > > 
> > > > On Tue, Aug 13, 2019 at 02:45:12PM -0600, Kelsey Skunberg wrote:
> > > > > Symbolic permissions such as "(S_IWUSR | S_IWGRP)" are not
> > > > > preferred and octal permissions should be used instead. Change all
> > > > > symbolic permissions to octal permissions.
> > > > > 
> > > > > Example of old:
> > > > > 
> > > > > "(S_IWUSR | S_IWGRP)"
> > > > > 
> > > > > Example of new:
> > > > > 
> > > > > "0220"
> > > > 
> > > > 
> > > > >    static DEVICE_ATTR_RO(sriov_totalvfs);
> > > > > -static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> > > > > -				  sriov_numvfs_show, sriov_numvfs_store);
> > > > > +static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
> > > > >    static DEVICE_ATTR_RO(sriov_offset);
> > > > >    static DEVICE_ATTR_RO(sriov_stride);
> > > > >    static DEVICE_ATTR_RO(sriov_vf_device);
> > > > > -static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> > > > > -		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> > > > > +static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
> > > > > +		   sriov_drivers_autoprobe_store);
> > > > 
> > > > Greg noticed that sriov_numvfs and sriov_drivers_autoprobe have
> > > > "unusual" permissions.  These were added by:
> > > > 
> > > >     0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
> > > >     1789382a72a5 ("PCI: SRIOV control and status via sysfs")
> > > > 
> > > > Kelsey's patch correctly preserves the existing permissions, but we
> > > > should double-check that they are the permissions they want, and
> > > > possibly add a comment about why they're different from the rest.
> > > > 
> > > > Bjorn
> > > > 
> > 
> > Hi Don,
> > 
> > > The rest being? ... 0644 vs 0664 ?
> > > The file is read & written, thus the (first) 6; I'll have to dig through very old (7 yr) notes to see if the second 6 is needed for libvirt (so it doesn't have to be root to enable).
> > > 
> > > -dd
> > > 
> > 
> > Were you able to see if the unusual permissions (0664) are needed for
> > libvirt? I appreciate your help!
> > 
> > -Kelsey
> > 
> Daniel Berrangé reported that libvirt runs as root when dealing with anything PCI, and chowns files for qemu needs, so there is no need for the 664 permission.
> For all I know, it's a simple typo that was allowed to creep in. :-/
> 
> Feel free to modify to 644.
> 
> -dd
>

Thank you for checking into this and getting back so quick! I'll cc you in
the patch. :)

Thanks again!

-Kelsey

[PATCH v2 3/3] PCI/IOV: Move sysfs SR-IOV functions to iov.c

[Kelsey Skunberg]

The sysfs SR-IOV functions are for an optional feature and will be better
organized to keep with the feature's code. Move the sysfs SR-IOV functions
to /pci/iov.c.

Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
 drivers/pci/iov.c       | 168 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci-sysfs.c | 173 ----------------------------------------
 drivers/pci/pci.h       |   2 +-
 3 files changed, 169 insertions(+), 174 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 9b48818ced01..b335db21c85e 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -240,6 +240,174 @@ void pci_iov_remove_virtfn(struct pci_dev *dev, int id)
 	pci_dev_put(dev);
 }
 
+static ssize_t sriov_totalvfs_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
+}
+
+static ssize_t sriov_numvfs_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->num_VFs);
+}
+
+/*
+ * num_vfs > 0; number of VFs to enable
+ * num_vfs = 0; disable all VFs
+ *
+ * Note: SRIOV spec does not allow partial VF
+ *	 disable, so it's all or none.
+ */
+static ssize_t sriov_numvfs_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	int ret;
+	u16 num_vfs;
+
+	ret = kstrtou16(buf, 0, &num_vfs);
+	if (ret < 0)
+		return ret;
+
+	if (num_vfs > pci_sriov_get_totalvfs(pdev))
+		return -ERANGE;
+
+	device_lock(&pdev->dev);
+
+	if (num_vfs == pdev->sriov->num_VFs)
+		goto exit;
+
+	/* is PF driver loaded w/callback */
+	if (!pdev->driver || !pdev->driver->sriov_configure) {
+		pci_info(pdev, "Driver does not support SRIOV configuration via sysfs\n");
+		ret = -ENOENT;
+		goto exit;
+	}
+
+	if (num_vfs == 0) {
+		/* disable VFs */
+		ret = pdev->driver->sriov_configure(pdev, 0);
+		goto exit;
+	}
+
+	/* enable VFs */
+	if (pdev->sriov->num_VFs) {
+		pci_warn(pdev, "%d VFs already enabled. Disable before enabling %d VFs\n",
+			 pdev->sriov->num_VFs, num_vfs);
+		ret = -EBUSY;
+		goto exit;
+	}
+
+	ret = pdev->driver->sriov_configure(pdev, num_vfs);
+	if (ret < 0)
+		goto exit;
+
+	if (ret != num_vfs)
+		pci_warn(pdev, "%d VFs requested; only %d enabled\n",
+			 num_vfs, ret);
+
+exit:
+	device_unlock(&pdev->dev);
+
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static ssize_t sriov_offset_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->offset);
+}
+
+static ssize_t sriov_stride_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->stride);
+}
+
+static ssize_t sriov_vf_device_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%x\n", pdev->sriov->vf_device);
+}
+
+static ssize_t sriov_drivers_autoprobe_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->drivers_autoprobe);
+}
+
+static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	bool drivers_autoprobe;
+
+	if (kstrtobool(buf, &drivers_autoprobe) < 0)
+		return -EINVAL;
+
+	pdev->sriov->drivers_autoprobe = drivers_autoprobe;
+
+	return count;
+}
+
+static DEVICE_ATTR_RO(sriov_totalvfs);
+static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
+static DEVICE_ATTR_RO(sriov_offset);
+static DEVICE_ATTR_RO(sriov_stride);
+static DEVICE_ATTR_RO(sriov_vf_device);
+static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
+		   sriov_drivers_autoprobe_store);
+
+static struct attribute *sriov_dev_attrs[] = {
+	&dev_attr_sriov_totalvfs.attr,
+	&dev_attr_sriov_numvfs.attr,
+	&dev_attr_sriov_offset.attr,
+	&dev_attr_sriov_stride.attr,
+	&dev_attr_sriov_vf_device.attr,
+	&dev_attr_sriov_drivers_autoprobe.attr,
+	NULL,
+};
+
+static umode_t sriov_attrs_are_visible(struct kobject *kobj,
+				       struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+
+	if (!dev_is_pf(dev))
+		return 0;
+
+	return a->mode;
+}
+
+const struct attribute_group sriov_dev_attr_group = {
+	.attrs = sriov_dev_attrs,
+	.is_visible = sriov_attrs_are_visible,
+};
+
 int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
 {
 	return 0;
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 346193ca4826..f48af6e01bb0 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -548,151 +548,6 @@ static ssize_t devspec_show(struct device *dev,
 static DEVICE_ATTR_RO(devspec);
 #endif
 
-#ifdef CONFIG_PCI_IOV
-static ssize_t sriov_totalvfs_show(struct device *dev,
-				   struct device_attribute *attr,
-				   char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
-}
-
-
-static ssize_t sriov_numvfs_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->num_VFs);
-}
-
-/*
- * num_vfs > 0; number of VFs to enable
- * num_vfs = 0; disable all VFs
- *
- * Note: SRIOV spec doesn't allow partial VF
- *       disable, so it's all or none.
- */
-static ssize_t sriov_numvfs_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t count)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	int ret;
-	u16 num_vfs;
-
-	ret = kstrtou16(buf, 0, &num_vfs);
-	if (ret < 0)
-		return ret;
-
-	if (num_vfs > pci_sriov_get_totalvfs(pdev))
-		return -ERANGE;
-
-	device_lock(&pdev->dev);
-
-	if (num_vfs == pdev->sriov->num_VFs)
-		goto exit;
-
-	/* is PF driver loaded w/callback */
-	if (!pdev->driver || !pdev->driver->sriov_configure) {
-		pci_info(pdev, "Driver doesn't support SRIOV configuration via sysfs\n");
-		ret = -ENOENT;
-		goto exit;
-	}
-
-	if (num_vfs == 0) {
-		/* disable VFs */
-		ret = pdev->driver->sriov_configure(pdev, 0);
-		goto exit;
-	}
-
-	/* enable VFs */
-	if (pdev->sriov->num_VFs) {
-		pci_warn(pdev, "%d VFs already enabled. Disable before enabling %d VFs\n",
-			 pdev->sriov->num_VFs, num_vfs);
-		ret = -EBUSY;
-		goto exit;
-	}
-
-	ret = pdev->driver->sriov_configure(pdev, num_vfs);
-	if (ret < 0)
-		goto exit;
-
-	if (ret != num_vfs)
-		pci_warn(pdev, "%d VFs requested; only %d enabled\n",
-			 num_vfs, ret);
-
-exit:
-	device_unlock(&pdev->dev);
-
-	if (ret < 0)
-		return ret;
-
-	return count;
-}
-
-static ssize_t sriov_offset_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->offset);
-}
-
-static ssize_t sriov_stride_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->stride);
-}
-
-static ssize_t sriov_vf_device_show(struct device *dev,
-				    struct device_attribute *attr,
-				    char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%x\n", pdev->sriov->vf_device);
-}
-
-static ssize_t sriov_drivers_autoprobe_show(struct device *dev,
-					    struct device_attribute *attr,
-					    char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->drivers_autoprobe);
-}
-
-static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
-					     struct device_attribute *attr,
-					     const char *buf, size_t count)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	bool drivers_autoprobe;
-
-	if (kstrtobool(buf, &drivers_autoprobe) < 0)
-		return -EINVAL;
-
-	pdev->sriov->drivers_autoprobe = drivers_autoprobe;
-
-	return count;
-}
-
-static DEVICE_ATTR_RO(sriov_totalvfs);
-static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
-static DEVICE_ATTR_RO(sriov_offset);
-static DEVICE_ATTR_RO(sriov_stride);
-static DEVICE_ATTR_RO(sriov_vf_device);
-static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
-		   sriov_drivers_autoprobe_store);
-#endif /* CONFIG_PCI_IOV */
-
 static ssize_t driver_override_store(struct device *dev,
 				     struct device_attribute *attr,
 				     const char *buf, size_t count)
@@ -1691,34 +1546,6 @@ static const struct attribute_group pci_dev_hp_attr_group = {
 	.is_visible = pci_dev_hp_attrs_are_visible,
 };
 
-#ifdef CONFIG_PCI_IOV
-static struct attribute *sriov_dev_attrs[] = {
-	&dev_attr_sriov_totalvfs.attr,
-	&dev_attr_sriov_numvfs.attr,
-	&dev_attr_sriov_offset.attr,
-	&dev_attr_sriov_stride.attr,
-	&dev_attr_sriov_vf_device.attr,
-	&dev_attr_sriov_drivers_autoprobe.attr,
-	NULL,
-};
-
-static umode_t sriov_attrs_are_visible(struct kobject *kobj,
-				       struct attribute *a, int n)
-{
-	struct device *dev = kobj_to_dev(kobj);
-
-	if (!dev_is_pf(dev))
-		return 0;
-
-	return a->mode;
-}
-
-static const struct attribute_group sriov_dev_attr_group = {
-	.attrs = sriov_dev_attrs,
-	.is_visible = sriov_attrs_are_visible,
-};
-#endif /* CONFIG_PCI_IOV */
-
 static const struct attribute_group pci_dev_attr_group = {
 	.attrs = pci_dev_dev_attrs,
 	.is_visible = pci_dev_attrs_are_visible,
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 61bbfd611140..7e3c6c8ae6f9 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -455,7 +455,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno);
 resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
 void pci_restore_iov_state(struct pci_dev *dev);
 int pci_iov_bus_range(struct pci_bus *bus);
-
+extern const struct attribute_group sriov_dev_attr_group;
 #else
 static inline int pci_iov_init(struct pci_dev *dev)
 {
-- 
2.20.1

Re: [Linux-kernel-mentees] [PATCH v2 0/3] PCI: pci-sysfs.c cleanup

[Bjorn Helgaas]

[+cc Greg]

On Tue, Aug 13, 2019 at 02:45:10PM -0600, Kelsey Skunberg wrote:
> This series is designed to clean up device attributes and permissions in
> pci-sysfs.c. Then move the sysfs SR-IOV functions from pci-sysfs.c to
> iov.c for better organization. Patches build off of each other.
> 
> Patch 1: Define device attributes with DEVICE_ATTR*() instead of __ATTR*().
> 
> Patch 2: Change permissions from symbolic to the preferred octal.
> 
> Patch 3: Move sysfs SR-IOV functions to iov.c to keep the feature's code
> together.
> 
> Changes since v1:
>         Add patch 1 and 2 to fix the way device attributes are defined
>         and change permissions from symbolic to octal. Patch 3 which moves
>         sysfs SR-IOV functions to iov.c will then apply cleaner.
> 
> 
> Kelsey Skunberg (3):
>   PCI: sysfs: Define device attributes with DEVICE_ATTR*()
>   PCI: sysfs: Change permissions from symbolic to octal
>   PCI/IOV: Move sysfs SR-IOV functions to iov.c
> 
>  drivers/pci/iov.c       | 168 +++++++++++++++++++++++++++++++
>  drivers/pci/pci-sysfs.c | 217 ++++------------------------------------
>  drivers/pci/pci.h       |   2 +-
>  3 files changed, 188 insertions(+), 199 deletions(-)

Applied to pci/virtualization for v5.4, thanks!

Beginning of thread:
https://lore.kernel.org/r/20190813204513.4790-1-skunberg.kelsey@gmail.com

[PATCH v3 0/4] PCI: Clean up pci-sysfs.c

[Kelsey Skunberg]

This series is designed to clean up device attributes and permissions in
pci-sysfs.c. Then move the sysfs SR-IOV functions from pci-sysfs.c to
iov.c for better organization.

Patch 1: Define device attributes with DEVICE_ATTR* instead of __ATTR*.

Patch 2: Change permissions from symbolic to the preferred octal.

Patch 3: Change DEVICE_ATTR() with 0220 permissions to DEVICE_ATTR_WO().

Patch 4: Move sysfs SR-IOV functions to iov.c to keep the feature's code
together.


Patch 1, 2, and 4 will report unusual permissions '0664' used from the
following:

  static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show,
                     sriov_numvfs_store);

  static DEVICE_ATTR(sriov_drivers_autoprobe, 0664,
                     sriov_drivers_autoprobe_show,
                     sriov_drivers_autoprobe_store);

This series preserves the existing permissions set in:


  commit 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control
                        VF driver binding")

  commit 1789382a72a5 ("PCI: SRIOV control and status via sysfs")

Either adding a comment verifying permissions are okay or changing the
permissions is to be completed with a new patch.

Changes since v1:
        Add patch 1 and 2 to fix the way device attributes are defined
        and change permissions from symbolic to octal. Patch 4 which moves
        sysfs SR-IOV functions to iov.c will then apply cleaner.

Changes since v2:

        Patch 1: Commit log updated. Example shows DEVICE_ATTR_RO()
        example instead of DEVICE_ATTR(). DEVICE_ATTR() should be avoided
        unless the files have unusual permissions. Changed to reflect a
        more encouraged usage.  Also updated regex to be accurate.

        Patch 3: [NEW] Add patch to change DEVICE_ATTR() with 0220
        permissions to DEVICE_ATTR_WO().

        Updated series log to reflect new patch and unusual permissions
        information.


Kelsey Skunberg (4):
  PCI: sysfs: Define device attributes with DEVICE_ATTR*
  PCI: sysfs: Change permissions from symbolic to octal
  PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()
  PCI/IOV: Move sysfs SR-IOV functions to iov.c

 drivers/pci/iov.c       | 168 ++++++++++++++++++++++++++++++
 drivers/pci/pci-sysfs.c | 223 ++++------------------------------------
 drivers/pci/pci.h       |   2 +-
 3 files changed, 191 insertions(+), 202 deletions(-)

-- 
2.20.1

Re: [PATCH v3 0/4] PCI: Clean up pci-sysfs.c

[Greg KH]

On Thu, Aug 15, 2019 at 09:33:49AM -0600, Kelsey Skunberg wrote:
> This series is designed to clean up device attributes and permissions in
> pci-sysfs.c. Then move the sysfs SR-IOV functions from pci-sysfs.c to
> iov.c for better organization.
> 
> Patch 1: Define device attributes with DEVICE_ATTR* instead of __ATTR*.
> 
> Patch 2: Change permissions from symbolic to the preferred octal.
> 
> Patch 3: Change DEVICE_ATTR() with 0220 permissions to DEVICE_ATTR_WO().
> 
> Patch 4: Move sysfs SR-IOV functions to iov.c to keep the feature's code
> together.

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v3 0/4] PCI: Clean up pci-sysfs.c

[Don Dutile]

On 08/15/2019 11:33 AM, Kelsey Skunberg wrote:
> This series is designed to clean up device attributes and permissions in
> pci-sysfs.c. Then move the sysfs SR-IOV functions from pci-sysfs.c to
> iov.c for better organization.
> 
> Patch 1: Define device attributes with DEVICE_ATTR* instead of __ATTR*.
> 
> Patch 2: Change permissions from symbolic to the preferred octal.
> 
> Patch 3: Change DEVICE_ATTR() with 0220 permissions to DEVICE_ATTR_WO().
> 
> Patch 4: Move sysfs SR-IOV functions to iov.c to keep the feature's code
> together.
> 
> 
> Patch 1, 2, and 4 will report unusual permissions '0664' used from the
> following:
> 
>    static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show,
>                       sriov_numvfs_store);
> 
>    static DEVICE_ATTR(sriov_drivers_autoprobe, 0664,
>                       sriov_drivers_autoprobe_show,
>                       sriov_drivers_autoprobe_store);
> 
> This series preserves the existing permissions set in:
> 
> 
>    commit 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control
>                          VF driver binding")
> 
>    commit 1789382a72a5 ("PCI: SRIOV control and status via sysfs")
> 
> Either adding a comment verifying permissions are okay or changing the
> permissions is to be completed with a new patch.
> 
> Changes since v1:
>          Add patch 1 and 2 to fix the way device attributes are defined
>          and change permissions from symbolic to octal. Patch 4 which moves
>          sysfs SR-IOV functions to iov.c will then apply cleaner.
> 
> Changes since v2:
> 
>          Patch 1: Commit log updated. Example shows DEVICE_ATTR_RO()
>          example instead of DEVICE_ATTR(). DEVICE_ATTR() should be avoided
>          unless the files have unusual permissions. Changed to reflect a
>          more encouraged usage.  Also updated regex to be accurate.
> 
>          Patch 3: [NEW] Add patch to change DEVICE_ATTR() with 0220
>          permissions to DEVICE_ATTR_WO().
> 
>          Updated series log to reflect new patch and unusual permissions
>          information.
> 
> 
> Kelsey Skunberg (4):
>    PCI: sysfs: Define device attributes with DEVICE_ATTR*
>    PCI: sysfs: Change permissions from symbolic to octal
>    PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()
>    PCI/IOV: Move sysfs SR-IOV functions to iov.c
> 
>   drivers/pci/iov.c       | 168 ++++++++++++++++++++++++++++++
>   drivers/pci/pci-sysfs.c | 223 ++++------------------------------------
>   drivers/pci/pci.h       |   2 +-
>   3 files changed, 191 insertions(+), 202 deletions(-)
> 
Thanks for the cleanup.

Reviewed-by: Donald Dutile <ddutile@redhat.com>

Re: [Linux-kernel-mentees] [PATCH v3 0/4] PCI: Clean up pci-sysfs.c

[Bjorn Helgaas]

On Thu, Aug 15, 2019 at 09:33:49AM -0600, Kelsey Skunberg wrote:
> This series is designed to clean up device attributes and permissions in
> pci-sysfs.c. Then move the sysfs SR-IOV functions from pci-sysfs.c to
> iov.c for better organization.
> 
> Patch 1: Define device attributes with DEVICE_ATTR* instead of __ATTR*.
> 
> Patch 2: Change permissions from symbolic to the preferred octal.
> 
> Patch 3: Change DEVICE_ATTR() with 0220 permissions to DEVICE_ATTR_WO().
> 
> Patch 4: Move sysfs SR-IOV functions to iov.c to keep the feature's code
> together.
> 
> 
> Patch 1, 2, and 4 will report unusual permissions '0664' used from the
> following:
> 
>   static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show,
>                      sriov_numvfs_store);
> 
>   static DEVICE_ATTR(sriov_drivers_autoprobe, 0664,
>                      sriov_drivers_autoprobe_show,
>                      sriov_drivers_autoprobe_store);
> 
> This series preserves the existing permissions set in:
> 
> 
>   commit 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control
>                         VF driver binding")
> 
>   commit 1789382a72a5 ("PCI: SRIOV control and status via sysfs")
> 
> Either adding a comment verifying permissions are okay or changing the
> permissions is to be completed with a new patch.
> 
> Changes since v1:
>         Add patch 1 and 2 to fix the way device attributes are defined
>         and change permissions from symbolic to octal. Patch 4 which moves
>         sysfs SR-IOV functions to iov.c will then apply cleaner.
> 
> Changes since v2:
> 
>         Patch 1: Commit log updated. Example shows DEVICE_ATTR_RO()
>         example instead of DEVICE_ATTR(). DEVICE_ATTR() should be avoided
>         unless the files have unusual permissions. Changed to reflect a
>         more encouraged usage.  Also updated regex to be accurate.
> 
>         Patch 3: [NEW] Add patch to change DEVICE_ATTR() with 0220
>         permissions to DEVICE_ATTR_WO().
> 
>         Updated series log to reflect new patch and unusual permissions
>         information.
> 
> 
> Kelsey Skunberg (4):
>   PCI: sysfs: Define device attributes with DEVICE_ATTR*
>   PCI: sysfs: Change permissions from symbolic to octal
>   PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()
>   PCI/IOV: Move sysfs SR-IOV functions to iov.c
> 
>  drivers/pci/iov.c       | 168 ++++++++++++++++++++++++++++++
>  drivers/pci/pci-sysfs.c | 223 ++++------------------------------------
>  drivers/pci/pci.h       |   2 +-
>  3 files changed, 191 insertions(+), 202 deletions(-)

Thanks, I applied the new DEVICE_ATTR_WO() patch as the *second* patch
so the two DEVICE_ATTR patches were together.  I added Greg and Don's
Reviewed-by to all and Kuppuswamy's to the last.  This is all on
pci/virtualization for v5.4.

Bjorn

[PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR*

[Kelsey Skunberg]

Defining device attributes should be done through the helper
DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
__ATTR* to now use its equivalent DEVICE_ATTR*.

Example of old:

static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);

Example of new:

static DEVICE_ATTR_RO(_name);

Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
 drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 965c72104150..8af7944fdccb 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static struct device_attribute dev_rescan_attr = __ATTR(rescan,
-							(S_IWUSR|S_IWGRP),
-							NULL, dev_rescan_store);
+static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
 
 static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
@@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
 	return count;
 }
-static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
-							(S_IWUSR|S_IWGRP),
-							NULL, remove_store);
+static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
+				  remove_store);
 
 static ssize_t dev_bus_rescan_store(struct device *dev,
 				    struct device_attribute *attr,
@@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
+static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
 
 #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
 static ssize_t d3cold_allowed_store(struct device *dev,
@@ -687,16 +684,14 @@ static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
 	return count;
 }
 
-static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
-static struct device_attribute sriov_numvfs_attr =
-		__ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
-		       sriov_numvfs_show, sriov_numvfs_store);
-static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
-static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
-static struct device_attribute sriov_vf_device_attr = __ATTR_RO(sriov_vf_device);
-static struct device_attribute sriov_drivers_autoprobe_attr =
-		__ATTR(sriov_drivers_autoprobe, (S_IRUGO|S_IWUSR|S_IWGRP),
-		       sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
+static DEVICE_ATTR_RO(sriov_totalvfs);
+static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
+				  sriov_numvfs_show, sriov_numvfs_store);
+static DEVICE_ATTR_RO(sriov_offset);
+static DEVICE_ATTR_RO(sriov_stride);
+static DEVICE_ATTR_RO(sriov_vf_device);
+static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
+		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
 #endif /* CONFIG_PCI_IOV */
 
 static ssize_t driver_override_store(struct device *dev,
@@ -792,7 +787,7 @@ static struct attribute *pcie_dev_attrs[] = {
 };
 
 static struct attribute *pcibus_attrs[] = {
-	&dev_attr_rescan.attr,
+	&dev_attr_bus_rescan.attr,
 	&dev_attr_cpuaffinity.attr,
 	&dev_attr_cpulistaffinity.attr,
 	NULL,
@@ -820,7 +815,7 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
 		!!(pdev->resource[PCI_ROM_RESOURCE].flags &
 		   IORESOURCE_ROM_SHADOW));
 }
-static struct device_attribute vga_attr = __ATTR_RO(boot_vga);
+static DEVICE_ATTR_RO(boot_vga);
 
 static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
 			       struct bin_attribute *bin_attr, char *buf,
@@ -1458,7 +1453,7 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-static struct device_attribute reset_attr = __ATTR(reset, 0200, NULL, reset_store);
+static DEVICE_ATTR(reset, 0200, NULL, reset_store);
 
 static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 {
@@ -1468,7 +1463,7 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 	pcie_aspm_create_sysfs_dev_files(dev);
 
 	if (dev->reset_fn) {
-		retval = device_create_file(&dev->dev, &reset_attr);
+		retval = device_create_file(&dev->dev, &dev_attr_reset);
 		if (retval)
 			goto error;
 	}
@@ -1553,7 +1548,7 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
 	pcie_vpd_remove_sysfs_dev_files(dev);
 	pcie_aspm_remove_sysfs_dev_files(dev);
 	if (dev->reset_fn) {
-		device_remove_file(&dev->dev, &reset_attr);
+		device_remove_file(&dev->dev, &dev_attr_reset);
 		dev->reset_fn = 0;
 	}
 }
@@ -1606,7 +1601,7 @@ static int __init pci_sysfs_init(void)
 late_initcall(pci_sysfs_init);
 
 static struct attribute *pci_dev_dev_attrs[] = {
-	&vga_attr.attr,
+	&dev_attr_boot_vga.attr,
 	NULL,
 };
 
@@ -1616,7 +1611,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	if (a == &vga_attr.attr)
+	if (a == &dev_attr_boot_vga.attr)
 		if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
 			return 0;
 
@@ -1624,8 +1619,8 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
 }
 
 static struct attribute *pci_dev_hp_attrs[] = {
-	&dev_remove_attr.attr,
-	&dev_rescan_attr.attr,
+	&dev_attr_remove.attr,
+	&dev_attr_rescan.attr,
 	NULL,
 };
 
@@ -1699,12 +1694,12 @@ static const struct attribute_group pci_dev_hp_attr_group = {
 
 #ifdef CONFIG_PCI_IOV
 static struct attribute *sriov_dev_attrs[] = {
-	&sriov_totalvfs_attr.attr,
-	&sriov_numvfs_attr.attr,
-	&sriov_offset_attr.attr,
-	&sriov_stride_attr.attr,
-	&sriov_vf_device_attr.attr,
-	&sriov_drivers_autoprobe_attr.attr,
+	&dev_attr_sriov_totalvfs.attr,
+	&dev_attr_sriov_numvfs.attr,
+	&dev_attr_sriov_offset.attr,
+	&dev_attr_sriov_stride.attr,
+	&dev_attr_sriov_vf_device.attr,
+	&dev_attr_sriov_drivers_autoprobe.attr,
 	NULL,
 };
 
-- 
2.20.1

Re: [PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR*

[Ruslan Bilovol]

On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
<skunberg.kelsey@gmail.com> wrote:
>
> Defining device attributes should be done through the helper
> DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> __ATTR* to now use its equivalent DEVICE_ATTR*.
>
> Example of old:
>
> static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
>
> Example of new:
>
> static DEVICE_ATTR_RO(_name);
>
> Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
> ---
>  drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
>  1 file changed, 27 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 965c72104150..8af7944fdccb 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
>         }
>         return count;
>  }
> -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> -                                                       (S_IWUSR|S_IWGRP),
> -                                                       NULL, dev_rescan_store);
> +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
>
>  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
>                             const char *buf, size_t count)
> @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
>                 pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
>         return count;
>  }
> -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> -                                                       (S_IWUSR|S_IWGRP),
> -                                                       NULL, remove_store);
> +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> +                                 remove_store);
>
>  static ssize_t dev_bus_rescan_store(struct device *dev,
>                                     struct device_attribute *attr,
> @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
>         }
>         return count;
>  }
> -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);

This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
There is also mismatch now between real functionality and documentation
Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
descriptions.

Another patch from this patch series also renamed 'rescan' to 'dev_rescan'

Here is a comparison between two stable kernels (with and without this
patch series):

v5.4
# find /sys -name '*rescan'
/sys/devices/pci0000:00/0000:00:01.2/dev_rescan
/sys/devices/pci0000:00/0000:00:01.0/dev_rescan
/sys/devices/pci0000:00/0000:00:04.0/dev_rescan
/sys/devices/pci0000:00/0000:00:00.0/dev_rescan
/sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
/sys/devices/pci0000:00/0000:00:01.3/dev_rescan
/sys/devices/pci0000:00/0000:00:03.0/dev_rescan
/sys/devices/pci0000:00/0000:00:01.1/dev_rescan
/sys/devices/pci0000:00/0000:00:02.0/dev_rescan
/sys/devices/pci0000:00/0000:00:05.0/dev_rescan
/sys/bus/pci/rescan

v4.19
# find /sys -name '*rescan'
/sys/devices/pci0000:00/0000:00:01.2/rescan
/sys/devices/pci0000:00/0000:00:01.0/rescan
/sys/devices/pci0000:00/0000:00:04.0/rescan
/sys/devices/pci0000:00/0000:00:00.0/rescan
/sys/devices/pci0000:00/pci_bus/0000:00/rescan
/sys/devices/pci0000:00/0000:00:01.3/rescan
/sys/devices/pci0000:00/0000:00:03.0/rescan
/sys/devices/pci0000:00/0000:00:01.1/rescan
/sys/devices/pci0000:00/0000:00:02.0/rescan
/sys/devices/pci0000:00/0000:00:05.0/rescan
/sys/bus/pci/rescan

Do we maintain this kind of API as non-changeable?

Thanks,
Ruslan

>
>  #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
>  static ssize_t d3cold_allowed_store(struct device *dev,
> @@ -687,16 +684,14 @@ static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
>         return count;
>  }
>
> -static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> -static struct device_attribute sriov_numvfs_attr =
> -               __ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
> -                      sriov_numvfs_show, sriov_numvfs_store);
> -static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
> -static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
> -static struct device_attribute sriov_vf_device_attr = __ATTR_RO(sriov_vf_device);
> -static struct device_attribute sriov_drivers_autoprobe_attr =
> -               __ATTR(sriov_drivers_autoprobe, (S_IRUGO|S_IWUSR|S_IWGRP),
> -                      sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> +static DEVICE_ATTR_RO(sriov_totalvfs);
> +static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> +                                 sriov_numvfs_show, sriov_numvfs_store);
> +static DEVICE_ATTR_RO(sriov_offset);
> +static DEVICE_ATTR_RO(sriov_stride);
> +static DEVICE_ATTR_RO(sriov_vf_device);
> +static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> +                  sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
>  #endif /* CONFIG_PCI_IOV */
>
>  static ssize_t driver_override_store(struct device *dev,
> @@ -792,7 +787,7 @@ static struct attribute *pcie_dev_attrs[] = {
>  };
>
>  static struct attribute *pcibus_attrs[] = {
> -       &dev_attr_rescan.attr,
> +       &dev_attr_bus_rescan.attr,
>         &dev_attr_cpuaffinity.attr,
>         &dev_attr_cpulistaffinity.attr,
>         NULL,
> @@ -820,7 +815,7 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
>                 !!(pdev->resource[PCI_ROM_RESOURCE].flags &
>                    IORESOURCE_ROM_SHADOW));
>  }
> -static struct device_attribute vga_attr = __ATTR_RO(boot_vga);
> +static DEVICE_ATTR_RO(boot_vga);
>
>  static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
>                                struct bin_attribute *bin_attr, char *buf,
> @@ -1458,7 +1453,7 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
>         return count;
>  }
>
> -static struct device_attribute reset_attr = __ATTR(reset, 0200, NULL, reset_store);
> +static DEVICE_ATTR(reset, 0200, NULL, reset_store);
>
>  static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>  {
> @@ -1468,7 +1463,7 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>         pcie_aspm_create_sysfs_dev_files(dev);
>
>         if (dev->reset_fn) {
> -               retval = device_create_file(&dev->dev, &reset_attr);
> +               retval = device_create_file(&dev->dev, &dev_attr_reset);
>                 if (retval)
>                         goto error;
>         }
> @@ -1553,7 +1548,7 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
>         pcie_vpd_remove_sysfs_dev_files(dev);
>         pcie_aspm_remove_sysfs_dev_files(dev);
>         if (dev->reset_fn) {
> -               device_remove_file(&dev->dev, &reset_attr);
> +               device_remove_file(&dev->dev, &dev_attr_reset);
>                 dev->reset_fn = 0;
>         }
>  }
> @@ -1606,7 +1601,7 @@ static int __init pci_sysfs_init(void)
>  late_initcall(pci_sysfs_init);
>
>  static struct attribute *pci_dev_dev_attrs[] = {
> -       &vga_attr.attr,
> +       &dev_attr_boot_vga.attr,
>         NULL,
>  };
>
> @@ -1616,7 +1611,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
>         struct device *dev = kobj_to_dev(kobj);
>         struct pci_dev *pdev = to_pci_dev(dev);
>
> -       if (a == &vga_attr.attr)
> +       if (a == &dev_attr_boot_vga.attr)
>                 if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
>                         return 0;
>
> @@ -1624,8 +1619,8 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
>  }
>
>  static struct attribute *pci_dev_hp_attrs[] = {
> -       &dev_remove_attr.attr,
> -       &dev_rescan_attr.attr,
> +       &dev_attr_remove.attr,
> +       &dev_attr_rescan.attr,
>         NULL,
>  };
>
> @@ -1699,12 +1694,12 @@ static const struct attribute_group pci_dev_hp_attr_group = {
>
>  #ifdef CONFIG_PCI_IOV
>  static struct attribute *sriov_dev_attrs[] = {
> -       &sriov_totalvfs_attr.attr,
> -       &sriov_numvfs_attr.attr,
> -       &sriov_offset_attr.attr,
> -       &sriov_stride_attr.attr,
> -       &sriov_vf_device_attr.attr,
> -       &sriov_drivers_autoprobe_attr.attr,
> +       &dev_attr_sriov_totalvfs.attr,
> +       &dev_attr_sriov_numvfs.attr,
> +       &dev_attr_sriov_offset.attr,
> +       &dev_attr_sriov_stride.attr,
> +       &dev_attr_sriov_vf_device.attr,
> +       &dev_attr_sriov_drivers_autoprobe.attr,
>         NULL,
>  };
>
> --
> 2.20.1
>

Re: [PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR*

[Greg Kroah-Hartman]

On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> <skunberg.kelsey@gmail.com> wrote:
> >
> > Defining device attributes should be done through the helper
> > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > __ATTR* to now use its equivalent DEVICE_ATTR*.
> >
> > Example of old:
> >
> > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> >
> > Example of new:
> >
> > static DEVICE_ATTR_RO(_name);
> >
> > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
> > ---
> >  drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> >  1 file changed, 27 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 965c72104150..8af7944fdccb 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> >         }
> >         return count;
> >  }
> > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > -                                                       (S_IWUSR|S_IWGRP),
> > -                                                       NULL, dev_rescan_store);
> > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> >
> >  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> >                             const char *buf, size_t count)
> > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> >                 pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> >         return count;
> >  }
> > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > -                                                       (S_IWUSR|S_IWGRP),
> > -                                                       NULL, remove_store);
> > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > +                                 remove_store);
> >
> >  static ssize_t dev_bus_rescan_store(struct device *dev,
> >                                     struct device_attribute *attr,
> > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> >         }
> >         return count;
> >  }
> > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> 
> This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> There is also mismatch now between real functionality and documentation
> Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> descriptions.
> 
> Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
> 
> Here is a comparison between two stable kernels (with and without this
> patch series):
> 
> v5.4
> # find /sys -name '*rescan'
> /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> /sys/bus/pci/rescan
> 
> v4.19
> # find /sys -name '*rescan'
> /sys/devices/pci0000:00/0000:00:01.2/rescan
> /sys/devices/pci0000:00/0000:00:01.0/rescan
> /sys/devices/pci0000:00/0000:00:04.0/rescan
> /sys/devices/pci0000:00/0000:00:00.0/rescan
> /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> /sys/devices/pci0000:00/0000:00:01.3/rescan
> /sys/devices/pci0000:00/0000:00:03.0/rescan
> /sys/devices/pci0000:00/0000:00:01.1/rescan
> /sys/devices/pci0000:00/0000:00:02.0/rescan
> /sys/devices/pci0000:00/0000:00:05.0/rescan
> /sys/bus/pci/rescan
> 
> Do we maintain this kind of API as non-changeable?

Yeah, that's a bug and should be fixed, sorry for missing that on
review.

Kelsey, can you fix this up?

thanks,

greg k-h

Re: [PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR*

[Kelsey]

On Sat, Mar 14, 2020 at 5:20 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> > On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> > <skunberg.kelsey@gmail.com> wrote:
> > >
> > > Defining device attributes should be done through the helper
> > > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > > __ATTR* to now use its equivalent DEVICE_ATTR*.
> > >
> > > Example of old:
> > >
> > > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> > >
> > > Example of new:
> > >
> > > static DEVICE_ATTR_RO(_name);
> > >
> > > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
> > > ---
> > >  drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> > >  1 file changed, 27 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > index 965c72104150..8af7944fdccb 100644
> > > --- a/drivers/pci/pci-sysfs.c
> > > +++ b/drivers/pci/pci-sysfs.c
> > > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> > >         }
> > >         return count;
> > >  }
> > > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > > -                                                       (S_IWUSR|S_IWGRP),
> > > -                                                       NULL, dev_rescan_store);
> > > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> > >
> > >  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > >                             const char *buf, size_t count)
> > > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > >                 pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > >         return count;
> > >  }
> > > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > > -                                                       (S_IWUSR|S_IWGRP),
> > > -                                                       NULL, remove_store);
> > > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > > +                                 remove_store);
> > >
> > >  static ssize_t dev_bus_rescan_store(struct device *dev,
> > >                                     struct device_attribute *attr,
> > > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> > >         }
> > >         return count;
> > >  }
> > > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> >
> > This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> > There is also mismatch now between real functionality and documentation
> > Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> > descriptions.
> >
> > Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
> >
> > Here is a comparison between two stable kernels (with and without this
> > patch series):
> >
> > v5.4
> > # find /sys -name '*rescan'
> > /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> > /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> > /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> > /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> > /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> > /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> > /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> > /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> > /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> > /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> > /sys/bus/pci/rescan
> >
> > v4.19
> > # find /sys -name '*rescan'
> > /sys/devices/pci0000:00/0000:00:01.2/rescan
> > /sys/devices/pci0000:00/0000:00:01.0/rescan
> > /sys/devices/pci0000:00/0000:00:04.0/rescan
> > /sys/devices/pci0000:00/0000:00:00.0/rescan
> > /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> > /sys/devices/pci0000:00/0000:00:01.3/rescan
> > /sys/devices/pci0000:00/0000:00:03.0/rescan
> > /sys/devices/pci0000:00/0000:00:01.1/rescan
> > /sys/devices/pci0000:00/0000:00:02.0/rescan
> > /sys/devices/pci0000:00/0000:00:05.0/rescan
> > /sys/bus/pci/rescan
> >
> > Do we maintain this kind of API as non-changeable?
>
> Yeah, that's a bug and should be fixed, sorry for missing that on
> review.
>
> Kelsey, can you fix this up?
>
> thanks,
>
> greg k-h

I'd be happy to help get this fixed up.

Would it be proper to go back to using DEVICE_ATTR() for 'bus_rescan'
and 'dev_rescan' in order to change their names back to 'rescan'?

The name changes were done so the correct *_store() would still be
called. When using DEVICE_ATTR() the *_store() name is passed as the
last argument, as seen here:

    static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);

When using the helper, only the name is passed and it assumes default
<name>_show(), as seen here:

    static DEVICE_ATTR_WO(dev_rescan);   # (This would assume
dev_rescan_store())

This can be verified in Documentation/filesystems/sysfs.txt.

There is already a rescan attribute using rescan_store(), so changing
at least one of these to DEVICE_ATTR_WO(rescan) would be conflicting.

I understand it's ideal to stay away from using DEVICE_ATTR() unless
an unusual mode is needed. Would having a different name vs
name_store() be another reason to justify using DEVICE_ATTR()?

Thank you Ruslan for pointing this out!

- Kelsey

Re: [PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR*

[Greg Kroah-Hartman]

On Tue, Mar 24, 2020 at 12:10:33AM -0600, Kelsey wrote:
> On Sat, Mar 14, 2020 at 5:20 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> > > On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> > > <skunberg.kelsey@gmail.com> wrote:
> > > >
> > > > Defining device attributes should be done through the helper
> > > > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > > > __ATTR* to now use its equivalent DEVICE_ATTR*.
> > > >
> > > > Example of old:
> > > >
> > > > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> > > >
> > > > Example of new:
> > > >
> > > > static DEVICE_ATTR_RO(_name);
> > > >
> > > > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
> > > > ---
> > > >  drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> > > >  1 file changed, 27 insertions(+), 32 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > index 965c72104150..8af7944fdccb 100644
> > > > --- a/drivers/pci/pci-sysfs.c
> > > > +++ b/drivers/pci/pci-sysfs.c
> > > > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> > > >         }
> > > >         return count;
> > > >  }
> > > > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > > > -                                                       (S_IWUSR|S_IWGRP),
> > > > -                                                       NULL, dev_rescan_store);
> > > > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> > > >
> > > >  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > >                             const char *buf, size_t count)
> > > > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > >                 pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > > >         return count;
> > > >  }
> > > > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > > > -                                                       (S_IWUSR|S_IWGRP),
> > > > -                                                       NULL, remove_store);
> > > > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > > > +                                 remove_store);
> > > >
> > > >  static ssize_t dev_bus_rescan_store(struct device *dev,
> > > >                                     struct device_attribute *attr,
> > > > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> > > >         }
> > > >         return count;
> > > >  }
> > > > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > > > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> > >
> > > This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> > > There is also mismatch now between real functionality and documentation
> > > Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> > > descriptions.
> > >
> > > Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
> > >
> > > Here is a comparison between two stable kernels (with and without this
> > > patch series):
> > >
> > > v5.4
> > > # find /sys -name '*rescan'
> > > /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> > > /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> > > /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> > > /sys/bus/pci/rescan
> > >
> > > v4.19
> > > # find /sys -name '*rescan'
> > > /sys/devices/pci0000:00/0000:00:01.2/rescan
> > > /sys/devices/pci0000:00/0000:00:01.0/rescan
> > > /sys/devices/pci0000:00/0000:00:04.0/rescan
> > > /sys/devices/pci0000:00/0000:00:00.0/rescan
> > > /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> > > /sys/devices/pci0000:00/0000:00:01.3/rescan
> > > /sys/devices/pci0000:00/0000:00:03.0/rescan
> > > /sys/devices/pci0000:00/0000:00:01.1/rescan
> > > /sys/devices/pci0000:00/0000:00:02.0/rescan
> > > /sys/devices/pci0000:00/0000:00:05.0/rescan
> > > /sys/bus/pci/rescan
> > >
> > > Do we maintain this kind of API as non-changeable?
> >
> > Yeah, that's a bug and should be fixed, sorry for missing that on
> > review.
> >
> > Kelsey, can you fix this up?
> >
> > thanks,
> >
> > greg k-h
> 
> I'd be happy to help get this fixed up.
> 
> Would it be proper to go back to using DEVICE_ATTR() for 'bus_rescan'
> and 'dev_rescan' in order to change their names back to 'rescan'?

Yes.

thanks,

greg k-h

Re: [PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR*

[Kelsey]

On Tue, Mar 24, 2020 at 12:24 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 24, 2020 at 12:10:33AM -0600, Kelsey wrote:
> > On Sat, Mar 14, 2020 at 5:20 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> > > > On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> > > > <skunberg.kelsey@gmail.com> wrote:
> > > > >
> > > > > Defining device attributes should be done through the helper
> > > > > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > > > > __ATTR* to now use its equivalent DEVICE_ATTR*.
> > > > >
> > > > > Example of old:
> > > > >
> > > > > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> > > > >
> > > > > Example of new:
> > > > >
> > > > > static DEVICE_ATTR_RO(_name);
> > > > >
> > > > > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
> > > > > ---
> > > > >  drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> > > > >  1 file changed, 27 insertions(+), 32 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > > index 965c72104150..8af7944fdccb 100644
> > > > > --- a/drivers/pci/pci-sysfs.c
> > > > > +++ b/drivers/pci/pci-sysfs.c
> > > > > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> > > > >         }
> > > > >         return count;
> > > > >  }
> > > > > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > > > > -                                                       (S_IWUSR|S_IWGRP),
> > > > > -                                                       NULL, dev_rescan_store);
> > > > > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> > > > >
> > > > >  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > >                             const char *buf, size_t count)
> > > > > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > >                 pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > > > >         return count;
> > > > >  }
> > > > > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > > > > -                                                       (S_IWUSR|S_IWGRP),
> > > > > -                                                       NULL, remove_store);
> > > > > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > > > > +                                 remove_store);
> > > > >
> > > > >  static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > >                                     struct device_attribute *attr,
> > > > > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > >         }
> > > > >         return count;
> > > > >  }
> > > > > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > > > > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> > > >
> > > > This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> > > > There is also mismatch now between real functionality and documentation
> > > > Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> > > > descriptions.
> > > >
> > > > Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
> > > >
> > > > Here is a comparison between two stable kernels (with and without this
> > > > patch series):
> > > >
> > > > v5.4
> > > > # find /sys -name '*rescan'
> > > > /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> > > > /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> > > > /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> > > > /sys/bus/pci/rescan
> > > >
> > > > v4.19
> > > > # find /sys -name '*rescan'
> > > > /sys/devices/pci0000:00/0000:00:01.2/rescan
> > > > /sys/devices/pci0000:00/0000:00:01.0/rescan
> > > > /sys/devices/pci0000:00/0000:00:04.0/rescan
> > > > /sys/devices/pci0000:00/0000:00:00.0/rescan
> > > > /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> > > > /sys/devices/pci0000:00/0000:00:01.3/rescan
> > > > /sys/devices/pci0000:00/0000:00:03.0/rescan
> > > > /sys/devices/pci0000:00/0000:00:01.1/rescan
> > > > /sys/devices/pci0000:00/0000:00:02.0/rescan
> > > > /sys/devices/pci0000:00/0000:00:05.0/rescan
> > > > /sys/bus/pci/rescan
> > > >
> > > > Do we maintain this kind of API as non-changeable?
> > >
> > > Yeah, that's a bug and should be fixed, sorry for missing that on
> > > review.
> > >
> > > Kelsey, can you fix this up?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > I'd be happy to help get this fixed up.
> >
> > Would it be proper to go back to using DEVICE_ATTR() for 'bus_rescan'
> > and 'dev_rescan' in order to change their names back to 'rescan'?
>
> Yes.
>
> thanks,
>
> greg k-h


Ack. Sent a patch out. Will stay posted in case any updates need to be made.

commit 4cb9e42d3226 ("PCI: sysfs: Change bus_rescan and dev_rescan to rescan")

Thanks!

- Kelsey

Re: [PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR*

[Greg Kroah-Hartman]

On Tue, Mar 24, 2020 at 05:53:59PM -0600, Kelsey wrote:
> On Tue, Mar 24, 2020 at 12:24 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Mar 24, 2020 at 12:10:33AM -0600, Kelsey wrote:
> > > On Sat, Mar 14, 2020 at 5:20 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> > > > > On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> > > > > <skunberg.kelsey@gmail.com> wrote:
> > > > > >
> > > > > > Defining device attributes should be done through the helper
> > > > > > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > > > > > __ATTR* to now use its equivalent DEVICE_ATTR*.
> > > > > >
> > > > > > Example of old:
> > > > > >
> > > > > > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> > > > > >
> > > > > > Example of new:
> > > > > >
> > > > > > static DEVICE_ATTR_RO(_name);
> > > > > >
> > > > > > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
> > > > > > ---
> > > > > >  drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> > > > > >  1 file changed, 27 insertions(+), 32 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > > > index 965c72104150..8af7944fdccb 100644
> > > > > > --- a/drivers/pci/pci-sysfs.c
> > > > > > +++ b/drivers/pci/pci-sysfs.c
> > > > > > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> > > > > >         }
> > > > > >         return count;
> > > > > >  }
> > > > > > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > > > > > -                                                       (S_IWUSR|S_IWGRP),
> > > > > > -                                                       NULL, dev_rescan_store);
> > > > > > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> > > > > >
> > > > > >  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > > >                             const char *buf, size_t count)
> > > > > > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > > >                 pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > > > > >         return count;
> > > > > >  }
> > > > > > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > > > > > -                                                       (S_IWUSR|S_IWGRP),
> > > > > > -                                                       NULL, remove_store);
> > > > > > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > > > > > +                                 remove_store);
> > > > > >
> > > > > >  static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > > >                                     struct device_attribute *attr,
> > > > > > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > > >         }
> > > > > >         return count;
> > > > > >  }
> > > > > > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > > > > > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> > > > >
> > > > > This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> > > > > There is also mismatch now between real functionality and documentation
> > > > > Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> > > > > descriptions.
> > > > >
> > > > > Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
> > > > >
> > > > > Here is a comparison between two stable kernels (with and without this
> > > > > patch series):
> > > > >
> > > > > v5.4
> > > > > # find /sys -name '*rescan'
> > > > > /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> > > > > /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> > > > > /sys/bus/pci/rescan
> > > > >
> > > > > v4.19
> > > > > # find /sys -name '*rescan'
> > > > > /sys/devices/pci0000:00/0000:00:01.2/rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.0/rescan
> > > > > /sys/devices/pci0000:00/0000:00:04.0/rescan
> > > > > /sys/devices/pci0000:00/0000:00:00.0/rescan
> > > > > /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.3/rescan
> > > > > /sys/devices/pci0000:00/0000:00:03.0/rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.1/rescan
> > > > > /sys/devices/pci0000:00/0000:00:02.0/rescan
> > > > > /sys/devices/pci0000:00/0000:00:05.0/rescan
> > > > > /sys/bus/pci/rescan
> > > > >
> > > > > Do we maintain this kind of API as non-changeable?
> > > >
> > > > Yeah, that's a bug and should be fixed, sorry for missing that on
> > > > review.
> > > >
> > > > Kelsey, can you fix this up?
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > I'd be happy to help get this fixed up.
> > >
> > > Would it be proper to go back to using DEVICE_ATTR() for 'bus_rescan'
> > > and 'dev_rescan' in order to change their names back to 'rescan'?
> >
> > Yes.
> >
> > thanks,
> >
> > greg k-h
> 
> 
> Ack. Sent a patch out. Will stay posted in case any updates need to be made.
> 
> commit 4cb9e42d3226 ("PCI: sysfs: Change bus_rescan and dev_rescan to rescan")

That's your local commit, not the commit in Linus's tree :)

greg k-h

Re: [PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR*

[Kelsey]

On Wed, Mar 25, 2020 at 1:17 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Mar 24, 2020 at 05:53:59PM -0600, Kelsey wrote:
> > On Tue, Mar 24, 2020 at 12:24 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Mar 24, 2020 at 12:10:33AM -0600, Kelsey wrote:
> > > > On Sat, Mar 14, 2020 at 5:20 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> > > > > > On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> > > > > > <skunberg.kelsey@gmail.com> wrote:
> > > > > > >
> > > > > > > Defining device attributes should be done through the helper
> > > > > > > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > > > > > > __ATTR* to now use its equivalent DEVICE_ATTR*.
> > > > > > >
> > > > > > > Example of old:
> > > > > > >
> > > > > > > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> > > > > > >
> > > > > > > Example of new:
> > > > > > >
> > > > > > > static DEVICE_ATTR_RO(_name);
> > > > > > >
> > > > > > > Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
> > > > > > > ---
> > > > > > >  drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> > > > > > >  1 file changed, 27 insertions(+), 32 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > > > > index 965c72104150..8af7944fdccb 100644
> > > > > > > --- a/drivers/pci/pci-sysfs.c
> > > > > > > +++ b/drivers/pci/pci-sysfs.c
> > > > > > > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> > > > > > >         }
> > > > > > >         return count;
> > > > > > >  }
> > > > > > > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > > > > > > -                                                       (S_IWUSR|S_IWGRP),
> > > > > > > -                                                       NULL, dev_rescan_store);
> > > > > > > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> > > > > > >
> > > > > > >  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > > > >                             const char *buf, size_t count)
> > > > > > > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > > > >                 pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > > > > > >         return count;
> > > > > > >  }
> > > > > > > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > > > > > > -                                                       (S_IWUSR|S_IWGRP),
> > > > > > > -                                                       NULL, remove_store);
> > > > > > > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > > > > > > +                                 remove_store);
> > > > > > >
> > > > > > >  static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > > > >                                     struct device_attribute *attr,
> > > > > > > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > > > >         }
> > > > > > >         return count;
> > > > > > >  }
> > > > > > > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > > > > > > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> > > > > >
> > > > > > This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> > > > > > There is also mismatch now between real functionality and documentation
> > > > > > Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> > > > > > descriptions.
> > > > > >
> > > > > > Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
> > > > > >
> > > > > > Here is a comparison between two stable kernels (with and without this
> > > > > > patch series):
> > > > > >
> > > > > > v5.4
> > > > > > # find /sys -name '*rescan'
> > > > > > /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> > > > > > /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> > > > > > /sys/bus/pci/rescan
> > > > > >
> > > > > > v4.19
> > > > > > # find /sys -name '*rescan'
> > > > > > /sys/devices/pci0000:00/0000:00:01.2/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.0/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:04.0/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:00.0/rescan
> > > > > > /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.3/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:03.0/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.1/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:02.0/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:05.0/rescan
> > > > > > /sys/bus/pci/rescan
> > > > > >
> > > > > > Do we maintain this kind of API as non-changeable?
> > > > >
> > > > > Yeah, that's a bug and should be fixed, sorry for missing that on
> > > > > review.
> > > > >
> > > > > Kelsey, can you fix this up?
> > > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h
> > > >
> > > > I'd be happy to help get this fixed up.
> > > >
> > > > Would it be proper to go back to using DEVICE_ATTR() for 'bus_rescan'
> > > > and 'dev_rescan' in order to change their names back to 'rescan'?
> > >
> > > Yes.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> >
> > Ack. Sent a patch out. Will stay posted in case any updates need to be made.
> >
> > commit 4cb9e42d3226 ("PCI: sysfs: Change bus_rescan and dev_rescan to rescan")
>
> That's your local commit, not the commit in Linus's tree :)
>
> greg k-h

hah, whoops! Wanted to reference the patch name and didn't think that
through. Maybe would have been better to reference a link to the patch
anyways. :)

https://lore.kernel.org/r/20200324234848.8299-1-skunberg.kelsey@gmail.com

- Kelsey

[PATCH v3 2/4] PCI: sysfs: Change permissions from symbolic to octal

[Kelsey Skunberg]

Symbolic permissions such as "(S_IWUSR | S_IWGRP)" are not
preferred and octal permissions should be used instead. Change all
symbolic permissions to octal permissions.

Example of old:

"(S_IWUSR | S_IWGRP)"

Example of new:

"0220"

Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
 drivers/pci/pci-sysfs.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 8af7944fdccb..346193ca4826 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -464,7 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
+static DEVICE_ATTR(rescan, 0220, NULL, dev_rescan_store);
 
 static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
@@ -478,7 +478,7 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 		pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
 	return count;
 }
-static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
+static DEVICE_ATTR_IGNORE_LOCKDEP(remove, 0220, NULL,
 				  remove_store);
 
 static ssize_t dev_bus_rescan_store(struct device *dev,
@@ -501,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
+static DEVICE_ATTR(bus_rescan, 0220, NULL, dev_bus_rescan_store);
 
 #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
 static ssize_t d3cold_allowed_store(struct device *dev,
@@ -685,13 +685,12 @@ static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
 }
 
 static DEVICE_ATTR_RO(sriov_totalvfs);
-static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
-				  sriov_numvfs_show, sriov_numvfs_store);
+static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
 static DEVICE_ATTR_RO(sriov_offset);
 static DEVICE_ATTR_RO(sriov_stride);
 static DEVICE_ATTR_RO(sriov_vf_device);
-static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
-		   sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
+static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
+		   sriov_drivers_autoprobe_store);
 #endif /* CONFIG_PCI_IOV */
 
 static ssize_t driver_override_store(struct device *dev,
@@ -1080,7 +1079,7 @@ void pci_create_legacy_files(struct pci_bus *b)
 	sysfs_bin_attr_init(b->legacy_io);
 	b->legacy_io->attr.name = "legacy_io";
 	b->legacy_io->size = 0xffff;
-	b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
+	b->legacy_io->attr.mode = 0600;
 	b->legacy_io->read = pci_read_legacy_io;
 	b->legacy_io->write = pci_write_legacy_io;
 	b->legacy_io->mmap = pci_mmap_legacy_io;
@@ -1094,7 +1093,7 @@ void pci_create_legacy_files(struct pci_bus *b)
 	sysfs_bin_attr_init(b->legacy_mem);
 	b->legacy_mem->attr.name = "legacy_mem";
 	b->legacy_mem->size = 1024*1024;
-	b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
+	b->legacy_mem->attr.mode = 0600;
 	b->legacy_mem->mmap = pci_mmap_legacy_mem;
 	pci_adjust_legacy_attr(b, pci_mmap_mem);
 	error = device_create_bin_file(&b->dev, b->legacy_mem);
@@ -1301,7 +1300,7 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
 		}
 	}
 	res_attr->attr.name = res_attr_name;
-	res_attr->attr.mode = S_IRUSR | S_IWUSR;
+	res_attr->attr.mode = 0600;
 	res_attr->size = pci_resource_len(pdev, num);
 	res_attr->private = (void *)(unsigned long)num;
 	retval = sysfs_create_bin_file(&pdev->dev.kobj, res_attr);
@@ -1414,7 +1413,7 @@ static ssize_t pci_read_rom(struct file *filp, struct kobject *kobj,
 static const struct bin_attribute pci_config_attr = {
 	.attr =	{
 		.name = "config",
-		.mode = S_IRUGO | S_IWUSR,
+		.mode = 0644,
 	},
 	.size = PCI_CFG_SPACE_SIZE,
 	.read = pci_read_config,
@@ -1424,7 +1423,7 @@ static const struct bin_attribute pci_config_attr = {
 static const struct bin_attribute pcie_config_attr = {
 	.attr =	{
 		.name = "config",
-		.mode = S_IRUGO | S_IWUSR,
+		.mode = 0644,
 	},
 	.size = PCI_CFG_SPACE_EXP_SIZE,
 	.read = pci_read_config,
@@ -1506,7 +1505,7 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
 		sysfs_bin_attr_init(attr);
 		attr->size = rom_size;
 		attr->attr.name = "rom";
-		attr->attr.mode = S_IRUSR | S_IWUSR;
+		attr->attr.mode = 0600;
 		attr->read = pci_read_rom;
 		attr->write = pci_write_rom;
 		retval = sysfs_create_bin_file(&pdev->dev.kobj, attr);
-- 
2.20.1

[PATCH v3 3/4] PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()

[Kelsey Skunberg]

DEVICE_ATTR() should only be used when files have unusual permissions.
Change DEVICE_ATTR() with '0220' permissions to DEVICE_ATTR_WO().

Example of old:

static DEVICE_ATTR(_name, 0220, NULL, _store);

Example of new:

static DEVICE_ATTR_WO(_name);

Since _store is no longer passed, make the _name passed by
DEVICE_ATTR_WO() and the related _name##_store() name match with each
other.

Example:

DEVICE_ATTR_WO(bus_rescan) must be able to call bus_rescan_store()

Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
 drivers/pci/pci-sysfs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 346193ca4826..5bb301efec98 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -464,7 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR(rescan, 0220, NULL, dev_rescan_store);
+static DEVICE_ATTR_WO(dev_rescan);
 
 static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
@@ -481,9 +481,9 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR_IGNORE_LOCKDEP(remove, 0220, NULL,
 				  remove_store);
 
-static ssize_t dev_bus_rescan_store(struct device *dev,
-				    struct device_attribute *attr,
-				    const char *buf, size_t count)
+static ssize_t bus_rescan_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
 {
 	unsigned long val;
 	struct pci_bus *bus = to_pci_bus(dev);
@@ -501,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR(bus_rescan, 0220, NULL, dev_bus_rescan_store);
+static DEVICE_ATTR_WO(bus_rescan);
 
 #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
 static ssize_t d3cold_allowed_store(struct device *dev,
@@ -1619,7 +1619,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
 
 static struct attribute *pci_dev_hp_attrs[] = {
 	&dev_attr_remove.attr,
-	&dev_attr_rescan.attr,
+	&dev_attr_dev_rescan.attr,
 	NULL,
 };
 
-- 
2.20.1

[PATCH v3 4/4] PCI/IOV: Move sysfs SR-IOV functions to iov.c

[Kelsey Skunberg]

The sysfs SR-IOV functions are for an optional feature and will be better
organized to keep with the feature's code. Move the sysfs SR-IOV functions
to /pci/iov.c.

Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
 drivers/pci/iov.c       | 168 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci-sysfs.c | 173 ----------------------------------------
 drivers/pci/pci.h       |   2 +-
 3 files changed, 169 insertions(+), 174 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 9b48818ced01..b335db21c85e 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -240,6 +240,174 @@ void pci_iov_remove_virtfn(struct pci_dev *dev, int id)
 	pci_dev_put(dev);
 }
 
+static ssize_t sriov_totalvfs_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
+}
+
+static ssize_t sriov_numvfs_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->num_VFs);
+}
+
+/*
+ * num_vfs > 0; number of VFs to enable
+ * num_vfs = 0; disable all VFs
+ *
+ * Note: SRIOV spec does not allow partial VF
+ *	 disable, so it's all or none.
+ */
+static ssize_t sriov_numvfs_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	int ret;
+	u16 num_vfs;
+
+	ret = kstrtou16(buf, 0, &num_vfs);
+	if (ret < 0)
+		return ret;
+
+	if (num_vfs > pci_sriov_get_totalvfs(pdev))
+		return -ERANGE;
+
+	device_lock(&pdev->dev);
+
+	if (num_vfs == pdev->sriov->num_VFs)
+		goto exit;
+
+	/* is PF driver loaded w/callback */
+	if (!pdev->driver || !pdev->driver->sriov_configure) {
+		pci_info(pdev, "Driver does not support SRIOV configuration via sysfs\n");
+		ret = -ENOENT;
+		goto exit;
+	}
+
+	if (num_vfs == 0) {
+		/* disable VFs */
+		ret = pdev->driver->sriov_configure(pdev, 0);
+		goto exit;
+	}
+
+	/* enable VFs */
+	if (pdev->sriov->num_VFs) {
+		pci_warn(pdev, "%d VFs already enabled. Disable before enabling %d VFs\n",
+			 pdev->sriov->num_VFs, num_vfs);
+		ret = -EBUSY;
+		goto exit;
+	}
+
+	ret = pdev->driver->sriov_configure(pdev, num_vfs);
+	if (ret < 0)
+		goto exit;
+
+	if (ret != num_vfs)
+		pci_warn(pdev, "%d VFs requested; only %d enabled\n",
+			 num_vfs, ret);
+
+exit:
+	device_unlock(&pdev->dev);
+
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static ssize_t sriov_offset_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->offset);
+}
+
+static ssize_t sriov_stride_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->stride);
+}
+
+static ssize_t sriov_vf_device_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%x\n", pdev->sriov->vf_device);
+}
+
+static ssize_t sriov_drivers_autoprobe_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%u\n", pdev->sriov->drivers_autoprobe);
+}
+
+static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	bool drivers_autoprobe;
+
+	if (kstrtobool(buf, &drivers_autoprobe) < 0)
+		return -EINVAL;
+
+	pdev->sriov->drivers_autoprobe = drivers_autoprobe;
+
+	return count;
+}
+
+static DEVICE_ATTR_RO(sriov_totalvfs);
+static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
+static DEVICE_ATTR_RO(sriov_offset);
+static DEVICE_ATTR_RO(sriov_stride);
+static DEVICE_ATTR_RO(sriov_vf_device);
+static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
+		   sriov_drivers_autoprobe_store);
+
+static struct attribute *sriov_dev_attrs[] = {
+	&dev_attr_sriov_totalvfs.attr,
+	&dev_attr_sriov_numvfs.attr,
+	&dev_attr_sriov_offset.attr,
+	&dev_attr_sriov_stride.attr,
+	&dev_attr_sriov_vf_device.attr,
+	&dev_attr_sriov_drivers_autoprobe.attr,
+	NULL,
+};
+
+static umode_t sriov_attrs_are_visible(struct kobject *kobj,
+				       struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+
+	if (!dev_is_pf(dev))
+		return 0;
+
+	return a->mode;
+}
+
+const struct attribute_group sriov_dev_attr_group = {
+	.attrs = sriov_dev_attrs,
+	.is_visible = sriov_attrs_are_visible,
+};
+
 int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
 {
 	return 0;
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 5bb301efec98..868e35109284 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -548,151 +548,6 @@ static ssize_t devspec_show(struct device *dev,
 static DEVICE_ATTR_RO(devspec);
 #endif
 
-#ifdef CONFIG_PCI_IOV
-static ssize_t sriov_totalvfs_show(struct device *dev,
-				   struct device_attribute *attr,
-				   char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
-}
-
-
-static ssize_t sriov_numvfs_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->num_VFs);
-}
-
-/*
- * num_vfs > 0; number of VFs to enable
- * num_vfs = 0; disable all VFs
- *
- * Note: SRIOV spec doesn't allow partial VF
- *       disable, so it's all or none.
- */
-static ssize_t sriov_numvfs_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t count)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	int ret;
-	u16 num_vfs;
-
-	ret = kstrtou16(buf, 0, &num_vfs);
-	if (ret < 0)
-		return ret;
-
-	if (num_vfs > pci_sriov_get_totalvfs(pdev))
-		return -ERANGE;
-
-	device_lock(&pdev->dev);
-
-	if (num_vfs == pdev->sriov->num_VFs)
-		goto exit;
-
-	/* is PF driver loaded w/callback */
-	if (!pdev->driver || !pdev->driver->sriov_configure) {
-		pci_info(pdev, "Driver doesn't support SRIOV configuration via sysfs\n");
-		ret = -ENOENT;
-		goto exit;
-	}
-
-	if (num_vfs == 0) {
-		/* disable VFs */
-		ret = pdev->driver->sriov_configure(pdev, 0);
-		goto exit;
-	}
-
-	/* enable VFs */
-	if (pdev->sriov->num_VFs) {
-		pci_warn(pdev, "%d VFs already enabled. Disable before enabling %d VFs\n",
-			 pdev->sriov->num_VFs, num_vfs);
-		ret = -EBUSY;
-		goto exit;
-	}
-
-	ret = pdev->driver->sriov_configure(pdev, num_vfs);
-	if (ret < 0)
-		goto exit;
-
-	if (ret != num_vfs)
-		pci_warn(pdev, "%d VFs requested; only %d enabled\n",
-			 num_vfs, ret);
-
-exit:
-	device_unlock(&pdev->dev);
-
-	if (ret < 0)
-		return ret;
-
-	return count;
-}
-
-static ssize_t sriov_offset_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->offset);
-}
-
-static ssize_t sriov_stride_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->stride);
-}
-
-static ssize_t sriov_vf_device_show(struct device *dev,
-				    struct device_attribute *attr,
-				    char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%x\n", pdev->sriov->vf_device);
-}
-
-static ssize_t sriov_drivers_autoprobe_show(struct device *dev,
-					    struct device_attribute *attr,
-					    char *buf)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-
-	return sprintf(buf, "%u\n", pdev->sriov->drivers_autoprobe);
-}
-
-static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
-					     struct device_attribute *attr,
-					     const char *buf, size_t count)
-{
-	struct pci_dev *pdev = to_pci_dev(dev);
-	bool drivers_autoprobe;
-
-	if (kstrtobool(buf, &drivers_autoprobe) < 0)
-		return -EINVAL;
-
-	pdev->sriov->drivers_autoprobe = drivers_autoprobe;
-
-	return count;
-}
-
-static DEVICE_ATTR_RO(sriov_totalvfs);
-static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
-static DEVICE_ATTR_RO(sriov_offset);
-static DEVICE_ATTR_RO(sriov_stride);
-static DEVICE_ATTR_RO(sriov_vf_device);
-static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
-		   sriov_drivers_autoprobe_store);
-#endif /* CONFIG_PCI_IOV */
-
 static ssize_t driver_override_store(struct device *dev,
 				     struct device_attribute *attr,
 				     const char *buf, size_t count)
@@ -1691,34 +1546,6 @@ static const struct attribute_group pci_dev_hp_attr_group = {
 	.is_visible = pci_dev_hp_attrs_are_visible,
 };
 
-#ifdef CONFIG_PCI_IOV
-static struct attribute *sriov_dev_attrs[] = {
-	&dev_attr_sriov_totalvfs.attr,
-	&dev_attr_sriov_numvfs.attr,
-	&dev_attr_sriov_offset.attr,
-	&dev_attr_sriov_stride.attr,
-	&dev_attr_sriov_vf_device.attr,
-	&dev_attr_sriov_drivers_autoprobe.attr,
-	NULL,
-};
-
-static umode_t sriov_attrs_are_visible(struct kobject *kobj,
-				       struct attribute *a, int n)
-{
-	struct device *dev = kobj_to_dev(kobj);
-
-	if (!dev_is_pf(dev))
-		return 0;
-
-	return a->mode;
-}
-
-static const struct attribute_group sriov_dev_attr_group = {
-	.attrs = sriov_dev_attrs,
-	.is_visible = sriov_attrs_are_visible,
-};
-#endif /* CONFIG_PCI_IOV */
-
 static const struct attribute_group pci_dev_attr_group = {
 	.attrs = pci_dev_dev_attrs,
 	.is_visible = pci_dev_attrs_are_visible,
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 61bbfd611140..7e3c6c8ae6f9 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -455,7 +455,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno);
 resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
 void pci_restore_iov_state(struct pci_dev *dev);
 int pci_iov_bus_range(struct pci_bus *bus);
-
+extern const struct attribute_group sriov_dev_attr_group;
 #else
 static inline int pci_iov_init(struct pci_dev *dev)
 {
-- 
2.20.1

Re: [PATCH v3 4/4] PCI/IOV: Move sysfs SR-IOV functions to iov.c

[sathyanarayanan kuppuswamy]

On 8/15/19 8:33 AM, Kelsey Skunberg wrote:
> The sysfs SR-IOV functions are for an optional feature and will be better
> organized to keep with the feature's code. Move the sysfs SR-IOV functions
> to /pci/iov.c.
>
> Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan 
<sathyanarayanan.kuppuswamy@linux.intel.com>

> ---
>   drivers/pci/iov.c       | 168 ++++++++++++++++++++++++++++++++++++++
>   drivers/pci/pci-sysfs.c | 173 ----------------------------------------
>   drivers/pci/pci.h       |   2 +-
>   3 files changed, 169 insertions(+), 174 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 9b48818ced01..b335db21c85e 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -240,6 +240,174 @@ void pci_iov_remove_virtfn(struct pci_dev *dev, int id)
>   	pci_dev_put(dev);
>   }
>   
> +static ssize_t sriov_totalvfs_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
> +}
> +
> +static ssize_t sriov_numvfs_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sprintf(buf, "%u\n", pdev->sriov->num_VFs);
> +}
> +
> +/*
> + * num_vfs > 0; number of VFs to enable
> + * num_vfs = 0; disable all VFs
> + *
> + * Note: SRIOV spec does not allow partial VF
> + *	 disable, so it's all or none.
> + */
> +static ssize_t sriov_numvfs_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	int ret;
> +	u16 num_vfs;
> +
> +	ret = kstrtou16(buf, 0, &num_vfs);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (num_vfs > pci_sriov_get_totalvfs(pdev))
> +		return -ERANGE;
> +
> +	device_lock(&pdev->dev);
> +
> +	if (num_vfs == pdev->sriov->num_VFs)
> +		goto exit;
> +
> +	/* is PF driver loaded w/callback */
> +	if (!pdev->driver || !pdev->driver->sriov_configure) {
> +		pci_info(pdev, "Driver does not support SRIOV configuration via sysfs\n");
> +		ret = -ENOENT;
> +		goto exit;
> +	}
> +
> +	if (num_vfs == 0) {
> +		/* disable VFs */
> +		ret = pdev->driver->sriov_configure(pdev, 0);
> +		goto exit;
> +	}
> +
> +	/* enable VFs */
> +	if (pdev->sriov->num_VFs) {
> +		pci_warn(pdev, "%d VFs already enabled. Disable before enabling %d VFs\n",
> +			 pdev->sriov->num_VFs, num_vfs);
> +		ret = -EBUSY;
> +		goto exit;
> +	}
> +
> +	ret = pdev->driver->sriov_configure(pdev, num_vfs);
> +	if (ret < 0)
> +		goto exit;
> +
> +	if (ret != num_vfs)
> +		pci_warn(pdev, "%d VFs requested; only %d enabled\n",
> +			 num_vfs, ret);
> +
> +exit:
> +	device_unlock(&pdev->dev);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t sriov_offset_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sprintf(buf, "%u\n", pdev->sriov->offset);
> +}
> +
> +static ssize_t sriov_stride_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sprintf(buf, "%u\n", pdev->sriov->stride);
> +}
> +
> +static ssize_t sriov_vf_device_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sprintf(buf, "%x\n", pdev->sriov->vf_device);
> +}
> +
> +static ssize_t sriov_drivers_autoprobe_show(struct device *dev,
> +					    struct device_attribute *attr,
> +					    char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	return sprintf(buf, "%u\n", pdev->sriov->drivers_autoprobe);
> +}
> +
> +static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
> +					     struct device_attribute *attr,
> +					     const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	bool drivers_autoprobe;
> +
> +	if (kstrtobool(buf, &drivers_autoprobe) < 0)
> +		return -EINVAL;
> +
> +	pdev->sriov->drivers_autoprobe = drivers_autoprobe;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RO(sriov_totalvfs);
> +static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
> +static DEVICE_ATTR_RO(sriov_offset);
> +static DEVICE_ATTR_RO(sriov_stride);
> +static DEVICE_ATTR_RO(sriov_vf_device);
> +static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
> +		   sriov_drivers_autoprobe_store);
> +
> +static struct attribute *sriov_dev_attrs[] = {
> +	&dev_attr_sriov_totalvfs.attr,
> +	&dev_attr_sriov_numvfs.attr,
> +	&dev_attr_sriov_offset.attr,
> +	&dev_attr_sriov_stride.attr,
> +	&dev_attr_sriov_vf_device.attr,
> +	&dev_attr_sriov_drivers_autoprobe.attr,
> +	NULL,
> +};
> +
> +static umode_t sriov_attrs_are_visible(struct kobject *kobj,
> +				       struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +
> +	if (!dev_is_pf(dev))
> +		return 0;
> +
> +	return a->mode;
> +}
> +
> +const struct attribute_group sriov_dev_attr_group = {
> +	.attrs = sriov_dev_attrs,
> +	.is_visible = sriov_attrs_are_visible,
> +};
> +
>   int __weak pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs)
>   {
>   	return 0;
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 5bb301efec98..868e35109284 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -548,151 +548,6 @@ static ssize_t devspec_show(struct device *dev,
>   static DEVICE_ATTR_RO(devspec);
>   #endif
>   
> -#ifdef CONFIG_PCI_IOV
> -static ssize_t sriov_totalvfs_show(struct device *dev,
> -				   struct device_attribute *attr,
> -				   char *buf)
> -{
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -
> -	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
> -}
> -
> -
> -static ssize_t sriov_numvfs_show(struct device *dev,
> -				 struct device_attribute *attr,
> -				 char *buf)
> -{
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -
> -	return sprintf(buf, "%u\n", pdev->sriov->num_VFs);
> -}
> -
> -/*
> - * num_vfs > 0; number of VFs to enable
> - * num_vfs = 0; disable all VFs
> - *
> - * Note: SRIOV spec doesn't allow partial VF
> - *       disable, so it's all or none.
> - */
> -static ssize_t sriov_numvfs_store(struct device *dev,
> -				  struct device_attribute *attr,
> -				  const char *buf, size_t count)
> -{
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	int ret;
> -	u16 num_vfs;
> -
> -	ret = kstrtou16(buf, 0, &num_vfs);
> -	if (ret < 0)
> -		return ret;
> -
> -	if (num_vfs > pci_sriov_get_totalvfs(pdev))
> -		return -ERANGE;
> -
> -	device_lock(&pdev->dev);
> -
> -	if (num_vfs == pdev->sriov->num_VFs)
> -		goto exit;
> -
> -	/* is PF driver loaded w/callback */
> -	if (!pdev->driver || !pdev->driver->sriov_configure) {
> -		pci_info(pdev, "Driver doesn't support SRIOV configuration via sysfs\n");
> -		ret = -ENOENT;
> -		goto exit;
> -	}
> -
> -	if (num_vfs == 0) {
> -		/* disable VFs */
> -		ret = pdev->driver->sriov_configure(pdev, 0);
> -		goto exit;
> -	}
> -
> -	/* enable VFs */
> -	if (pdev->sriov->num_VFs) {
> -		pci_warn(pdev, "%d VFs already enabled. Disable before enabling %d VFs\n",
> -			 pdev->sriov->num_VFs, num_vfs);
> -		ret = -EBUSY;
> -		goto exit;
> -	}
> -
> -	ret = pdev->driver->sriov_configure(pdev, num_vfs);
> -	if (ret < 0)
> -		goto exit;
> -
> -	if (ret != num_vfs)
> -		pci_warn(pdev, "%d VFs requested; only %d enabled\n",
> -			 num_vfs, ret);
> -
> -exit:
> -	device_unlock(&pdev->dev);
> -
> -	if (ret < 0)
> -		return ret;
> -
> -	return count;
> -}
> -
> -static ssize_t sriov_offset_show(struct device *dev,
> -				 struct device_attribute *attr,
> -				 char *buf)
> -{
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -
> -	return sprintf(buf, "%u\n", pdev->sriov->offset);
> -}
> -
> -static ssize_t sriov_stride_show(struct device *dev,
> -				 struct device_attribute *attr,
> -				 char *buf)
> -{
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -
> -	return sprintf(buf, "%u\n", pdev->sriov->stride);
> -}
> -
> -static ssize_t sriov_vf_device_show(struct device *dev,
> -				    struct device_attribute *attr,
> -				    char *buf)
> -{
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -
> -	return sprintf(buf, "%x\n", pdev->sriov->vf_device);
> -}
> -
> -static ssize_t sriov_drivers_autoprobe_show(struct device *dev,
> -					    struct device_attribute *attr,
> -					    char *buf)
> -{
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -
> -	return sprintf(buf, "%u\n", pdev->sriov->drivers_autoprobe);
> -}
> -
> -static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
> -					     struct device_attribute *attr,
> -					     const char *buf, size_t count)
> -{
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	bool drivers_autoprobe;
> -
> -	if (kstrtobool(buf, &drivers_autoprobe) < 0)
> -		return -EINVAL;
> -
> -	pdev->sriov->drivers_autoprobe = drivers_autoprobe;
> -
> -	return count;
> -}
> -
> -static DEVICE_ATTR_RO(sriov_totalvfs);
> -static DEVICE_ATTR(sriov_numvfs, 0664, sriov_numvfs_show, sriov_numvfs_store);
> -static DEVICE_ATTR_RO(sriov_offset);
> -static DEVICE_ATTR_RO(sriov_stride);
> -static DEVICE_ATTR_RO(sriov_vf_device);
> -static DEVICE_ATTR(sriov_drivers_autoprobe, 0664, sriov_drivers_autoprobe_show,
> -		   sriov_drivers_autoprobe_store);
> -#endif /* CONFIG_PCI_IOV */
> -
>   static ssize_t driver_override_store(struct device *dev,
>   				     struct device_attribute *attr,
>   				     const char *buf, size_t count)
> @@ -1691,34 +1546,6 @@ static const struct attribute_group pci_dev_hp_attr_group = {
>   	.is_visible = pci_dev_hp_attrs_are_visible,
>   };
>   
> -#ifdef CONFIG_PCI_IOV
> -static struct attribute *sriov_dev_attrs[] = {
> -	&dev_attr_sriov_totalvfs.attr,
> -	&dev_attr_sriov_numvfs.attr,
> -	&dev_attr_sriov_offset.attr,
> -	&dev_attr_sriov_stride.attr,
> -	&dev_attr_sriov_vf_device.attr,
> -	&dev_attr_sriov_drivers_autoprobe.attr,
> -	NULL,
> -};
> -
> -static umode_t sriov_attrs_are_visible(struct kobject *kobj,
> -				       struct attribute *a, int n)
> -{
> -	struct device *dev = kobj_to_dev(kobj);
> -
> -	if (!dev_is_pf(dev))
> -		return 0;
> -
> -	return a->mode;
> -}
> -
> -static const struct attribute_group sriov_dev_attr_group = {
> -	.attrs = sriov_dev_attrs,
> -	.is_visible = sriov_attrs_are_visible,
> -};
> -#endif /* CONFIG_PCI_IOV */
> -
>   static const struct attribute_group pci_dev_attr_group = {
>   	.attrs = pci_dev_dev_attrs,
>   	.is_visible = pci_dev_attrs_are_visible,
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 61bbfd611140..7e3c6c8ae6f9 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -455,7 +455,7 @@ void pci_iov_update_resource(struct pci_dev *dev, int resno);
>   resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
>   void pci_restore_iov_state(struct pci_dev *dev);
>   int pci_iov_bus_range(struct pci_bus *bus);
> -
> +extern const struct attribute_group sriov_dev_attr_group;
>   #else
>   static inline int pci_iov_init(struct pci_dev *dev)
>   {

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer