[PATCH v2] PCI: sysfs: Change bus_rescan and dev_rescan to rescan

[PATCH v2] PCI: sysfs: Change bus_rescan and dev_rescan to rescan

[Kelsey Skunberg]

From: Kelsey Skunberg <kelsey.skunberg@gmail.com>

rename device attribute name arguments 'bus_rescan' and 'dev_rescan' to 'rescan'
to avoid breaking userspace applications.

The attribute argument names were changed in the following commits:
8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")

Revert the names used for attributes back to the names used before the above
patches were applied. This also requires to change DEVICE_ATTR_WO() to
DEVICE_ATTR() and __ATTR().

Note when using DEVICE_ATTR() the attribute is automatically named
dev_attr_<name>.attr. To avoid duplicated names between attributes, use
__ATTR() instead of DEVICE_ATTR() to a assign a custom attribute name for
dev_rescan.

change bus_rescan_store() to dev_bus_rescan_store() to complete matching the
names used before the mentioned patches were applied.

Fixes: 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
Fixes: 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")

Cc: stable <stable@vger.kernel.org>
Signed-off-by: Kelsey Skunberg <kelsey.skunberg@gmail.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---

v2 updates: 
	commit log updated to include 'Fixes: *' and Cc: stable to aid commit
	being backported properly.

 drivers/pci/pci-sysfs.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 13f766db0684..667e13d597ff 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -464,7 +464,10 @@ static ssize_t dev_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR_WO(dev_rescan);
