Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c
@ 2019-08-09 19:57 Kelsey Skunberg
  2019-08-10  7:17 ` [Linux-kernel-mentees] " Greg KH
  2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
  0 siblings, 2 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-09 19:57 UTC (permalink / raw)
  To: bhelgaas, linux-pci, linux-kernel
  Cc: skunberg.kelsey, skhan, linux-kernel-mentees

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c
  2019-08-09 19:57 [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c Kelsey Skunberg
@ 2019-08-10  7:17 ` " Greg KH
  2019-08-10 17:15   ` Bjorn Helgaas
  2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
  1 sibling, 1 reply; 29+ messages in thread
From: Greg KH @ 2019-08-10  7:17 UTC (permalink / raw)
  To: Kelsey Skunberg; +Cc: bhelgaas, linux-pci, linux-kernel, linux-kernel-mentees

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c
  2019-08-10  7:17 ` [Linux-kernel-mentees] " Greg KH
@ 2019-08-10 17:15   ` Bjorn Helgaas
  2019-08-10 17:24     ` Greg KH
  0 siblings, 1 reply; 29+ messages in thread
From: Bjorn Helgaas @ 2019-08-10 17:15 UTC (permalink / raw)
  To: Greg KH; +Cc: Kelsey Skunberg, linux-pci, linux-kernel-mentees, linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c
  2019-08-10 17:15   ` Bjorn Helgaas
@ 2019-08-10 17:24     ` Greg KH
  2019-08-10 21:32       ` Kelsey Skunberg
  0 siblings, 1 reply; 29+ messages in thread
From: Greg KH @ 2019-08-10 17:24 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Kelsey Skunberg, linux-pci, linux-kernel-mentees, linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c
  2019-08-10 17:24     ` Greg KH
@ 2019-08-10 21:32       ` Kelsey Skunberg
  0 siblings, 0 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-10 21:32 UTC (permalink / raw)
  To: Greg KH; +Cc: Bjorn Helgaas, linux-pci, linux-kernel-mentees, linux-kernel

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 0/3] PCI: pci-sysfs.c cleanup
  2019-08-09 19:57 [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c Kelsey Skunberg
  2019-08-10  7:17 ` [Linux-kernel-mentees] " Greg KH
