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 <[email protected]>
---
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
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
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
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
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
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
[+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
[+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/[email protected]
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
> > ---
> > 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
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
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
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 <[email protected]>
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan
<[email protected]>
> ---
> 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
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 <[email protected]>
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
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
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
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
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
On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> <[email protected]> wrote:
> >
> > Defining device attributes should be done through the helper
> > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > __ATTR* to now use its equivalent DEVICE_ATTR*.
> >
> > Example of old:
> >
> > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> >
> > Example of new:
> >
> > static DEVICE_ATTR_RO(_name);
> >
> > Signed-off-by: Kelsey Skunberg <[email protected]>
> > ---
> > drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> > 1 file changed, 27 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 965c72104150..8af7944fdccb 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> > }
> > return count;
> > }
> > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > - (S_IWUSR|S_IWGRP),
> > - NULL, dev_rescan_store);
> > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> >
> > static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > const char *buf, size_t count)
> > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > return count;
> > }
> > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > - (S_IWUSR|S_IWGRP),
> > - NULL, remove_store);
> > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > + remove_store);
> >
> > static ssize_t dev_bus_rescan_store(struct device *dev,
> > struct device_attribute *attr,
> > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> > }
> > return count;
> > }
> > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
>
> This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> There is also mismatch now between real functionality and documentation
> Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> descriptions.
>
> Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
>
> Here is a comparison between two stable kernels (with and without this
> patch series):
>
> v5.4
> # find /sys -name '*rescan'
> /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> /sys/bus/pci/rescan
>
> v4.19
> # find /sys -name '*rescan'
> /sys/devices/pci0000:00/0000:00:01.2/rescan
> /sys/devices/pci0000:00/0000:00:01.0/rescan
> /sys/devices/pci0000:00/0000:00:04.0/rescan
> /sys/devices/pci0000:00/0000:00:00.0/rescan
> /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> /sys/devices/pci0000:00/0000:00:01.3/rescan
> /sys/devices/pci0000:00/0000:00:03.0/rescan
> /sys/devices/pci0000:00/0000:00:01.1/rescan
> /sys/devices/pci0000:00/0000:00:02.0/rescan
> /sys/devices/pci0000:00/0000:00:05.0/rescan
> /sys/bus/pci/rescan
>
> Do we maintain this kind of API as non-changeable?
Yeah, that's a bug and should be fixed, sorry for missing that on
review.
Kelsey, can you fix this up?
thanks,
greg k-h
On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
<[email protected]> wrote:
>
> Defining device attributes should be done through the helper
> DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> __ATTR* to now use its equivalent DEVICE_ATTR*.
>
> Example of old:
>
> static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
>
> Example of new:
>
> static DEVICE_ATTR_RO(_name);
>
> Signed-off-by: Kelsey Skunberg <[email protected]>
> ---
> drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> 1 file changed, 27 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 965c72104150..8af7944fdccb 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> }
> return count;
> }
> -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> - (S_IWUSR|S_IWGRP),
> - NULL, dev_rescan_store);
> +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
>
> static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> return count;
> }
> -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> - (S_IWUSR|S_IWGRP),
> - NULL, remove_store);
> +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> + remove_store);
>
> static ssize_t dev_bus_rescan_store(struct device *dev,
> struct device_attribute *attr,
> @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> }
> return count;
> }
> -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
There is also mismatch now between real functionality and documentation
Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
descriptions.
Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
Here is a comparison between two stable kernels (with and without this
patch series):
v5.4
# find /sys -name '*rescan'
/sys/devices/pci0000:00/0000:00:01.2/dev_rescan
/sys/devices/pci0000:00/0000:00:01.0/dev_rescan
/sys/devices/pci0000:00/0000:00:04.0/dev_rescan
/sys/devices/pci0000:00/0000:00:00.0/dev_rescan
/sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
/sys/devices/pci0000:00/0000:00:01.3/dev_rescan
/sys/devices/pci0000:00/0000:00:03.0/dev_rescan
/sys/devices/pci0000:00/0000:00:01.1/dev_rescan
/sys/devices/pci0000:00/0000:00:02.0/dev_rescan
/sys/devices/pci0000:00/0000:00:05.0/dev_rescan
/sys/bus/pci/rescan
v4.19
# find /sys -name '*rescan'
/sys/devices/pci0000:00/0000:00:01.2/rescan
/sys/devices/pci0000:00/0000:00:01.0/rescan
/sys/devices/pci0000:00/0000:00:04.0/rescan
/sys/devices/pci0000:00/0000:00:00.0/rescan
/sys/devices/pci0000:00/pci_bus/0000:00/rescan
/sys/devices/pci0000:00/0000:00:01.3/rescan
/sys/devices/pci0000:00/0000:00:03.0/rescan
/sys/devices/pci0000:00/0000:00:01.1/rescan
/sys/devices/pci0000:00/0000:00:02.0/rescan
/sys/devices/pci0000:00/0000:00:05.0/rescan
/sys/bus/pci/rescan
Do we maintain this kind of API as non-changeable?
Thanks,
Ruslan
>
> #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
> static ssize_t d3cold_allowed_store(struct device *dev,
> @@ -687,16 +684,14 @@ static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
> return count;
> }
>
> -static struct device_attribute sriov_totalvfs_attr = __ATTR_RO(sriov_totalvfs);
> -static struct device_attribute sriov_numvfs_attr =
> - __ATTR(sriov_numvfs, (S_IRUGO|S_IWUSR|S_IWGRP),
> - sriov_numvfs_show, sriov_numvfs_store);
> -static struct device_attribute sriov_offset_attr = __ATTR_RO(sriov_offset);
> -static struct device_attribute sriov_stride_attr = __ATTR_RO(sriov_stride);
> -static struct device_attribute sriov_vf_device_attr = __ATTR_RO(sriov_vf_device);
> -static struct device_attribute sriov_drivers_autoprobe_attr =
> - __ATTR(sriov_drivers_autoprobe, (S_IRUGO|S_IWUSR|S_IWGRP),
> - sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> +static DEVICE_ATTR_RO(sriov_totalvfs);
> +static DEVICE_ATTR(sriov_numvfs, (S_IRUGO | S_IWUSR | S_IWGRP),
> + sriov_numvfs_show, sriov_numvfs_store);
> +static DEVICE_ATTR_RO(sriov_offset);
> +static DEVICE_ATTR_RO(sriov_stride);
> +static DEVICE_ATTR_RO(sriov_vf_device);
> +static DEVICE_ATTR(sriov_drivers_autoprobe, (S_IRUGO | S_IWUSR | S_IWGRP),
> + sriov_drivers_autoprobe_show, sriov_drivers_autoprobe_store);
> #endif /* CONFIG_PCI_IOV */
>
> static ssize_t driver_override_store(struct device *dev,
> @@ -792,7 +787,7 @@ static struct attribute *pcie_dev_attrs[] = {
> };
>
> static struct attribute *pcibus_attrs[] = {
> - &dev_attr_rescan.attr,
> + &dev_attr_bus_rescan.attr,
> &dev_attr_cpuaffinity.attr,
> &dev_attr_cpulistaffinity.attr,
> NULL,
> @@ -820,7 +815,7 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
> !!(pdev->resource[PCI_ROM_RESOURCE].flags &
> IORESOURCE_ROM_SHADOW));
> }
> -static struct device_attribute vga_attr = __ATTR_RO(boot_vga);
> +static DEVICE_ATTR_RO(boot_vga);
>
> static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
> struct bin_attribute *bin_attr, char *buf,
> @@ -1458,7 +1453,7 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> return count;
> }
>
> -static struct device_attribute reset_attr = __ATTR(reset, 0200, NULL, reset_store);
> +static DEVICE_ATTR(reset, 0200, NULL, reset_store);
>
> static int pci_create_capabilities_sysfs(struct pci_dev *dev)
> {
> @@ -1468,7 +1463,7 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
> pcie_aspm_create_sysfs_dev_files(dev);
>
> if (dev->reset_fn) {
> - retval = device_create_file(&dev->dev, &reset_attr);
> + retval = device_create_file(&dev->dev, &dev_attr_reset);
> if (retval)
> goto error;
> }
> @@ -1553,7 +1548,7 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
> pcie_vpd_remove_sysfs_dev_files(dev);
> pcie_aspm_remove_sysfs_dev_files(dev);
> if (dev->reset_fn) {
> - device_remove_file(&dev->dev, &reset_attr);
> + device_remove_file(&dev->dev, &dev_attr_reset);
> dev->reset_fn = 0;
> }
> }
> @@ -1606,7 +1601,7 @@ static int __init pci_sysfs_init(void)
> late_initcall(pci_sysfs_init);
>
> static struct attribute *pci_dev_dev_attrs[] = {
> - &vga_attr.attr,
> + &dev_attr_boot_vga.attr,
> NULL,
> };
>
> @@ -1616,7 +1611,7 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
> struct device *dev = kobj_to_dev(kobj);
> struct pci_dev *pdev = to_pci_dev(dev);
>
> - if (a == &vga_attr.attr)
> + if (a == &dev_attr_boot_vga.attr)
> if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> return 0;
>
> @@ -1624,8 +1619,8 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
> }
>
> static struct attribute *pci_dev_hp_attrs[] = {
> - &dev_remove_attr.attr,
> - &dev_rescan_attr.attr,
> + &dev_attr_remove.attr,
> + &dev_attr_rescan.attr,
> NULL,
> };
>
> @@ -1699,12 +1694,12 @@ static const struct attribute_group pci_dev_hp_attr_group = {
>
> #ifdef CONFIG_PCI_IOV
> static struct attribute *sriov_dev_attrs[] = {
> - &sriov_totalvfs_attr.attr,
> - &sriov_numvfs_attr.attr,
> - &sriov_offset_attr.attr,
> - &sriov_stride_attr.attr,
> - &sriov_vf_device_attr.attr,
> - &sriov_drivers_autoprobe_attr.attr,
> + &dev_attr_sriov_totalvfs.attr,
> + &dev_attr_sriov_numvfs.attr,
> + &dev_attr_sriov_offset.attr,
> + &dev_attr_sriov_stride.attr,
> + &dev_attr_sriov_vf_device.attr,
> + &dev_attr_sriov_drivers_autoprobe.attr,
> NULL,
> };
>
> --
> 2.20.1
>
On Sat, Mar 14, 2020 at 5:20 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> > On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> > <[email protected]> wrote:
> > >
> > > Defining device attributes should be done through the helper
> > > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > > __ATTR* to now use its equivalent DEVICE_ATTR*.
> > >
> > > Example of old:
> > >
> > > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> > >
> > > Example of new:
> > >
> > > static DEVICE_ATTR_RO(_name);
> > >
> > > Signed-off-by: Kelsey Skunberg <[email protected]>
> > > ---
> > > drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> > > 1 file changed, 27 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > index 965c72104150..8af7944fdccb 100644
> > > --- a/drivers/pci/pci-sysfs.c
> > > +++ b/drivers/pci/pci-sysfs.c
> > > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> > > }
> > > return count;
> > > }
> > > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > > - (S_IWUSR|S_IWGRP),
> > > - NULL, dev_rescan_store);
> > > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> > >
> > > static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > const char *buf, size_t count)
> > > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > > return count;
> > > }
> > > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > > - (S_IWUSR|S_IWGRP),
> > > - NULL, remove_store);
> > > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > > + remove_store);
> > >
> > > static ssize_t dev_bus_rescan_store(struct device *dev,
> > > struct device_attribute *attr,
> > > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> > > }
> > > return count;
> > > }
> > > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> >
> > This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> > There is also mismatch now between real functionality and documentation
> > Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> > descriptions.
> >
> > Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
> >
> > Here is a comparison between two stable kernels (with and without this
> > patch series):
> >
> > v5.4
> > # find /sys -name '*rescan'
> > /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> > /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> > /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> > /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> > /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> > /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> > /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> > /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> > /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> > /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> > /sys/bus/pci/rescan
> >
> > v4.19
> > # find /sys -name '*rescan'
> > /sys/devices/pci0000:00/0000:00:01.2/rescan
> > /sys/devices/pci0000:00/0000:00:01.0/rescan
> > /sys/devices/pci0000:00/0000:00:04.0/rescan
> > /sys/devices/pci0000:00/0000:00:00.0/rescan
> > /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> > /sys/devices/pci0000:00/0000:00:01.3/rescan
> > /sys/devices/pci0000:00/0000:00:03.0/rescan
> > /sys/devices/pci0000:00/0000:00:01.1/rescan
> > /sys/devices/pci0000:00/0000:00:02.0/rescan
> > /sys/devices/pci0000:00/0000:00:05.0/rescan
> > /sys/bus/pci/rescan
> >
> > Do we maintain this kind of API as non-changeable?
>
> Yeah, that's a bug and should be fixed, sorry for missing that on
> review.
>
> Kelsey, can you fix this up?
>
> thanks,
>
> greg k-h
I'd be happy to help get this fixed up.
Would it be proper to go back to using DEVICE_ATTR() for 'bus_rescan'
and 'dev_rescan' in order to change their names back to 'rescan'?
The name changes were done so the correct *_store() would still be
called. When using DEVICE_ATTR() the *_store() name is passed as the
last argument, as seen here:
static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
When using the helper, only the name is passed and it assumes default
<name>_show(), as seen here:
static DEVICE_ATTR_WO(dev_rescan); # (This would assume
dev_rescan_store())
This can be verified in Documentation/filesystems/sysfs.txt.
There is already a rescan attribute using rescan_store(), so changing
at least one of these to DEVICE_ATTR_WO(rescan) would be conflicting.
I understand it's ideal to stay away from using DEVICE_ATTR() unless
an unusual mode is needed. Would having a different name vs
name_store() be another reason to justify using DEVICE_ATTR()?
Thank you Ruslan for pointing this out!
- Kelsey
On Tue, Mar 24, 2020 at 12:10:33AM -0600, Kelsey wrote:
> On Sat, Mar 14, 2020 at 5:20 AM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> > > On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> > > <[email protected]> wrote:
> > > >
> > > > Defining device attributes should be done through the helper
> > > > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > > > __ATTR* to now use its equivalent DEVICE_ATTR*.
> > > >
> > > > Example of old:
> > > >
> > > > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> > > >
> > > > Example of new:
> > > >
> > > > static DEVICE_ATTR_RO(_name);
> > > >
> > > > Signed-off-by: Kelsey Skunberg <[email protected]>
> > > > ---
> > > > drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> > > > 1 file changed, 27 insertions(+), 32 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > index 965c72104150..8af7944fdccb 100644
> > > > --- a/drivers/pci/pci-sysfs.c
> > > > +++ b/drivers/pci/pci-sysfs.c
> > > > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> > > > }
> > > > return count;
> > > > }
> > > > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > > > - (S_IWUSR|S_IWGRP),
> > > > - NULL, dev_rescan_store);
> > > > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> > > >
> > > > static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > const char *buf, size_t count)
> > > > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > > > return count;
> > > > }
> > > > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > > > - (S_IWUSR|S_IWGRP),
> > > > - NULL, remove_store);
> > > > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > > > + remove_store);
> > > >
> > > > static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > struct device_attribute *attr,
> > > > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > }
> > > > return count;
> > > > }
> > > > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > > > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> > >
> > > This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> > > There is also mismatch now between real functionality and documentation
> > > Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> > > descriptions.
> > >
> > > Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
> > >
> > > Here is a comparison between two stable kernels (with and without this
> > > patch series):
> > >
> > > v5.4
> > > # find /sys -name '*rescan'
> > > /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> > > /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> > > /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> > > /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> > > /sys/bus/pci/rescan
> > >
> > > v4.19
> > > # find /sys -name '*rescan'
> > > /sys/devices/pci0000:00/0000:00:01.2/rescan
> > > /sys/devices/pci0000:00/0000:00:01.0/rescan
> > > /sys/devices/pci0000:00/0000:00:04.0/rescan
> > > /sys/devices/pci0000:00/0000:00:00.0/rescan
> > > /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> > > /sys/devices/pci0000:00/0000:00:01.3/rescan
> > > /sys/devices/pci0000:00/0000:00:03.0/rescan
> > > /sys/devices/pci0000:00/0000:00:01.1/rescan
> > > /sys/devices/pci0000:00/0000:00:02.0/rescan
> > > /sys/devices/pci0000:00/0000:00:05.0/rescan
> > > /sys/bus/pci/rescan
> > >
> > > Do we maintain this kind of API as non-changeable?
> >
> > Yeah, that's a bug and should be fixed, sorry for missing that on
> > review.
> >
> > Kelsey, can you fix this up?
> >
> > thanks,
> >
> > greg k-h
>
> I'd be happy to help get this fixed up.
>
> Would it be proper to go back to using DEVICE_ATTR() for 'bus_rescan'
> and 'dev_rescan' in order to change their names back to 'rescan'?
Yes.
thanks,
greg k-h
On Tue, Mar 24, 2020 at 12:24 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Tue, Mar 24, 2020 at 12:10:33AM -0600, Kelsey wrote:
> > On Sat, Mar 14, 2020 at 5:20 AM Greg Kroah-Hartman
> > <[email protected]> wrote:
> > >
> > > On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> > > > On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> > > > <[email protected]> wrote:
> > > > >
> > > > > Defining device attributes should be done through the helper
> > > > > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > > > > __ATTR* to now use its equivalent DEVICE_ATTR*.
> > > > >
> > > > > Example of old:
> > > > >
> > > > > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> > > > >
> > > > > Example of new:
> > > > >
> > > > > static DEVICE_ATTR_RO(_name);
> > > > >
> > > > > Signed-off-by: Kelsey Skunberg <[email protected]>
> > > > > ---
> > > > > drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> > > > > 1 file changed, 27 insertions(+), 32 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > > index 965c72104150..8af7944fdccb 100644
> > > > > --- a/drivers/pci/pci-sysfs.c
> > > > > +++ b/drivers/pci/pci-sysfs.c
> > > > > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> > > > > }
> > > > > return count;
> > > > > }
> > > > > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > > > > - (S_IWUSR|S_IWGRP),
> > > > > - NULL, dev_rescan_store);
> > > > > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> > > > >
> > > > > static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > > const char *buf, size_t count)
> > > > > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > > pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > > > > return count;
> > > > > }
> > > > > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > > > > - (S_IWUSR|S_IWGRP),
> > > > > - NULL, remove_store);
> > > > > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > > > > + remove_store);
> > > > >
> > > > > static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > > struct device_attribute *attr,
> > > > > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > > }
> > > > > return count;
> > > > > }
> > > > > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > > > > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> > > >
> > > > This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> > > > There is also mismatch now between real functionality and documentation
> > > > Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> > > > descriptions.
> > > >
> > > > Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
> > > >
> > > > Here is a comparison between two stable kernels (with and without this
> > > > patch series):
> > > >
> > > > v5.4
> > > > # find /sys -name '*rescan'
> > > > /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> > > > /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> > > > /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> > > > /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> > > > /sys/bus/pci/rescan
> > > >
> > > > v4.19
> > > > # find /sys -name '*rescan'
> > > > /sys/devices/pci0000:00/0000:00:01.2/rescan
> > > > /sys/devices/pci0000:00/0000:00:01.0/rescan
> > > > /sys/devices/pci0000:00/0000:00:04.0/rescan
> > > > /sys/devices/pci0000:00/0000:00:00.0/rescan
> > > > /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> > > > /sys/devices/pci0000:00/0000:00:01.3/rescan
> > > > /sys/devices/pci0000:00/0000:00:03.0/rescan
> > > > /sys/devices/pci0000:00/0000:00:01.1/rescan
> > > > /sys/devices/pci0000:00/0000:00:02.0/rescan
> > > > /sys/devices/pci0000:00/0000:00:05.0/rescan
> > > > /sys/bus/pci/rescan
> > > >
> > > > Do we maintain this kind of API as non-changeable?
> > >
> > > Yeah, that's a bug and should be fixed, sorry for missing that on
> > > review.
> > >
> > > Kelsey, can you fix this up?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > I'd be happy to help get this fixed up.
> >
> > Would it be proper to go back to using DEVICE_ATTR() for 'bus_rescan'
> > and 'dev_rescan' in order to change their names back to 'rescan'?
>
> Yes.
>
> thanks,
>
> greg k-h
Ack. Sent a patch out. Will stay posted in case any updates need to be made.
commit 4cb9e42d3226 ("PCI: sysfs: Change bus_rescan and dev_rescan to rescan")
Thanks!
- Kelsey
On Tue, Mar 24, 2020 at 05:53:59PM -0600, Kelsey wrote:
> On Tue, Mar 24, 2020 at 12:24 AM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Tue, Mar 24, 2020 at 12:10:33AM -0600, Kelsey wrote:
> > > On Sat, Mar 14, 2020 at 5:20 AM Greg Kroah-Hartman
> > > <[email protected]> wrote:
> > > >
> > > > On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> > > > > On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > Defining device attributes should be done through the helper
> > > > > > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > > > > > __ATTR* to now use its equivalent DEVICE_ATTR*.
> > > > > >
> > > > > > Example of old:
> > > > > >
> > > > > > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> > > > > >
> > > > > > Example of new:
> > > > > >
> > > > > > static DEVICE_ATTR_RO(_name);
> > > > > >
> > > > > > Signed-off-by: Kelsey Skunberg <[email protected]>
> > > > > > ---
> > > > > > drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> > > > > > 1 file changed, 27 insertions(+), 32 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > > > index 965c72104150..8af7944fdccb 100644
> > > > > > --- a/drivers/pci/pci-sysfs.c
> > > > > > +++ b/drivers/pci/pci-sysfs.c
> > > > > > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> > > > > > }
> > > > > > return count;
> > > > > > }
> > > > > > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > > > > > - (S_IWUSR|S_IWGRP),
> > > > > > - NULL, dev_rescan_store);
> > > > > > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> > > > > >
> > > > > > static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > > > const char *buf, size_t count)
> > > > > > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > > > pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > > > > > return count;
> > > > > > }
> > > > > > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > > > > > - (S_IWUSR|S_IWGRP),
> > > > > > - NULL, remove_store);
> > > > > > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > > > > > + remove_store);
> > > > > >
> > > > > > static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > > > struct device_attribute *attr,
> > > > > > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > > > }
> > > > > > return count;
> > > > > > }
> > > > > > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > > > > > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> > > > >
> > > > > This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> > > > > There is also mismatch now between real functionality and documentation
> > > > > Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> > > > > descriptions.
> > > > >
> > > > > Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
> > > > >
> > > > > Here is a comparison between two stable kernels (with and without this
> > > > > patch series):
> > > > >
> > > > > v5.4
> > > > > # find /sys -name '*rescan'
> > > > > /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> > > > > /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> > > > > /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> > > > > /sys/bus/pci/rescan
> > > > >
> > > > > v4.19
> > > > > # find /sys -name '*rescan'
> > > > > /sys/devices/pci0000:00/0000:00:01.2/rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.0/rescan
> > > > > /sys/devices/pci0000:00/0000:00:04.0/rescan
> > > > > /sys/devices/pci0000:00/0000:00:00.0/rescan
> > > > > /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.3/rescan
> > > > > /sys/devices/pci0000:00/0000:00:03.0/rescan
> > > > > /sys/devices/pci0000:00/0000:00:01.1/rescan
> > > > > /sys/devices/pci0000:00/0000:00:02.0/rescan
> > > > > /sys/devices/pci0000:00/0000:00:05.0/rescan
> > > > > /sys/bus/pci/rescan
> > > > >
> > > > > Do we maintain this kind of API as non-changeable?
> > > >
> > > > Yeah, that's a bug and should be fixed, sorry for missing that on
> > > > review.
> > > >
> > > > Kelsey, can you fix this up?
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > I'd be happy to help get this fixed up.
> > >
> > > Would it be proper to go back to using DEVICE_ATTR() for 'bus_rescan'
> > > and 'dev_rescan' in order to change their names back to 'rescan'?
> >
> > Yes.
> >
> > thanks,
> >
> > greg k-h
>
>
> Ack. Sent a patch out. Will stay posted in case any updates need to be made.
>
> commit 4cb9e42d3226 ("PCI: sysfs: Change bus_rescan and dev_rescan to rescan")
That's your local commit, not the commit in Linus's tree :)
greg k-h
On Wed, Mar 25, 2020 at 1:17 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Tue, Mar 24, 2020 at 05:53:59PM -0600, Kelsey wrote:
> > On Tue, Mar 24, 2020 at 12:24 AM Greg Kroah-Hartman
> > <[email protected]> wrote:
> > >
> > > On Tue, Mar 24, 2020 at 12:10:33AM -0600, Kelsey wrote:
> > > > On Sat, Mar 14, 2020 at 5:20 AM Greg Kroah-Hartman
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Sat, Mar 14, 2020 at 12:51:47PM +0200, Ruslan Bilovol wrote:
> > > > > > On Thu, Aug 15, 2019 at 7:01 PM Kelsey Skunberg
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > Defining device attributes should be done through the helper
> > > > > > > DEVICE_ATTR_RO(), DEVICE_ATTR_WO(), or similar. Change all instances using
> > > > > > > __ATTR* to now use its equivalent DEVICE_ATTR*.
> > > > > > >
> > > > > > > Example of old:
> > > > > > >
> > > > > > > static struct device_attribute dev_name_##_attr=__ATTR_RO(_name);
> > > > > > >
> > > > > > > Example of new:
> > > > > > >
> > > > > > > static DEVICE_ATTR_RO(_name);
> > > > > > >
> > > > > > > Signed-off-by: Kelsey Skunberg <[email protected]>
> > > > > > > ---
> > > > > > > drivers/pci/pci-sysfs.c | 59 +++++++++++++++++++----------------------
> > > > > > > 1 file changed, 27 insertions(+), 32 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > > > > > index 965c72104150..8af7944fdccb 100644
> > > > > > > --- a/drivers/pci/pci-sysfs.c
> > > > > > > +++ b/drivers/pci/pci-sysfs.c
> > > > > > > @@ -464,9 +464,7 @@ static ssize_t dev_rescan_store(struct device *dev,
> > > > > > > }
> > > > > > > return count;
> > > > > > > }
> > > > > > > -static struct device_attribute dev_rescan_attr = __ATTR(rescan,
> > > > > > > - (S_IWUSR|S_IWGRP),
> > > > > > > - NULL, dev_rescan_store);
> > > > > > > +static DEVICE_ATTR(rescan, (S_IWUSR | S_IWGRP), NULL, dev_rescan_store);
> > > > > > >
> > > > > > > static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > > > > const char *buf, size_t count)
> > > > > > > @@ -480,9 +478,8 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
> > > > > > > pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
> > > > > > > return count;
> > > > > > > }
> > > > > > > -static struct device_attribute dev_remove_attr = __ATTR_IGNORE_LOCKDEP(remove,
> > > > > > > - (S_IWUSR|S_IWGRP),
> > > > > > > - NULL, remove_store);
> > > > > > > +static DEVICE_ATTR_IGNORE_LOCKDEP(remove, (S_IWUSR | S_IWGRP), NULL,
> > > > > > > + remove_store);
> > > > > > >
> > > > > > > static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > > > > struct device_attribute *attr,
> > > > > > > @@ -504,7 +501,7 @@ static ssize_t dev_bus_rescan_store(struct device *dev,
> > > > > > > }
> > > > > > > return count;
> > > > > > > }
> > > > > > > -static DEVICE_ATTR(rescan, (S_IWUSR|S_IWGRP), NULL, dev_bus_rescan_store);
> > > > > > > +static DEVICE_ATTR(bus_rescan, (S_IWUSR | S_IWGRP), NULL, dev_bus_rescan_store);
> > > > > >
> > > > > > This patch renamed 'rescan' to 'bus_rescan' and broke my userspace application.
> > > > > > There is also mismatch now between real functionality and documentation
> > > > > > Documentation/ABI/testing/sysfs-bus-pci which still contains old "rescan"
> > > > > > descriptions.
> > > > > >
> > > > > > Another patch from this patch series also renamed 'rescan' to 'dev_rescan'
> > > > > >
> > > > > > Here is a comparison between two stable kernels (with and without this
> > > > > > patch series):
> > > > > >
> > > > > > v5.4
> > > > > > # find /sys -name '*rescan'
> > > > > > /sys/devices/pci0000:00/0000:00:01.2/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.0/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:04.0/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:00.0/dev_rescan
> > > > > > /sys/devices/pci0000:00/pci_bus/0000:00/bus_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.3/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:03.0/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.1/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:02.0/dev_rescan
> > > > > > /sys/devices/pci0000:00/0000:00:05.0/dev_rescan
> > > > > > /sys/bus/pci/rescan
> > > > > >
> > > > > > v4.19
> > > > > > # find /sys -name '*rescan'
> > > > > > /sys/devices/pci0000:00/0000:00:01.2/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.0/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:04.0/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:00.0/rescan
> > > > > > /sys/devices/pci0000:00/pci_bus/0000:00/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.3/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:03.0/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:01.1/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:02.0/rescan
> > > > > > /sys/devices/pci0000:00/0000:00:05.0/rescan
> > > > > > /sys/bus/pci/rescan
> > > > > >
> > > > > > Do we maintain this kind of API as non-changeable?
> > > > >
> > > > > Yeah, that's a bug and should be fixed, sorry for missing that on
> > > > > review.
> > > > >
> > > > > Kelsey, can you fix this up?
> > > > >
> > > > > thanks,
> > > > >
> > > > > greg k-h
> > > >
> > > > I'd be happy to help get this fixed up.
> > > >
> > > > Would it be proper to go back to using DEVICE_ATTR() for 'bus_rescan'
> > > > and 'dev_rescan' in order to change their names back to 'rescan'?
> > >
> > > Yes.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> >
> > Ack. Sent a patch out. Will stay posted in case any updates need to be made.
> >
> > commit 4cb9e42d3226 ("PCI: sysfs: Change bus_rescan and dev_rescan to rescan")
>
> That's your local commit, not the commit in Linus's tree :)
>
> greg k-h
hah, whoops! Wanted to reference the patch name and didn't think that
through. Maybe would have been better to reference a link to the patch
anyways. :)
https://lore.kernel.org/r/[email protected]
- Kelsey