+static struct device_attribute dev_rescan_attr = __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)
@@ -481,9 +484,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 bus_rescan_store(struct device *dev,
-				struct device_attribute *attr,
-				const char *buf, size_t count)
+static ssize_t dev_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 +504,7 @@ static ssize_t bus_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR_WO(bus_rescan);
+static DEVICE_ATTR(rescan, 0220, NULL, dev_bus_rescan_store);
 
 #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
 static ssize_t d3cold_allowed_store(struct device *dev,
@@ -641,7 +644,7 @@ static struct attribute *pcie_dev_attrs[] = {
 };
 
 static struct attribute *pcibus_attrs[] = {
-	&dev_attr_bus_rescan.attr,
+	&dev_attr_rescan.attr,
 	&dev_attr_cpuaffinity.attr,
 	&dev_attr_cpulistaffinity.attr,
 	NULL,
@@ -1487,7 +1490,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_dev_rescan.attr,
+	&dev_rescan_attr.attr,
 	NULL,
 };
 
-- 
2.20.1

Re: [PATCH v2] PCI: sysfs: Change bus_rescan and dev_rescan to rescan

[Ruslan Bilovol]

On 3/25/20 5:17 PM, Kelsey Skunberg wrote:
> From: Kelsey Skunberg <kelsey.skunberg@gmail.com>
> 
> rename device attribute name arguments 'bus_rescan' and 'dev_rescan' to 'rescan'
> to avoid breaking userspace applications.
> 
> The attribute argument names were changed in the following commits:
> 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
> 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
> 
> Revert the names used for attributes back to the names used before the above
> patches were applied. This also requires to change DEVICE_ATTR_WO() to
> DEVICE_ATTR() and __ATTR().
> 
> Note when using DEVICE_ATTR() the attribute is automatically named
> dev_attr_<name>.attr. To avoid duplicated names between attributes, use
> __ATTR() instead of DEVICE_ATTR() to a assign a custom attribute name for
> dev_rescan.
> 
> change bus_rescan_store() to dev_bus_rescan_store() to complete matching the
> names used before the mentioned patches were applied.
> 
> Fixes: 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
> Fixes: 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")

Thanks Kelsey for the quick fix.

Tested-by: Ruslan Bilovol <rbilovol@cisco.com>

> 
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Kelsey Skunberg <kelsey.skunberg@gmail.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> 
> v2 updates:
> 	commit log updated to include 'Fixes: *' and Cc: stable to aid commit
> 	being backported properly.
> 
>   drivers/pci/pci-sysfs.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 13f766db0684..667e13d597ff 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -464,7 +464,10 @@ static ssize_t dev_rescan_store(struct device *dev,
>   	}
>   	return count;
>   }
> -static DEVICE_ATTR_WO(dev_rescan);
> +static struct device_attribute dev_rescan_attr = __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)
> @@ -481,9 +484,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 bus_rescan_store(struct device *dev,
> -				struct device_attribute *attr,
> -				const char *buf, size_t count)
> +static ssize_t dev_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 +504,7 @@ static ssize_t bus_rescan_store(struct device *dev,
>   	}
>   	return count;
>   }
> -static DEVICE_ATTR_WO(bus_rescan);
> +static DEVICE_ATTR(rescan, 0220, NULL, dev_bus_rescan_store);
>   
>   #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
>   static ssize_t d3cold_allowed_store(struct device *dev,
> @@ -641,7 +644,7 @@ static struct attribute *pcie_dev_attrs[] = {
>   };
>   
>   static struct attribute *pcibus_attrs[] = {
> -	&dev_attr_bus_rescan.attr,
> +	&dev_attr_rescan.attr,
>   	&dev_attr_cpuaffinity.attr,
>   	&dev_attr_cpulistaffinity.attr,
>   	NULL,
> @@ -1487,7 +1490,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_dev_rescan.attr,
> +	&dev_rescan_attr.attr,
>   	NULL,
>   };
>   
> 

Re: [Linux-kernel-mentees] [PATCH v2] PCI: sysfs: Change bus_rescan and dev_rescan to rescan

[Bjorn Helgaas]

Hi Kelsey,

On Wed, Mar 25, 2020 at 09:17:08AM -0600, Kelsey Skunberg wrote:
> From: Kelsey Skunberg <kelsey.skunberg@gmail.com>
> 
> rename device attribute name arguments 'bus_rescan' and 'dev_rescan' to 'rescan'
> to avoid breaking userspace applications.
> 
> The attribute argument names were changed in the following commits:
> 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
> 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
> 
> Revert the names used for attributes back to the names used before the above
> patches were applied. This also requires to change DEVICE_ATTR_WO() to
> DEVICE_ATTR() and __ATTR().
> 
> Note when using DEVICE_ATTR() the attribute is automatically named
> dev_attr_<name>.attr. To avoid duplicated names between attributes, use
> __ATTR() instead of DEVICE_ATTR() to a assign a custom attribute name for
> dev_rescan.
> 
> change bus_rescan_store() to dev_bus_rescan_store() to complete matching the
> names used before the mentioned patches were applied.
> 
> Fixes: 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
> Fixes: 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
> 
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Kelsey Skunberg <kelsey.skunberg@gmail.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> 
> v2 updates: 
> 	commit log updated to include 'Fixes: *' and Cc: stable to aid commit
> 	being backported properly.
> 
>  drivers/pci/pci-sysfs.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 13f766db0684..667e13d597ff 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -464,7 +464,10 @@ static ssize_t dev_rescan_store(struct device *dev,
>  	}
>  	return count;
>  }
> -static DEVICE_ATTR_WO(dev_rescan);
> +static struct device_attribute dev_rescan_attr = __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)
> @@ -481,9 +484,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 bus_rescan_store(struct device *dev,
> -				struct device_attribute *attr,
> -				const char *buf, size_t count)
> +static ssize_t dev_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 +504,7 @@ static ssize_t bus_rescan_store(struct device *dev,
>  	}
>  	return count;
>  }
> -static DEVICE_ATTR_WO(bus_rescan);
> +static DEVICE_ATTR(rescan, 0220, NULL, dev_bus_rescan_store);
>  
>  #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
>  static ssize_t d3cold_allowed_store(struct device *dev,
> @@ -641,7 +644,7 @@ static struct attribute *pcie_dev_attrs[] = {
>  };
>  
>  static struct attribute *pcibus_attrs[] = {
> -	&dev_attr_bus_rescan.attr,
> +	&dev_attr_rescan.attr,
>  	&dev_attr_cpuaffinity.attr,
>  	&dev_attr_cpulistaffinity.attr,
>  	NULL,
> @@ -1487,7 +1490,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_dev_rescan.attr,
> +	&dev_rescan_attr.attr,
>  	NULL,
>  };

Thanks for taking care of this!  Two questions:

1) You supplied permissions of 0220, but DEVICE_ATTR_WO()
uses__ATTR_WO(), which uses 0200.  Shouldn't we keep 0200?

2) I think the use of __ATTR() for the device side and DEVICE_ATTR()
for the bus side is confusing.  Couldn't we accomplish the same thing
with a patch like the following (compiled but not tested)?

Bjorn


commit 06094b3fd9f1 ("PCI: sysfs: Revert "rescan" file renames")
Author: Kelsey Skunberg <kelsey.skunberg@gmail.com>
Date:   Wed Mar 25 09:17:08 2020 -0600

    PCI: sysfs: Revert "rescan" file renames
    
    We changed these sysfs filenames:
    
      .../pci_bus/<domain:bus>/rescan  ->  .../pci_bus/<domain:bus>/bus_rescan
      .../<domain:bus:dev.fn>/rescan   ->  .../<domain:bus:dev.fn>/dev_rescan
    
    and Ruslan reported [1] that this broke a userspace application.
    
    Revert these name changes so both files are named "rescan" again.
    
    The argument to DEVICE_ATTR_WO() determines both the struct
    device_attribute name and the .store() function name.  We have to
    use __ATTR() so we can specify different .store() functions for
    the two "rescan" files.
    
    [1] https://lore.kernel.org/r/CAB=otbSYozS-ZfxB0nCiNnxcbqxwrHOSYxJJtDKa63KzXbXgpw@mail.gmail.com
    
    [bhelgaas: commit log]
    Fixes: 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
    Fixes: 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
    Link: https://lore.kernel.org/r/20200325151708.32612-1-skunberg.kelsey@gmail.com
    Signed-off-by: Kelsey Skunberg <kelsey.skunberg@gmail.com>
    Cc: stable@vger.kernel.org	# v5.4+

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 13f766db0684..bf025a2c296d 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -464,7 +464,8 @@ static ssize_t dev_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR_WO(dev_rescan);
+static struct device_attribute dev_attr_dev_rescan = __ATTR(rescan, 0200, NULL,
+							    dev_rescan_store);
 
 static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
@@ -501,7 +502,8 @@ static ssize_t bus_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR_WO(bus_rescan);
+static struct device_attribute dev_attr_bus_rescan = __ATTR(rescan, 0200, NULL,
+							    bus_rescan_store);
 
 #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
 static ssize_t d3cold_allowed_store(struct device *dev,

Re: [Linux-kernel-mentees] [PATCH v2] PCI: sysfs: Change bus_rescan and dev_rescan to rescan

[Kelsey]

Hi Bjorn,

On Wed, Mar 25, 2020 at 4:10 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Hi Kelsey,
>
> On Wed, Mar 25, 2020 at 09:17:08AM -0600, Kelsey Skunberg wrote:
> > From: Kelsey Skunberg <kelsey.skunberg@gmail.com>
> >
> > rename device attribute name arguments 'bus_rescan' and 'dev_rescan' to 'rescan'
> > to avoid breaking userspace applications.
> >
> > The attribute argument names were changed in the following commits:
> > 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
> > 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
> >
> > Revert the names used for attributes back to the names used before the above
> > patches were applied. This also requires to change DEVICE_ATTR_WO() to
> > DEVICE_ATTR() and __ATTR().
> >
> > Note when using DEVICE_ATTR() the attribute is automatically named
> > dev_attr_<name>.attr. To avoid duplicated names between attributes, use
> > __ATTR() instead of DEVICE_ATTR() to a assign a custom attribute name for
> > dev_rescan.
> >
> > change bus_rescan_store() to dev_bus_rescan_store() to complete matching the
> > names used before the mentioned patches were applied.
> >
> > Fixes: 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
> > Fixes: 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
> >
> > Cc: stable <stable@vger.kernel.org>
> > Signed-off-by: Kelsey Skunberg <kelsey.skunberg@gmail.com>
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >
> > v2 updates:
> >       commit log updated to include 'Fixes: *' and Cc: stable to aid commit
> >       being backported properly.
> >
> >  drivers/pci/pci-sysfs.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 13f766db0684..667e13d597ff 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -464,7 +464,10 @@ static ssize_t dev_rescan_store(struct device *dev,
> >       }
> >       return count;
> >  }
> > -static DEVICE_ATTR_WO(dev_rescan);
> > +static struct device_attribute dev_rescan_attr = __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)
> > @@ -481,9 +484,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 bus_rescan_store(struct device *dev,
> > -                             struct device_attribute *attr,
> > -                             const char *buf, size_t count)
> > +static ssize_t dev_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 +504,7 @@ static ssize_t bus_rescan_store(struct device *dev,
> >       }
> >       return count;
> >  }
> > -static DEVICE_ATTR_WO(bus_rescan);
> > +static DEVICE_ATTR(rescan, 0220, NULL, dev_bus_rescan_store);
> >
> >  #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
> >  static ssize_t d3cold_allowed_store(struct device *dev,
> > @@ -641,7 +644,7 @@ static struct attribute *pcie_dev_attrs[] = {
> >  };
> >
> >  static struct attribute *pcibus_attrs[] = {
> > -     &dev_attr_bus_rescan.attr,
> > +     &dev_attr_rescan.attr,
> >       &dev_attr_cpuaffinity.attr,
> >       &dev_attr_cpulistaffinity.attr,
> >       NULL,
> > @@ -1487,7 +1490,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_dev_rescan.attr,
> > +     &dev_rescan_attr.attr,
> >       NULL,
> >  };
>
> Thanks for taking care of this!  Two questions:
>
> 1) You supplied permissions of 0220, but DEVICE_ATTR_WO()
> uses__ATTR_WO(), which uses 0200.  Shouldn't we keep 0200?
>

Good catch. Before changing to DEVICE_ATTR_WO(), the permissions used
was (S_IWUSR | S_IWGRP), which would be 0220. This means the
permissions were mistakenly changed from 0220 to 0200 in the same
patch:

commit 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")

To verify DEVICE_ATTR_WO() is using __ATTR_WO() can be seen in
/include/linux/device.h
To verify permissions for __ATTR_WO() is 0200 can be seen in
/inlcude/linux/sysfs.h

These attributes had permissions 0220 when first being introduced and
before the above mentioned patch, so I'm on the side to believe that
0220 should be used.

The commits that introduced the files:
commit 738a6396c223 ("PCI: Introduce /sys/bus/pci/devices/.../rescan"):
commit b9d320fcb625 ("PCI: add rescan to /sys/.../pci_bus/.../"):

May you please sanity check this logic to use 0220? I can create a
second patch to change the permissions from the currently used 0200 to
0220.

> 2) I think the use of __ATTR() for the device side and DEVICE_ATTR()
> for the bus side is confusing.  Couldn't we accomplish the same thing
> with a patch like the following (compiled but not tested)?
>
> Bjorn
>
>
> commit 06094b3fd9f1 ("PCI: sysfs: Revert "rescan" file renames")
> Author: Kelsey Skunberg <kelsey.skunberg@gmail.com>
> Date:   Wed Mar 25 09:17:08 2020 -0600
>
>     PCI: sysfs: Revert "rescan" file renames
>
>     We changed these sysfs filenames:
>
>       .../pci_bus/<domain:bus>/rescan  ->  .../pci_bus/<domain:bus>/bus_rescan
>       .../<domain:bus:dev.fn>/rescan   ->  .../<domain:bus:dev.fn>/dev_rescan
>
>     and Ruslan reported [1] that this broke a userspace application.
>
>     Revert these name changes so both files are named "rescan" again.
>
>     The argument to DEVICE_ATTR_WO() determines both the struct
>     device_attribute name and the .store() function name.  We have to
>     use __ATTR() so we can specify different .store() functions for
>     the two "rescan" files.
>
>     [1] https://lore.kernel.org/r/CAB=otbSYozS-ZfxB0nCiNnxcbqxwrHOSYxJJtDKa63KzXbXgpw@mail.gmail.com
>
>     [bhelgaas: commit log]
>     Fixes: 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
>     Fixes: 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
>     Link: https://lore.kernel.org/r/20200325151708.32612-1-skunberg.kelsey@gmail.com
>     Signed-off-by: Kelsey Skunberg <kelsey.skunberg@gmail.com>
>     Cc: stable@vger.kernel.org  # v5.4+
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 13f766db0684..bf025a2c296d 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -464,7 +464,8 @@ static ssize_t dev_rescan_store(struct device *dev,
>         }
>         return count;
>  }
> -static DEVICE_ATTR_WO(dev_rescan);
> +static struct device_attribute dev_attr_dev_rescan = __ATTR(rescan, 0200, NULL,
> +                                                           dev_rescan_store);
>
>  static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
>                             const char *buf, size_t count)
> @@ -501,7 +502,8 @@ static ssize_t bus_rescan_store(struct device *dev,
>         }
>         return count;
>  }
> -static DEVICE_ATTR_WO(bus_rescan);
> +static struct device_attribute dev_attr_bus_rescan = __ATTR(rescan, 0200, NULL,
> +                                                           bus_rescan_store);
>
>  #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
>  static ssize_t d3cold_allowed_store(struct device *dev,


This can be done, yes! I agree this could help clarify and clean up the code.

I can implement your commit log as well.

Let me know your thoughts for the permissions and I'll get an updated
version out right away.

Thank you!

-Kelsey

Re: [Linux-kernel-mentees] [PATCH v2] PCI: sysfs: Change bus_rescan and dev_rescan to rescan

[Bjorn Helgaas]

On Thu, Mar 26, 2020 at 12:29:11AM -0600, Kelsey wrote:
> On Wed, Mar 25, 2020 at 4:10 PM Bjorn Helgaas <helgaas@kernel.org> wrote:

> > Thanks for taking care of this!  Two questions:
> >
> > 1) You supplied permissions of 0220, but DEVICE_ATTR_WO()
> > uses__ATTR_WO(), which uses 0200.  Shouldn't we keep 0200?
> >
> 
> Good catch. Before changing to DEVICE_ATTR_WO(), the permissions used
> was (S_IWUSR | S_IWGRP), which would be 0220. This means the
> permissions were mistakenly changed from 0220 to 0200 in the same
> patch:
> 
> commit 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
> 
> To verify DEVICE_ATTR_WO() is using __ATTR_WO() can be seen in
> /include/linux/device.h
> To verify permissions for __ATTR_WO() is 0200 can be seen in
> /inlcude/linux/sysfs.h
> 
> These attributes had permissions 0220 when first being introduced and
> before the above mentioned patch, so I'm on the side to believe that
> 0220 should be used.

I'm not sure it was a mistake that 4e2b79436e4f changed from 0220 to
200 or not.  I'd say __ATTR_WO (0200) is the "standard" one, and we
should have a special reason to use 0220.

Re: [Linux-kernel-mentees] [PATCH v2] PCI: sysfs: Change bus_rescan and dev_rescan to rescan

[Kelsey]

On Sat, Mar 28, 2020 at 1:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Mar 26, 2020 at 12:29:11AM -0600, Kelsey wrote:
> > On Wed, Mar 25, 2020 at 4:10 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> > > Thanks for taking care of this!  Two questions:
> > >
> > > 1) You supplied permissions of 0220, but DEVICE_ATTR_WO()
> > > uses__ATTR_WO(), which uses 0200.  Shouldn't we keep 0200?
> > >
> >
> > Good catch. Before changing to DEVICE_ATTR_WO(), the permissions used
> > was (S_IWUSR | S_IWGRP), which would be 0220. This means the
> > permissions were mistakenly changed from 0220 to 0200 in the same
> > patch:
> >
> > commit 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
> >
> > To verify DEVICE_ATTR_WO() is using __ATTR_WO() can be seen in
> > /include/linux/device.h
> > To verify permissions for __ATTR_WO() is 0200 can be seen in
> > /inlcude/linux/sysfs.h
> >
> > These attributes had permissions 0220 when first being introduced and
> > before the above mentioned patch, so I'm on the side to believe that
> > 0220 should be used.
>
> I'm not sure it was a mistake that 4e2b79436e4f changed from 0220 to
> 200 or not.  I'd say __ATTR_WO (0200) is the "standard" one, and we
> should have a special reason to use 0220.

Sounds good. I didn't find any information or reason stating the
permissions needed to be 0220. So sounds like 0200 will be the winner.

Appreciate the help and you checking this over!

- Kelsey

Re: [Linux-kernel-mentees] [PATCH v2] PCI: sysfs: Change bus_rescan and dev_rescan to rescan

[Don Dutile]

On 3/28/20 3:59 PM, Bjorn Helgaas wrote:
> On Thu, Mar 26, 2020 at 12:29:11AM -0600, Kelsey wrote:
>> On Wed, Mar 25, 2020 at 4:10 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
>>> Thanks for taking care of this!  Two questions:
>>>
>>> 1) You supplied permissions of 0220, but DEVICE_ATTR_WO()
>>> uses__ATTR_WO(), which uses 0200.  Shouldn't we keep 0200?
>>>
>>
>> Good catch. Before changing to DEVICE_ATTR_WO(), the permissions used
>> was (S_IWUSR | S_IWGRP), which would be 0220. This means the
>> permissions were mistakenly changed from 0220 to 0200 in the same
>> patch:
>>
>> commit 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
>>
>> To verify DEVICE_ATTR_WO() is using __ATTR_WO() can be seen in
>> /include/linux/device.h
>> To verify permissions for __ATTR_WO() is 0200 can be seen in
>> /inlcude/linux/sysfs.h
>>
>> These attributes had permissions 0220 when first being introduced and
>> before the above mentioned patch, so I'm on the side to believe that
>> 0220 should be used.
> 
> I'm not sure it was a mistake that 4e2b79436e4f changed from 0220 to
> 200 or not.  I'd say __ATTR_WO (0200) is the "standard" one, and we
> should have a special reason to use 0220.
> 
Bjorn,
Thanks for verifying the 0200 vs 0220 permissions.
I had recalled that discussion thread on the permissions when the original ATTR patch was proposed, but hadn't had time to dig it up.
Apologies for the delay, thanks for the (final?) cleanup.
- Don

Re: [Linux-kernel-mentees] [PATCH v2] PCI: sysfs: Change bus_rescan and dev_rescan to rescan

[Greg KH]

On Wed, Mar 25, 2020 at 05:10:33PM -0500, Bjorn Helgaas wrote:
> Hi Kelsey,
> 
> On Wed, Mar 25, 2020 at 09:17:08AM -0600, Kelsey Skunberg wrote:
> > From: Kelsey Skunberg <kelsey.skunberg@gmail.com>
> > 
> > rename device attribute name arguments 'bus_rescan' and 'dev_rescan' to 'rescan'
> > to avoid breaking userspace applications.
> > 
> > The attribute argument names were changed in the following commits:
> > 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
> > 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
> > 
> > Revert the names used for attributes back to the names used before the above
> > patches were applied. This also requires to change DEVICE_ATTR_WO() to
> > DEVICE_ATTR() and __ATTR().
> > 
> > Note when using DEVICE_ATTR() the attribute is automatically named
> > dev_attr_<name>.attr. To avoid duplicated names between attributes, use
> > __ATTR() instead of DEVICE_ATTR() to a assign a custom attribute name for
> > dev_rescan.
> > 
> > change bus_rescan_store() to dev_bus_rescan_store() to complete matching the
> > names used before the mentioned patches were applied.
> > 
> > Fixes: 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
> > Fixes: 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
> > 
> > Cc: stable <stable@vger.kernel.org>
> > Signed-off-by: Kelsey Skunberg <kelsey.skunberg@gmail.com>
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > 
> > v2 updates: 
> > 	commit log updated to include 'Fixes: *' and Cc: stable to aid commit
> > 	being backported properly.
> > 
> >  drivers/pci/pci-sysfs.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 13f766db0684..667e13d597ff 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -464,7 +464,10 @@ static ssize_t dev_rescan_store(struct device *dev,
> >  	}
> >  	return count;
> >  }
> > -static DEVICE_ATTR_WO(dev_rescan);
> > +static struct device_attribute dev_rescan_attr = __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)
> > @@ -481,9 +484,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 bus_rescan_store(struct device *dev,
> > -				struct device_attribute *attr,
> > -				const char *buf, size_t count)
> > +static ssize_t dev_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 +504,7 @@ static ssize_t bus_rescan_store(struct device *dev,
> >  	}
> >  	return count;
> >  }
> > -static DEVICE_ATTR_WO(bus_rescan);
> > +static DEVICE_ATTR(rescan, 0220, NULL, dev_bus_rescan_store);
> >  
> >  #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
> >  static ssize_t d3cold_allowed_store(struct device *dev,
> > @@ -641,7 +644,7 @@ static struct attribute *pcie_dev_attrs[] = {
> >  };
> >  
> >  static struct attribute *pcibus_attrs[] = {
> > -	&dev_attr_bus_rescan.attr,
> > +	&dev_attr_rescan.attr,
> >  	&dev_attr_cpuaffinity.attr,
> >  	&dev_attr_cpulistaffinity.attr,
> >  	NULL,
> > @@ -1487,7 +1490,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_dev_rescan.attr,
> > +	&dev_rescan_attr.attr,
> >  	NULL,
> >  };
> 
> Thanks for taking care of this!  Two questions:
> 
> 1) You supplied permissions of 0220, but DEVICE_ATTR_WO()
> uses__ATTR_WO(), which uses 0200.  Shouldn't we keep 0200?
> 
> 2) I think the use of __ATTR() for the device side and DEVICE_ATTR()
> for the bus side is confusing.  Couldn't we accomplish the same thing
> with a patch like the following (compiled but not tested)?
> 
> Bjorn
> 
> 
> commit 06094b3fd9f1 ("PCI: sysfs: Revert "rescan" file renames")
> Author: Kelsey Skunberg <kelsey.skunberg@gmail.com>
> Date:   Wed Mar 25 09:17:08 2020 -0600
> 
>     PCI: sysfs: Revert "rescan" file renames
>     
>     We changed these sysfs filenames:
>     
>       .../pci_bus/<domain:bus>/rescan  ->  .../pci_bus/<domain:bus>/bus_rescan
>       .../<domain:bus:dev.fn>/rescan   ->  .../<domain:bus:dev.fn>/dev_rescan
>     
>     and Ruslan reported [1] that this broke a userspace application.
>     
>     Revert these name changes so both files are named "rescan" again.
>     
>     The argument to DEVICE_ATTR_WO() determines both the struct
>     device_attribute name and the .store() function name.  We have to
>     use __ATTR() so we can specify different .store() functions for
>     the two "rescan" files.
>     
>     [1] https://lore.kernel.org/r/CAB=otbSYozS-ZfxB0nCiNnxcbqxwrHOSYxJJtDKa63KzXbXgpw@mail.gmail.com
>     
>     [bhelgaas: commit log]
>     Fixes: 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
>     Fixes: 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
>     Link: https://lore.kernel.org/r/20200325151708.32612-1-skunberg.kelsey@gmail.com
>     Signed-off-by: Kelsey Skunberg <kelsey.skunberg@gmail.com>
>     Cc: stable@vger.kernel.org	# v5.4+
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 13f766db0684..bf025a2c296d 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -464,7 +464,8 @@ static ssize_t dev_rescan_store(struct device *dev,
>  	}
>  	return count;
>  }
> -static DEVICE_ATTR_WO(dev_rescan);
> +static struct device_attribute dev_attr_dev_rescan = __ATTR(rescan, 0200, NULL,
> +							    dev_rescan_store);

Oops, this should just be DEVICE_ATTR(), no need for __ATTR() as this
isn't a kobject-only file.

So how about:

static DEVICE_ATTR(rescan, 0200, NULL, dev_rescan_store);

Much smaller :)

Sorry I missed that in the original review.

thanks,

greg k-h

Re: [Linux-kernel-mentees] [PATCH v2] PCI: sysfs: Change bus_rescan and dev_rescan to rescan

[Bjorn Helgaas]

On Thu, Mar 26, 2020 at 07:35:24AM +0100, Greg KH wrote:
> On Wed, Mar 25, 2020 at 05:10:33PM -0500, Bjorn Helgaas wrote:
> > -static DEVICE_ATTR_WO(dev_rescan);
> > +static struct device_attribute dev_attr_dev_rescan = __ATTR(rescan, 0200, NULL,
> > +							    dev_rescan_store);
> 
> Oops, this should just be DEVICE_ATTR(), no need for __ATTR() as this
> isn't a kobject-only file.
> 
> So how about:
> 
> static DEVICE_ATTR(rescan, 0200, NULL, dev_rescan_store);

I don' think DEVICE_ATTR() works in this case because it uses the
first argument ("rescan") to build both the C symbol for the
device_attribute struct and the sysfs filename.

There are two instances in this file.  The two sysfs "rescan" files
are not a problem, but the two "dev_attr_rescan_name" C symbols *are*.
We could resolve that by putting the bus attributes in a different
source file than the dev attributes, but it doesn't seem worth it now.

I tentatively have the patch below on pci/misc.  I dropped the
tested-by and reviewed-by because I didn't want to put words in your
mouths :)

Bjorn

commit bce34ce1806e ("PCI: sysfs: Revert "rescan" file renames")
Author: Kelsey Skunberg <kelsey.skunberg@gmail.com>
Date:   Wed Mar 25 09:17:08 2020 -0600

    PCI: sysfs: Revert "rescan" file renames
    
    We changed these sysfs filenames:
    
      .../pci_bus/<domain:bus>/rescan  ->  .../pci_bus/<domain:bus>/bus_rescan
      .../<domain:bus:dev.fn>/rescan   ->  .../<domain:bus:dev.fn>/dev_rescan
    
    and Ruslan reported [1] that this broke a userspace application.
    
    Revert these name changes so both files are named "rescan" again.
    
    Note that we have to use __ATTR() to assign custom C symbols, i.e.,
    "struct device_attribute <symbol>".
    
    [1] https://lore.kernel.org/r/CAB=otbSYozS-ZfxB0nCiNnxcbqxwrHOSYxJJtDKa63KzXbXgpw@mail.gmail.com
    
    [bhelgaas: commit log, use __ATTR() both places so we don't have to rename
    the attributes]
    Fixes: 8bdfa145f582 ("PCI: sysfs: Define device attributes with DEVICE_ATTR*()")
    Fixes: 4e2b79436e4f ("PCI: sysfs: Change DEVICE_ATTR() to DEVICE_ATTR_WO()")
    Link: https://lore.kernel.org/r/20200325151708.32612-1-skunberg.kelsey@gmail.com
    Signed-off-by: Kelsey Skunberg <kelsey.skunberg@gmail.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Cc: stable@vger.kernel.org	# v5.4+

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 13f766db0684..335dd6fbf039 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -464,7 +464,8 @@ static ssize_t dev_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR_WO(dev_rescan);
+static struct device_attribute dev_attr_dev_rescan = __ATTR(rescan, 0200, NULL,
+							    dev_rescan_store);
 
 static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)
@@ -501,7 +502,8 @@ static ssize_t bus_rescan_store(struct device *dev,
 	}
 	return count;
 }
-static DEVICE_ATTR_WO(bus_rescan);
+static struct device_attribute dev_attr_bus_rescan = __ATTR(rescan, 0200, NULL,
+							    bus_rescan_store);
 
 #if defined(CONFIG_PM) && defined(CONFIG_ACPI)
 static ssize_t d3cold_allowed_store(struct device *dev,

Re: [Linux-kernel-mentees] [PATCH v2] PCI: sysfs: Change bus_rescan and dev_rescan to rescan

[Greg KH]

On Sat, Mar 28, 2020 at 03:06:33PM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 26, 2020 at 07:35:24AM +0100, Greg KH wrote:
> > On Wed, Mar 25, 2020 at 05:10:33PM -0500, Bjorn Helgaas wrote:
> > > -static DEVICE_ATTR_WO(dev_rescan);
> > > +static struct device_attribute dev_attr_dev_rescan = __ATTR(rescan, 0200, NULL,
> > > +							    dev_rescan_store);
> > 
> > Oops, this should just be DEVICE_ATTR(), no need for __ATTR() as this
> > isn't a kobject-only file.
> > 
> > So how about:
> > 
> > static DEVICE_ATTR(rescan, 0200, NULL, dev_rescan_store);
> 
> I don' think DEVICE_ATTR() works in this case because it uses the
> first argument ("rescan") to build both the C symbol for the
> device_attribute struct and the sysfs filename.
> 
> There are two instances in this file.  The two sysfs "rescan" files
> are not a problem, but the two "dev_attr_rescan_name" C symbols *are*.
> We could resolve that by putting the bus attributes in a different
> source file than the dev attributes, but it doesn't seem worth it now.
> 
> I tentatively have the patch below on pci/misc.  I dropped the
> tested-by and reviewed-by because I didn't want to put words in your
> mouths :)

Ah, yeah, you are right __ATTR() is what you need to use here.

Your patch looks good:

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