@ 2019-08-13 20:45 ` Kelsey Skunberg
  2019-08-13 20:45   ` [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*() Kelsey Skunberg
                     ` (8 more replies)
  1 sibling, 9 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-13 20:45 UTC (permalink / raw)
  To: bhelgaas, linux-pci, linux-kernel
  Cc: skhan, linux-kernel-mentees, skunberg.kelsey

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*()
  2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
@ 2019-08-13 20:45   ` Kelsey Skunberg
  2019-08-14  7:52     ` [Linux-kernel-mentees] " Greg KH
  2019-08-13 20:45   ` [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal Kelsey Skunberg
                     ` (7 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-13 20:45 UTC (permalink / raw)
  To: bhelgaas, linux-pci, linux-kernel
  Cc: skhan, linux-kernel-mentees, skunberg.kelsey

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal
  2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
  2019-08-13 20:45   ` [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*() Kelsey Skunberg
@ 2019-08-13 20:45   ` Kelsey Skunberg
  2019-08-14  5:38     ` [Linux-kernel-mentees] " Bjorn Helgaas
  2019-08-13 20:45   ` [PATCH v2 3/3] PCI/IOV: Move sysfs SR-IOV functions to iov.c Kelsey Skunberg
                     ` (6 subsequent siblings)
  8 siblings, 1 reply; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-13 20:45 UTC (permalink / raw)
  To: bhelgaas, linux-pci, linux-kernel
  Cc: skhan, linux-kernel-mentees, skunberg.kelsey

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2 3/3] PCI/IOV: Move sysfs SR-IOV functions to iov.c
  2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
  2019-08-13 20:45   ` [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*() Kelsey Skunberg
  2019-08-13 20:45   ` [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal Kelsey Skunberg
@ 2019-08-13 20:45   ` Kelsey Skunberg
  2019-08-14  5:40   ` [Linux-kernel-mentees] [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Bjorn Helgaas
                     ` (5 subsequent siblings)
  8 siblings, 0 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-13 20:45 UTC (permalink / raw)
  To: bhelgaas, linux-pci, linux-kernel
  Cc: skhan, linux-kernel-mentees, skunberg.kelsey

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal
  2019-08-13 20:45   ` [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal Kelsey Skunberg
@ 2019-08-14  5:38     ` " Bjorn Helgaas
  2019-08-14  7:53       ` Greg Kroah-Hartman
  2019-08-15 14:37       ` Don Dutile
  0 siblings, 2 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2019-08-14  5:38 UTC (permalink / raw)
  To: Kelsey Skunberg
  Cc: linux-pci, linux-kernel, linux-kernel-mentees, Bodong Wang,
	Donald Dutile, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 0/3] PCI: pci-sysfs.c cleanup
  2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
                     ` (2 preceding siblings ...)
  2019-08-13 20:45   ` [PATCH v2 3/3] PCI/IOV: Move sysfs SR-IOV functions to iov.c Kelsey Skunberg
@ 2019-08-14  5:40   ` Bjorn Helgaas
  2019-08-15 15:33   ` [PATCH v3 0/4] PCI: Clean up pci-sysfs.c Kelsey Skunberg
                     ` (4 subsequent siblings)
  8 siblings, 0 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2019-08-14  5:40 UTC (permalink / raw)
  To: Kelsey Skunberg
  Cc: linux-pci, linux-kernel, linux-kernel-mentees, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*()
  2019-08-13 20:45   ` [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*() Kelsey Skunberg
@ 2019-08-14  7:52     ` " Greg KH
  2019-08-14 23:14       ` Kelsey Skunberg
  2019-08-15 15:54       ` Kelsey Skunberg
  0 siblings, 2 replies; 29+ messages in thread
From: Greg KH @ 2019-08-14  7:52 UTC (permalink / raw)
  To: Kelsey Skunberg; +Cc: bhelgaas, linux-pci, linux-kernel, linux-kernel-mentees

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal
  2019-08-14  5:38     ` [Linux-kernel-mentees] " Bjorn Helgaas
@ 2019-08-14  7:53       ` Greg Kroah-Hartman
  2019-08-15 14:37       ` Don Dutile
  1 sibling, 0 replies; 29+ messages in thread
From: Greg Kroah-Hartman @ 2019-08-14  7:53 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Kelsey Skunberg, linux-pci, linux-kernel, linux-kernel-mentees,
	Bodong Wang, Donald Dutile

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*()
  2019-08-14  7:52     ` [Linux-kernel-mentees] " Greg KH
@ 2019-08-14 23:14       ` Kelsey Skunberg
  2019-08-15 15:54       ` Kelsey Skunberg
  1 sibling, 0 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-14 23:14 UTC (permalink / raw)
  To: Greg KH; +Cc: bhelgaas, linux-pci, linux-kernel, linux-kernel-mentees, skhan

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal
  2019-08-14  5:38     ` [Linux-kernel-mentees] " Bjorn Helgaas
  2019-08-14  7:53       ` Greg Kroah-Hartman
@ 2019-08-15 14:37       ` Don Dutile
  2019-09-04  6:22         ` Kelsey Skunberg
  1 sibling, 1 reply; 29+ messages in thread
From: Don Dutile @ 2019-08-15 14:37 UTC (permalink / raw)
  To: Bjorn Helgaas, Kelsey Skunberg
  Cc: linux-pci, linux-kernel, linux-kernel-mentees, Bodong Wang,
	Greg Kroah-Hartman

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v3 0/4] PCI: Clean up pci-sysfs.c
  2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
                     ` (3 preceding siblings ...)
  2019-08-14  5:40   ` [Linux-kernel-mentees] [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Bjorn Helgaas
@ 2019-08-15 15:33   ` Kelsey Skunberg
  2019-08-15 16:08     ` Greg KH
                       ` (2 more replies)
  2019-08-15 15:33   ` [PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR* Kelsey Skunberg
                     ` (3 subsequent siblings)
  8 siblings, 3 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-15 15:33 UTC (permalink / raw)
  To: bhelgaas, linux-pci, linux-kernel
  Cc: skhan, linux-kernel-mentees, bodong, ddutile, gregkh, skunberg.kelsey

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR*
  2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
                     ` (4 preceding siblings ...)
  2019-08-15 15:33   ` [PATCH v3 0/4] PCI: Clean up pci-sysfs.c Kelsey Skunberg
@ 2019-08-15 15:33   ` Kelsey Skunberg
  2019-08-15 15:33   ` [PATCH v3 2/4] PCI: sysfs: Change permissions from symbolic to octal Kelsey Skunberg
                     ` (2 subsequent siblings)
  8 siblings, 0 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-15 15:33 UTC (permalink / raw)
  To: bhelgaas, linux-pci, linux-kernel
  Cc: skhan, linux-kernel-mentees, bodong, ddutile, gregkh, skunberg.kelsey

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v3 2/4] PCI: sysfs: Change permissions from symbolic to octal
  2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
                     ` (5 preceding siblings ...)
  2019-08-15 15:33   ` [PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR* Kelsey Skunberg
@ 2019-08-15 15:33   ` Kelsey Skunberg
  2019-08-15 15:33   ` [PATCH v3 3/4] PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO() Kelsey Skunberg
  2019-08-15 15:33   ` [PATCH v3 4/4] PCI/IOV: Move sysfs SR-IOV functions to iov.c Kelsey Skunberg
  8 siblings, 0 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-15 15:33 UTC (permalink / raw)
  To: bhelgaas, linux-pci, linux-kernel
  Cc: skhan, linux-kernel-mentees, bodong, ddutile, gregkh, skunberg.kelsey

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v3 3/4] PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()
  2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
                     ` (6 preceding siblings ...)
  2019-08-15 15:33   ` [PATCH v3 2/4] PCI: sysfs: Change permissions from symbolic to octal Kelsey Skunberg
@ 2019-08-15 15:33   ` Kelsey Skunberg
  2019-08-15 15:33   ` [PATCH v3 4/4] PCI/IOV: Move sysfs SR-IOV functions to iov.c Kelsey Skunberg
  8 siblings, 0 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-15 15:33 UTC (permalink / raw)
  To: bhelgaas, linux-pci, linux-kernel
  Cc: skhan, linux-kernel-mentees, bodong, ddutile, gregkh, skunberg.kelsey

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v3 4/4] PCI/IOV: Move sysfs SR-IOV functions to iov.c
  2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
                     ` (7 preceding siblings ...)
  2019-08-15 15:33   ` [PATCH v3 3/4] PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO() Kelsey Skunberg
@ 2019-08-15 15:33   ` Kelsey Skunberg
  2019-08-15 17:34     ` sathyanarayanan kuppuswamy
  8 siblings, 1 reply; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-15 15:33 UTC (permalink / raw)
  To: bhelgaas, linux-pci, linux-kernel
  Cc: skhan, linux-kernel-mentees, bodong, ddutile, gregkh, skunberg.kelsey

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*()
  2019-08-14  7:52     ` [Linux-kernel-mentees] " Greg KH
  2019-08-14 23:14       ` Kelsey Skunberg
@ 2019-08-15 15:54       ` Kelsey Skunberg
  1 sibling, 0 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-08-15 15:54 UTC (permalink / raw)
  To: Greg KH; +Cc: bhelgaas, linux-pci, linux-kernel, linux-kernel-mentees, skhan

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v3 0/4] PCI: Clean up pci-sysfs.c
  2019-08-15 15:33   ` [PATCH v3 0/4] PCI: Clean up pci-sysfs.c Kelsey Skunberg
@ 2019-08-15 16:08     ` Greg KH
  2019-08-16  4:22     ` Don Dutile
  2019-08-19 22:42     ` [Linux-kernel-mentees] " Bjorn Helgaas
  2 siblings, 0 replies; 29+ messages in thread
From: Greg KH @ 2019-08-15 16:08 UTC (permalink / raw)
  To: Kelsey Skunberg
  Cc: bhelgaas, linux-pci, linux-kernel, skhan, linux-kernel-mentees,
	bodong, ddutile

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>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v3 4/4] PCI/IOV: Move sysfs SR-IOV functions to iov.c
  2019-08-15 15:33   ` [PATCH v3 4/4] PCI/IOV: Move sysfs SR-IOV functions to iov.c Kelsey Skunberg
@ 2019-08-15 17:34     ` sathyanarayanan kuppuswamy
  0 siblings, 0 replies; 29+ messages in thread
From: sathyanarayanan kuppuswamy @ 2019-08-15 17:34 UTC (permalink / raw)
  To: Kelsey Skunberg, bhelgaas, linux-pci, linux-kernel
  Cc: skhan, linux-kernel-mentees, bodong, ddutile, gregkh


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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v3 0/4] PCI: Clean up pci-sysfs.c
  2019-08-15 15:33   ` [PATCH v3 0/4] PCI: Clean up pci-sysfs.c Kelsey Skunberg
  2019-08-15 16:08     ` Greg KH
@ 2019-08-16  4:22     ` Don Dutile
  2019-08-19 22:42     ` [Linux-kernel-mentees] " Bjorn Helgaas
  2 siblings, 0 replies; 29+ messages in thread
From: Don Dutile @ 2019-08-16  4:22 UTC (permalink / raw)
  To: Kelsey Skunberg, bhelgaas, linux-pci, linux-kernel
  Cc: skhan, linux-kernel-mentees, bodong, gregkh

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>


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v3 0/4] PCI: Clean up pci-sysfs.c
  2019-08-15 15:33   ` [PATCH v3 0/4] PCI: Clean up pci-sysfs.c Kelsey Skunberg
  2019-08-15 16:08     ` Greg KH
  2019-08-16  4:22     ` Don Dutile
@ 2019-08-19 22:42     ` " Bjorn Helgaas
  2 siblings, 0 replies; 29+ messages in thread
From: Bjorn Helgaas @ 2019-08-19 22:42 UTC (permalink / raw)
  To: Kelsey Skunberg
  Cc: linux-pci, linux-kernel, ddutile, bodong, linux-kernel-mentees

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal
  2019-08-15 14:37       ` Don Dutile
@ 2019-09-04  6:22         ` Kelsey Skunberg
  2019-09-04 15:32           ` Don Dutile
  2019-09-04 18:33           ` Don Dutile
  0 siblings, 2 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-09-04  6:22 UTC (permalink / raw)
  To: Don Dutile
  Cc: Bjorn Helgaas, linux-pci, linux-kernel, linux-kernel-mentees,
	Bodong Wang, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal
  2019-09-04  6:22         ` Kelsey Skunberg
@ 2019-09-04 15:32           ` Don Dutile
  2019-09-04 18:33           ` Don Dutile
  1 sibling, 0 replies; 29+ messages in thread
From: Don Dutile @ 2019-09-04 15:32 UTC (permalink / raw)
  To: Kelsey Skunberg
  Cc: Bjorn Helgaas, linux-pci, linux-kernel, linux-kernel-mentees,
	Bodong Wang, Greg Kroah-Hartman

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal
  2019-09-04  6:22         ` Kelsey Skunberg
  2019-09-04 15:32           ` Don Dutile
@ 2019-09-04 18:33           ` Don Dutile
  2019-09-05  4:04             ` Kelsey Skunberg
  1 sibling, 1 reply; 29+ messages in thread
From: Don Dutile @ 2019-09-04 18:33 UTC (permalink / raw)
  To: Kelsey Skunberg
  Cc: Bjorn Helgaas, linux-pci, linux-kernel, linux-kernel-mentees,
	Bodong Wang, Greg Kroah-Hartman

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal
  2019-09-04 18:33           ` Don Dutile
@ 2019-09-05  4:04             ` Kelsey Skunberg
  0 siblings, 0 replies; 29+ messages in thread
From: Kelsey Skunberg @ 2019-09-05  4:04 UTC (permalink / raw)
  To: Don Dutile
  Cc: Bjorn Helgaas, linux-pci, linux-kernel, linux-kernel-mentees,
	Bodong Wang, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, back to index

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-09 19:57 [PATCH] PCI/IOV: Move sysfs SR-IOV functions to iov.c Kelsey Skunberg
2019-08-10  7:17 ` [Linux-kernel-mentees] " Greg KH
2019-08-10 17:15   ` Bjorn Helgaas
2019-08-10 17:24     ` Greg KH
2019-08-10 21:32       ` Kelsey Skunberg
2019-08-13 20:45 ` [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Kelsey Skunberg
2019-08-13 20:45   ` [PATCH v2 1/3] PCI: sysfs: Define device attributes with DEVICE_ATTR*() Kelsey Skunberg
2019-08-14  7:52     ` [Linux-kernel-mentees] " Greg KH
2019-08-14 23:14       ` Kelsey Skunberg
2019-08-15 15:54       ` Kelsey Skunberg
2019-08-13 20:45   ` [PATCH v2 2/3] PCI: sysfs: Change permissions from symbolic to octal Kelsey Skunberg
2019-08-14  5:38     ` [Linux-kernel-mentees] " Bjorn Helgaas
2019-08-14  7:53       ` Greg Kroah-Hartman
2019-08-15 14:37       ` Don Dutile
2019-09-04  6:22         ` Kelsey Skunberg
2019-09-04 15:32           ` Don Dutile
2019-09-04 18:33           ` Don Dutile
2019-09-05  4:04             ` Kelsey Skunberg
2019-08-13 20:45   ` [PATCH v2 3/3] PCI/IOV: Move sysfs SR-IOV functions to iov.c Kelsey Skunberg
2019-08-14  5:40   ` [Linux-kernel-mentees] [PATCH v2 0/3] PCI: pci-sysfs.c cleanup Bjorn Helgaas
2019-08-15 15:33   ` [PATCH v3 0/4] PCI: Clean up pci-sysfs.c Kelsey Skunberg
2019-08-15 16:08     ` Greg KH
2019-08-16  4:22     ` Don Dutile
2019-08-19 22:42     ` [Linux-kernel-mentees] " Bjorn Helgaas
2019-08-15 15:33   ` [PATCH v3 1/4] PCI: sysfs: Define device attributes with DEVICE_ATTR* Kelsey Skunberg
2019-08-15 15:33   ` [PATCH v3 2/4] PCI: sysfs: Change permissions from symbolic to octal Kelsey Skunberg
2019-08-15 15:33   ` [PATCH v3 3/4] PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO() Kelsey Skunberg
2019-08-15 15:33   ` [PATCH v3 4/4] PCI/IOV: Move sysfs SR-IOV functions to iov.c Kelsey Skunberg
2019-08-15 17:34     ` sathyanarayanan kuppuswamy

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org linux-pci@archiver.kernel.org
	public-inbox-index linux-pci


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox