pcibios_add_platform_entries usage

pcibios_add_platform_entries usage

[Sebastian Ott]

Hello Bjorn,

for pci on s390 we currently use pcibios_add_platform_entries to add
some arch specific attributes to pdevs. This has 2 downsides - it will
race with userspace which is triggered by udev events and expecting
these attributes (but that's a theoretical issue). More important to
me is that one cannot use attribute_groups with this. Both issues could
be addressed by using pdev->dev.groups and let the driver core handle
attribute creation.

So would it be ok if we set pdev->dev.groups in pcibios_add_device?
(It should be since it's not used by pci common code which uses bus_type,
dev_type, and class groups).

Regards,
Sebastian

[Resend] pcibios_add_platform_entries usage

[Sebastian Ott]

Hello Bjorn,

for pci on s390 we currently use pcibios_add_platform_entries to add
some arch specific attributes to pdevs. This has 2 downsides - it will
race with userspace which is triggered by udev events and expecting
these attributes (but that's a theoretical issue). More important to
me is that one cannot use attribute_groups with this. Both issues could
be addressed by using pdev->dev.groups and let the driver core handle
attribute creation.

So would it be ok if we set pdev->dev.groups in pcibios_add_device?
(It should be since it's not used by pci common code which uses bus_type,
dev_type, and class groups).

Regards,
Sebastian

Re: [Resend] pcibios_add_platform_entries usage

[Bjorn Helgaas]

[+cc Greg]

On Mon, Apr 14, 2014 at 11:03:42AM +0200, Sebastian Ott wrote:
> Hello Bjorn,
> 
> for pci on s390 we currently use pcibios_add_platform_entries to add
> some arch specific attributes to pdevs. This has 2 downsides - it will
> race with userspace which is triggered by udev events and expecting
> these attributes (but that's a theoretical issue). More important to
> me is that one cannot use attribute_groups with this. Both issues could
> be addressed by using pdev->dev.groups and let the driver core handle
> attribute creation.
> 
> So would it be ok if we set pdev->dev.groups in pcibios_add_device?
> (It should be since it's not used by pci common code which uses bus_type,
> dev_type, and class groups).

Hi Sebastian,

Sorry, I meant to respond to this earlier, but forgot.  This sounds
reasonable to me, but Greg can give you a much better answer than I can.

Documentation/driver-model/device.txt says the dev->groups pointer
should be set before calling device_register().  PCI calls
device_initialize() and device_add() instead of using device_register(),
and pcibios_add_device() looks like it happens at the right time:

    pci_scan_root_bus
      pci_scan_child_bus
        pci_scan_slot
          pci_scan_single_device
            pci_device_add
              device_initialize
              pcibios_add_device                # <---
              device_add
      pci_bus_add_devices
        pci_bus_add_device
          pci_create_sysfs_dev_files
            pcibios_add_platform_entries        # 8d4cd0833107 (benh)
          device_attach

I'm not sure why pci_create_sysfs_dev_files() is done later.  It seems
like that should be done before device_add() as well.  Maybe it's
because BARs might not be valid yet (that doesn't seem like a very good
excuse, but it's all I can think of).

I assume that if you change s390, you'll also change microblaze and
powerpc?  They look structurally similar to s390.

Bjorn

Re: [Resend] pcibios_add_platform_entries usage

[Sebastian Ott]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2243 bytes --]

Hello Bjorn,

On Mon, 14 Apr 2014, Bjorn Helgaas wrote:
> [+cc Greg]
> 
> On Mon, Apr 14, 2014 at 11:03:42AM +0200, Sebastian Ott wrote:
> > Hello Bjorn,
> > 
> > for pci on s390 we currently use pcibios_add_platform_entries to add
> > some arch specific attributes to pdevs. This has 2 downsides - it will
> > race with userspace which is triggered by udev events and expecting
> > these attributes (but that's a theoretical issue). More important to
> > me is that one cannot use attribute_groups with this. Both issues could
> > be addressed by using pdev->dev.groups and let the driver core handle
> > attribute creation.
> > 
> > So would it be ok if we set pdev->dev.groups in pcibios_add_device?
> > (It should be since it's not used by pci common code which uses bus_type,
> > dev_type, and class groups).
> 
> Hi Sebastian,
> 
> Sorry, I meant to respond to this earlier, but forgot.  This sounds
> reasonable to me, but Greg can give you a much better answer than I can.
> 
> Documentation/driver-model/device.txt says the dev->groups pointer
> should be set before calling device_register().  PCI calls
> device_initialize() and device_add() instead of using device_register(),
> and pcibios_add_device() looks like it happens at the right time:
> 
>     pci_scan_root_bus
>       pci_scan_child_bus
>         pci_scan_slot
>           pci_scan_single_device
>             pci_device_add
>               device_initialize
>               pcibios_add_device                # <---
>               device_add
>       pci_bus_add_devices
>         pci_bus_add_device
>           pci_create_sysfs_dev_files
>             pcibios_add_platform_entries        # 8d4cd0833107 (benh)
>           device_attach
> 
> I'm not sure why pci_create_sysfs_dev_files() is done later.  It seems
> like that should be done before device_add() as well.  Maybe it's
> because BARs might not be valid yet (that doesn't seem like a very good
> excuse, but it's all I can think of).
> 
> I assume that if you change s390, you'll also change microblaze and
> powerpc?  They look structurally similar to s390.

Yes, that sounds like a plan - this way we can get rid of
pcibios_add_platform_entries altogether. I'll send these patches soon.

Regards,
Sebastian

Re: [Resend] pcibios_add_platform_entries usage

[Sebastian Ott]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5615 bytes --]

On Mon, 14 Apr 2014, Sebastian Ott wrote:
> On Mon, 14 Apr 2014, Bjorn Helgaas wrote:
> > On Mon, Apr 14, 2014 at 11:03:42AM +0200, Sebastian Ott wrote:
> > > for pci on s390 we currently use pcibios_add_platform_entries to add
> > > some arch specific attributes to pdevs. This has 2 downsides - it will
> > > race with userspace which is triggered by udev events and expecting
> > > these attributes (but that's a theoretical issue). More important to
> > > me is that one cannot use attribute_groups with this. Both issues could
> > > be addressed by using pdev->dev.groups and let the driver core handle
> > > attribute creation.
> > > 
> > > So would it be ok if we set pdev->dev.groups in pcibios_add_device?
> > > (It should be since it's not used by pci common code which uses bus_type,
> > > dev_type, and class groups).
> > 
> > Hi Sebastian,
> > 
> > Sorry, I meant to respond to this earlier, but forgot.  This sounds
> > reasonable to me, but Greg can give you a much better answer than I can.
> > 
> > Documentation/driver-model/device.txt says the dev->groups pointer
> > should be set before calling device_register().  PCI calls
> > device_initialize() and device_add() instead of using device_register(),
> > and pcibios_add_device() looks like it happens at the right time:
> > 
> >     pci_scan_root_bus
> >       pci_scan_child_bus
> >         pci_scan_slot
> >           pci_scan_single_device
> >             pci_device_add
> >               device_initialize
> >               pcibios_add_device                # <---
> >               device_add
> >       pci_bus_add_devices
> >         pci_bus_add_device
> >           pci_create_sysfs_dev_files
> >             pcibios_add_platform_entries        # 8d4cd0833107 (benh)
> >           device_attach
> > 
> > I'm not sure why pci_create_sysfs_dev_files() is done later.  It seems
> > like that should be done before device_add() as well.  Maybe it's
> > because BARs might not be valid yet (that doesn't seem like a very good
> > excuse, but it's all I can think of).
> > 
> > I assume that if you change s390, you'll also change microblaze and
> > powerpc?  They look structurally similar to s390.
> 
> Yes, that sounds like a plan - this way we can get rid of
> pcibios_add_platform_entries altogether. I'll send these patches soon.

Hm, pcibios_add_platform_entries for microblaze and power is identical
and this OF stuff seems not to be arch specific. How about the following
patch?


pci: move open fabric devspec attribute to pci common code

Move the devspec OF attribute to pci common code's set of device
attributes since it's not architecture dependent.
As a side effect microblaze and powerpc no longer need to use
pcibios_add_platform_entries.

Link: https://lkml.kernel.org/r/alpine.LFD.2.11.1404141101500.1529@denkbrett
Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
---
 arch/microblaze/pci/pci-common.c |   20 --------------------
 arch/powerpc/kernel/pci-common.c |   20 --------------------
 drivers/pci/pci-sysfs.c          |   17 +++++++++++++++++
 3 files changed, 17 insertions(+), 40 deletions(-)

--- a/arch/microblaze/pci/pci-common.c
+++ b/arch/microblaze/pci/pci-common.c
@@ -168,26 +168,6 @@ struct pci_controller *pci_find_hose_for
 	return NULL;
 }
 
-static ssize_t pci_show_devspec(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct pci_dev *pdev;
-	struct device_node *np;
-
-	pdev = to_pci_dev(dev);
-	np = pci_device_to_OF_node(pdev);
-	if (np == NULL || np->full_name == NULL)
-		return 0;
-	return sprintf(buf, "%s", np->full_name);
-}
-static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
-
-/* Add sysfs properties */
-int pcibios_add_platform_entries(struct pci_dev *pdev)
-{
-	return device_create_file(&pdev->dev, &dev_attr_devspec);
-}
-
 void pcibios_set_master(struct pci_dev *dev)
 {
 	/* No special bus mastering setup handling */
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -201,26 +201,6 @@ struct pci_controller* pci_find_hose_for
 	return NULL;
 }
 
-static ssize_t pci_show_devspec(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct pci_dev *pdev;
-	struct device_node *np;
-
-	pdev = to_pci_dev (dev);
-	np = pci_device_to_OF_node(pdev);
-	if (np == NULL || np->full_name == NULL)
-		return 0;
-	return sprintf(buf, "%s", np->full_name);
-}
-static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
-
-/* Add sysfs properties */
-int pcibios_add_platform_entries(struct pci_dev *pdev)
-{
-	return device_create_file(&pdev->dev, &dev_attr_devspec);
-}
-
 /*
  * Reads the interrupt pin to determine if interrupt is use by card.
  * If the interrupt is used, then gets the interrupt line from the
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -416,6 +416,20 @@ static ssize_t d3cold_allowed_show(struc
 static DEVICE_ATTR_RW(d3cold_allowed);
 #endif
 
+#ifdef CONFIG_OF
+static ssize_t devspec_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct device_node *np = pci_device_to_OF_node(pdev);
+
+	if (np == NULL || np->full_name == NULL)
+		return 0;
+	return sprintf(buf, "%s", np->full_name);
+}
+static DEVICE_ATTR_RO(devspec);
+#endif
+
 #ifdef CONFIG_PCI_IOV
 static ssize_t sriov_totalvfs_show(struct device *dev,
 				   struct device_attribute *attr,
@@ -521,6 +535,9 @@ static struct attribute *pci_dev_attrs[]
 #if defined(CONFIG_PM_RUNTIME) && defined(CONFIG_ACPI)
 	&dev_attr_d3cold_allowed.attr,
 #endif
+#ifdef CONFIG_OF
+	&dev_attr_devspec.attr,
+#endif
 	NULL,
 };
 

Re: [Resend] pcibios_add_platform_entries usage

[Benjamin Herrenschmidt]

On Thu, 2014-04-17 at 19:46 +0200, Sebastian Ott wrote:

> pci: move open fabric devspec attribute to pci common code
> 
> Move the devspec OF attribute to pci common code's set of device
> attributes since it's not architecture dependent.
> As a side effect microblaze and powerpc no longer need to use
> pcibios_add_platform_entries.
> 
> Link: https://lkml.kernel.org/r/alpine.LFD.2.11.1404141101500.1529@denkbrett
> Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

(Pending you've compile tested it on powerpc, I'm on vacation and
haven't done it :)

> ---
>  arch/microblaze/pci/pci-common.c |   20 --------------------
>  arch/powerpc/kernel/pci-common.c |   20 --------------------
>  drivers/pci/pci-sysfs.c          |   17 +++++++++++++++++
>  3 files changed, 17 insertions(+), 40 deletions(-)
> 
> --- a/arch/microblaze/pci/pci-common.c
> +++ b/arch/microblaze/pci/pci-common.c
> @@ -168,26 +168,6 @@ struct pci_controller *pci_find_hose_for
>  	return NULL;
>  }
>  
> -static ssize_t pci_show_devspec(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct pci_dev *pdev;
> -	struct device_node *np;
> -
> -	pdev = to_pci_dev(dev);
> -	np = pci_device_to_OF_node(pdev);
> -	if (np == NULL || np->full_name == NULL)
> -		return 0;
> -	return sprintf(buf, "%s", np->full_name);
> -}
> -static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> -
> -/* Add sysfs properties */
> -int pcibios_add_platform_entries(struct pci_dev *pdev)
> -{
> -	return device_create_file(&pdev->dev, &dev_attr_devspec);
> -}
> -
>  void pcibios_set_master(struct pci_dev *dev)
>  {
>  	/* No special bus mastering setup handling */
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -201,26 +201,6 @@ struct pci_controller* pci_find_hose_for
>  	return NULL;
>  }
>  
> -static ssize_t pci_show_devspec(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct pci_dev *pdev;
> -	struct device_node *np;
> -
> -	pdev = to_pci_dev (dev);
> -	np = pci_device_to_OF_node(pdev);
> -	if (np == NULL || np->full_name == NULL)
> -		return 0;
> -	return sprintf(buf, "%s", np->full_name);
> -}
> -static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> -
> -/* Add sysfs properties */
> -int pcibios_add_platform_entries(struct pci_dev *pdev)
> -{
> -	return device_create_file(&pdev->dev, &dev_attr_devspec);
> -}
> -
>  /*
>   * Reads the interrupt pin to determine if interrupt is use by card.
>   * If the interrupt is used, then gets the interrupt line from the
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -416,6 +416,20 @@ static ssize_t d3cold_allowed_show(struc
>  static DEVICE_ATTR_RW(d3cold_allowed);
>  #endif
>  
> +#ifdef CONFIG_OF
> +static ssize_t devspec_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct device_node *np = pci_device_to_OF_node(pdev);
> +
> +	if (np == NULL || np->full_name == NULL)
> +		return 0;
> +	return sprintf(buf, "%s", np->full_name);
> +}
> +static DEVICE_ATTR_RO(devspec);
> +#endif
> +
>  #ifdef CONFIG_PCI_IOV
>  static ssize_t sriov_totalvfs_show(struct device *dev,
>  				   struct device_attribute *attr,
> @@ -521,6 +535,9 @@ static struct attribute *pci_dev_attrs[]
>  #if defined(CONFIG_PM_RUNTIME) && defined(CONFIG_ACPI)
>  	&dev_attr_d3cold_allowed.attr,
>  #endif
> +#ifdef CONFIG_OF
> +	&dev_attr_devspec.attr,
> +#endif
>  	NULL,
>  };
>  

Re: [Resend] pcibios_add_platform_entries usage

[Sebastian Ott]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4510 bytes --]

On Fri, 18 Apr 2014, Benjamin Herrenschmidt wrote:
> On Thu, 2014-04-17 at 19:46 +0200, Sebastian Ott wrote:
> 
> > pci: move open fabric devspec attribute to pci common code
> > 
> > Move the devspec OF attribute to pci common code's set of device
> > attributes since it's not architecture dependent.
> > As a side effect microblaze and powerpc no longer need to use
> > pcibios_add_platform_entries.
> > 
> > Link: https://lkml.kernel.org/r/alpine.LFD.2.11.1404141101500.1529@denkbrett
> > Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
> 
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> (Pending you've compile tested it on powerpc, I'm on vacation and
> haven't done it :)

I did:
make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2- g5_defconfig
make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2-

and failed with:
  CC      arch/powerpc/lib/xor_vmx.o
In file included from include/linux/list.h:4,
                 from include/linux/preempt.h:10,
                 from arch/powerpc/lib/xor_vmx.c:22:
include/linux/types.h:29: error: both ‘unsigned’ and ‘_Bool’ in declaration specifiers
cc1: warnings being treated as errors
include/linux/types.h:29: error: useless type name in empty declaration



make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2- arch/powerpc/kernel/ drivers/pci/

worked just fine.

Regards,
Sebastian

> 
> > ---
> >  arch/microblaze/pci/pci-common.c |   20 --------------------
> >  arch/powerpc/kernel/pci-common.c |   20 --------------------
> >  drivers/pci/pci-sysfs.c          |   17 +++++++++++++++++
> >  3 files changed, 17 insertions(+), 40 deletions(-)
> > 
> > --- a/arch/microblaze/pci/pci-common.c
> > +++ b/arch/microblaze/pci/pci-common.c
> > @@ -168,26 +168,6 @@ struct pci_controller *pci_find_hose_for
> >  	return NULL;
> >  }
> >  
> > -static ssize_t pci_show_devspec(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct pci_dev *pdev;
> > -	struct device_node *np;
> > -
> > -	pdev = to_pci_dev(dev);
> > -	np = pci_device_to_OF_node(pdev);
> > -	if (np == NULL || np->full_name == NULL)
> > -		return 0;
> > -	return sprintf(buf, "%s", np->full_name);
> > -}
> > -static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> > -
> > -/* Add sysfs properties */
> > -int pcibios_add_platform_entries(struct pci_dev *pdev)
> > -{
> > -	return device_create_file(&pdev->dev, &dev_attr_devspec);
> > -}
> > -
> >  void pcibios_set_master(struct pci_dev *dev)
> >  {
> >  	/* No special bus mastering setup handling */
> > --- a/arch/powerpc/kernel/pci-common.c
> > +++ b/arch/powerpc/kernel/pci-common.c
> > @@ -201,26 +201,6 @@ struct pci_controller* pci_find_hose_for
> >  	return NULL;
> >  }
> >  
> > -static ssize_t pci_show_devspec(struct device *dev,
> > -		struct device_attribute *attr, char *buf)
> > -{
> > -	struct pci_dev *pdev;
> > -	struct device_node *np;
> > -
> > -	pdev = to_pci_dev (dev);
> > -	np = pci_device_to_OF_node(pdev);
> > -	if (np == NULL || np->full_name == NULL)
> > -		return 0;
> > -	return sprintf(buf, "%s", np->full_name);
> > -}
> > -static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> > -
> > -/* Add sysfs properties */
> > -int pcibios_add_platform_entries(struct pci_dev *pdev)
> > -{
> > -	return device_create_file(&pdev->dev, &dev_attr_devspec);
> > -}
> > -
> >  /*
> >   * Reads the interrupt pin to determine if interrupt is use by card.
> >   * If the interrupt is used, then gets the interrupt line from the
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -416,6 +416,20 @@ static ssize_t d3cold_allowed_show(struc
> >  static DEVICE_ATTR_RW(d3cold_allowed);
> >  #endif
> >  
> > +#ifdef CONFIG_OF
> > +static ssize_t devspec_show(struct device *dev,
> > +			    struct device_attribute *attr, char *buf)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct device_node *np = pci_device_to_OF_node(pdev);
> > +
> > +	if (np == NULL || np->full_name == NULL)
> > +		return 0;
> > +	return sprintf(buf, "%s", np->full_name);
> > +}
> > +static DEVICE_ATTR_RO(devspec);
> > +#endif
> > +
> >  #ifdef CONFIG_PCI_IOV
> >  static ssize_t sriov_totalvfs_show(struct device *dev,
> >  				   struct device_attribute *attr,
> > @@ -521,6 +535,9 @@ static struct attribute *pci_dev_attrs[]
> >  #if defined(CONFIG_PM_RUNTIME) && defined(CONFIG_ACPI)
> >  	&dev_attr_d3cold_allowed.attr,
> >  #endif
> > +#ifdef CONFIG_OF
> > +	&dev_attr_devspec.attr,
> > +#endif
> >  	NULL,
> >  };
> >  
> 
> 
> 

Re: [Resend] pcibios_add_platform_entries usage

[Benjamin Herrenschmidt]

On Fri, 2014-04-18 at 12:10 +0200, Sebastian Ott wrote:
> On Fri, 18 Apr 2014, Benjamin Herrenschmidt wrote:
> > On Thu, 2014-04-17 at 19:46 +0200, Sebastian Ott wrote:
> > 
> > > pci: move open fabric devspec attribute to pci common code
> > > 
> > > Move the devspec OF attribute to pci common code's set of device
> > > attributes since it's not architecture dependent.
> > > As a side effect microblaze and powerpc no longer need to use
> > > pcibios_add_platform_entries.
> > > 
> > > Link: https://lkml.kernel.org/r/alpine.LFD.2.11.1404141101500.1529@denkbrett
> > > Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
> > 
> > Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > 
> > (Pending you've compile tested it on powerpc, I'm on vacation and
> > haven't done it :)
> 
> I did:
> make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2- g5_defconfig
> make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2-
> 
> and failed with:
>   CC      arch/powerpc/lib/xor_vmx.o
> In file included from include/linux/list.h:4,
>                  from include/linux/preempt.h:10,
>                  from arch/powerpc/lib/xor_vmx.c:22:
> include/linux/types.h:29: error: both ‘unsigned’ and ‘_Bool’ in declaration specifiers
> cc1: warnings being treated as errors
> include/linux/types.h:29: error: useless type name in empty declaration

Not sure what's up here, did you try with a more recent compiler ? 4.3
is pretty ancient by kernel standards.

You can find cross compilers there:

https://www.kernel.org/pub/tools/crosstool/

They aren't the newest either but they should work.

Ben.

> 
> make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2- arch/powerpc/kernel/ drivers/pci/
> 
> worked just fine.
> 
> Regards,
> Sebastian
> 
> > 
> > > ---
> > >  arch/microblaze/pci/pci-common.c |   20 --------------------
> > >  arch/powerpc/kernel/pci-common.c |   20 --------------------
> > >  drivers/pci/pci-sysfs.c          |   17 +++++++++++++++++
> > >  3 files changed, 17 insertions(+), 40 deletions(-)
> > > 
> > > --- a/arch/microblaze/pci/pci-common.c
> > > +++ b/arch/microblaze/pci/pci-common.c
> > > @@ -168,26 +168,6 @@ struct pci_controller *pci_find_hose_for
> > >  	return NULL;
> > >  }
> > >  
> > > -static ssize_t pci_show_devspec(struct device *dev,
> > > -		struct device_attribute *attr, char *buf)
> > > -{
> > > -	struct pci_dev *pdev;
> > > -	struct device_node *np;
> > > -
> > > -	pdev = to_pci_dev(dev);
> > > -	np = pci_device_to_OF_node(pdev);
> > > -	if (np == NULL || np->full_name == NULL)
> > > -		return 0;
> > > -	return sprintf(buf, "%s", np->full_name);
> > > -}
> > > -static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> > > -
> > > -/* Add sysfs properties */
> > > -int pcibios_add_platform_entries(struct pci_dev *pdev)
> > > -{
> > > -	return device_create_file(&pdev->dev, &dev_attr_devspec);
> > > -}
> > > -
> > >  void pcibios_set_master(struct pci_dev *dev)
> > >  {
> > >  	/* No special bus mastering setup handling */
> > > --- a/arch/powerpc/kernel/pci-common.c
> > > +++ b/arch/powerpc/kernel/pci-common.c
> > > @@ -201,26 +201,6 @@ struct pci_controller* pci_find_hose_for
> > >  	return NULL;
> > >  }
> > >  
> > > -static ssize_t pci_show_devspec(struct device *dev,
> > > -		struct device_attribute *attr, char *buf)
> > > -{
> > > -	struct pci_dev *pdev;
> > > -	struct device_node *np;
> > > -
> > > -	pdev = to_pci_dev (dev);
> > > -	np = pci_device_to_OF_node(pdev);
> > > -	if (np == NULL || np->full_name == NULL)
> > > -		return 0;
> > > -	return sprintf(buf, "%s", np->full_name);
> > > -}
> > > -static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> > > -
> > > -/* Add sysfs properties */
> > > -int pcibios_add_platform_entries(struct pci_dev *pdev)
> > > -{
> > > -	return device_create_file(&pdev->dev, &dev_attr_devspec);
> > > -}
> > > -
> > >  /*
> > >   * Reads the interrupt pin to determine if interrupt is use by card.
> > >   * If the interrupt is used, then gets the interrupt line from the
> > > --- a/drivers/pci/pci-sysfs.c
> > > +++ b/drivers/pci/pci-sysfs.c
> > > @@ -416,6 +416,20 @@ static ssize_t d3cold_allowed_show(struc
> > >  static DEVICE_ATTR_RW(d3cold_allowed);
> > >  #endif
> > >  
> > > +#ifdef CONFIG_OF
> > > +static ssize_t devspec_show(struct device *dev,
> > > +			    struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > > +	struct device_node *np = pci_device_to_OF_node(pdev);
> > > +
> > > +	if (np == NULL || np->full_name == NULL)
> > > +		return 0;
> > > +	return sprintf(buf, "%s", np->full_name);
> > > +}
> > > +static DEVICE_ATTR_RO(devspec);
> > > +#endif
> > > +
> > >  #ifdef CONFIG_PCI_IOV
> > >  static ssize_t sriov_totalvfs_show(struct device *dev,
> > >  				   struct device_attribute *attr,
> > > @@ -521,6 +535,9 @@ static struct attribute *pci_dev_attrs[]
> > >  #if defined(CONFIG_PM_RUNTIME) && defined(CONFIG_ACPI)
> > >  	&dev_attr_d3cold_allowed.attr,
> > >  #endif
> > > +#ifdef CONFIG_OF
> > > +	&dev_attr_devspec.attr,
> > > +#endif
> > >  	NULL,
> > >  };
> > >  
> > 
> > 
> > 

Re: [Resend] pcibios_add_platform_entries usage

[Sebastian Ott]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5492 bytes --]

On Sat, 19 Apr 2014, Benjamin Herrenschmidt wrote:
> On Fri, 2014-04-18 at 12:10 +0200, Sebastian Ott wrote:
> > On Fri, 18 Apr 2014, Benjamin Herrenschmidt wrote:
> > > On Thu, 2014-04-17 at 19:46 +0200, Sebastian Ott wrote:
> > > 
> > > > pci: move open fabric devspec attribute to pci common code
> > > > 
> > > > Move the devspec OF attribute to pci common code's set of device
> > > > attributes since it's not architecture dependent.
> > > > As a side effect microblaze and powerpc no longer need to use
> > > > pcibios_add_platform_entries.
> > > > 
> > > > Link: https://lkml.kernel.org/r/alpine.LFD.2.11.1404141101500.1529@denkbrett
> > > > Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
> > > 
> > > Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > 
> > > (Pending you've compile tested it on powerpc, I'm on vacation and
> > > haven't done it :)
> > 
> > I did:
> > make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2- g5_defconfig
> > make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2-
> > 
> > and failed with:
> >   CC      arch/powerpc/lib/xor_vmx.o
> > In file included from include/linux/list.h:4,
> >                  from include/linux/preempt.h:10,
> >                  from arch/powerpc/lib/xor_vmx.c:22:
> > include/linux/types.h:29: error: both ‘unsigned’ and ‘_Bool’ in declaration specifiers
> > cc1: warnings being treated as errors
> > include/linux/types.h:29: error: useless type name in empty declaration
> 
> Not sure what's up here, did you try with a more recent compiler ? 4.3
> is pretty ancient by kernel standards.
> 
> You can find cross compilers there:
> 
> https://www.kernel.org/pub/tools/crosstool/
> 
> They aren't the newest either but they should work.

Yes. That did the trick.
Thanks,
Sebastian


> > 
> > make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2- arch/powerpc/kernel/ drivers/pci/
> > 
> > worked just fine.
> > 
> > Regards,
> > Sebastian
> > 
> > > 
> > > > ---
> > > >  arch/microblaze/pci/pci-common.c |   20 --------------------
> > > >  arch/powerpc/kernel/pci-common.c |   20 --------------------
> > > >  drivers/pci/pci-sysfs.c          |   17 +++++++++++++++++
> > > >  3 files changed, 17 insertions(+), 40 deletions(-)
> > > > 
> > > > --- a/arch/microblaze/pci/pci-common.c
> > > > +++ b/arch/microblaze/pci/pci-common.c
> > > > @@ -168,26 +168,6 @@ struct pci_controller *pci_find_hose_for
> > > >  	return NULL;
> > > >  }
> > > >  
> > > > -static ssize_t pci_show_devspec(struct device *dev,
> > > > -		struct device_attribute *attr, char *buf)
> > > > -{
> > > > -	struct pci_dev *pdev;
> > > > -	struct device_node *np;
> > > > -
> > > > -	pdev = to_pci_dev(dev);
> > > > -	np = pci_device_to_OF_node(pdev);
> > > > -	if (np == NULL || np->full_name == NULL)
> > > > -		return 0;
> > > > -	return sprintf(buf, "%s", np->full_name);
> > > > -}
> > > > -static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> > > > -
> > > > -/* Add sysfs properties */
> > > > -int pcibios_add_platform_entries(struct pci_dev *pdev)
> > > > -{
> > > > -	return device_create_file(&pdev->dev, &dev_attr_devspec);
> > > > -}
> > > > -
> > > >  void pcibios_set_master(struct pci_dev *dev)
> > > >  {
> > > >  	/* No special bus mastering setup handling */
> > > > --- a/arch/powerpc/kernel/pci-common.c
> > > > +++ b/arch/powerpc/kernel/pci-common.c
> > > > @@ -201,26 +201,6 @@ struct pci_controller* pci_find_hose_for
> > > >  	return NULL;
> > > >  }
> > > >  
> > > > -static ssize_t pci_show_devspec(struct device *dev,
> > > > -		struct device_attribute *attr, char *buf)
> > > > -{
> > > > -	struct pci_dev *pdev;
> > > > -	struct device_node *np;
> > > > -
> > > > -	pdev = to_pci_dev (dev);
> > > > -	np = pci_device_to_OF_node(pdev);
> > > > -	if (np == NULL || np->full_name == NULL)
> > > > -		return 0;
> > > > -	return sprintf(buf, "%s", np->full_name);
> > > > -}
> > > > -static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> > > > -
> > > > -/* Add sysfs properties */
> > > > -int pcibios_add_platform_entries(struct pci_dev *pdev)
> > > > -{
> > > > -	return device_create_file(&pdev->dev, &dev_attr_devspec);
> > > > -}
> > > > -
> > > >  /*
> > > >   * Reads the interrupt pin to determine if interrupt is use by card.
> > > >   * If the interrupt is used, then gets the interrupt line from the
> > > > --- a/drivers/pci/pci-sysfs.c
> > > > +++ b/drivers/pci/pci-sysfs.c
> > > > @@ -416,6 +416,20 @@ static ssize_t d3cold_allowed_show(struc
> > > >  static DEVICE_ATTR_RW(d3cold_allowed);
> > > >  #endif
> > > >  
> > > > +#ifdef CONFIG_OF
> > > > +static ssize_t devspec_show(struct device *dev,
> > > > +			    struct device_attribute *attr, char *buf)
> > > > +{
> > > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > > > +	struct device_node *np = pci_device_to_OF_node(pdev);
> > > > +
> > > > +	if (np == NULL || np->full_name == NULL)
> > > > +		return 0;
> > > > +	return sprintf(buf, "%s", np->full_name);
> > > > +}
> > > > +static DEVICE_ATTR_RO(devspec);
> > > > +#endif
> > > > +
> > > >  #ifdef CONFIG_PCI_IOV
> > > >  static ssize_t sriov_totalvfs_show(struct device *dev,
> > > >  				   struct device_attribute *attr,
> > > > @@ -521,6 +535,9 @@ static struct attribute *pci_dev_attrs[]
> > > >  #if defined(CONFIG_PM_RUNTIME) && defined(CONFIG_ACPI)
> > > >  	&dev_attr_d3cold_allowed.attr,
> > > >  #endif
> > > > +#ifdef CONFIG_OF
> > > > +	&dev_attr_devspec.attr,
> > > > +#endif
> > > >  	NULL,
> > > >  };
> > > >  
> > > 
> > > 
> > > 
> 
> 
> 

Re: [Resend] pcibios_add_platform_entries usage

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2300 bytes --]

On 04/22/2014 10:26 AM, Sebastian Ott wrote:
> On Sat, 19 Apr 2014, Benjamin Herrenschmidt wrote:
>> On Fri, 2014-04-18 at 12:10 +0200, Sebastian Ott wrote:
>>> On Fri, 18 Apr 2014, Benjamin Herrenschmidt wrote:
>>>> On Thu, 2014-04-17 at 19:46 +0200, Sebastian Ott wrote:
>>>>
>>>>> pci: move open fabric devspec attribute to pci common code
>>>>>
>>>>> Move the devspec OF attribute to pci common code's set of device
>>>>> attributes since it's not architecture dependent.
>>>>> As a side effect microblaze and powerpc no longer need to use
>>>>> pcibios_add_platform_entries.
>>>>>
>>>>> Link: https://lkml.kernel.org/r/alpine.LFD.2.11.1404141101500.1529@denkbrett
>>>>> Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
>>>>
>>>> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>
>>>> (Pending you've compile tested it on powerpc, I'm on vacation and
>>>> haven't done it :)
>>>
>>> I did:
>>> make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2- g5_defconfig
>>> make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2-
>>>
>>> and failed with:
>>>   CC      arch/powerpc/lib/xor_vmx.o
>>> In file included from include/linux/list.h:4,
>>>                  from include/linux/preempt.h:10,
>>>                  from arch/powerpc/lib/xor_vmx.c:22:
>>> include/linux/types.h:29: error: both ‘unsigned’ and ‘_Bool’ in declaration specifiers
>>> cc1: warnings being treated as errors
>>> include/linux/types.h:29: error: useless type name in empty declaration
>>
>> Not sure what's up here, did you try with a more recent compiler ? 4.3
>> is pretty ancient by kernel standards.
>>
>> You can find cross compilers there:
>>
>> https://www.kernel.org/pub/tools/crosstool/
>>
>> They aren't the newest either but they should work.
> 
> Yes. That did the trick.

Hope that you did the same for microblaze or just push the repo/branch with these
patches to zero-day testing system and you got the results.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Resend] pcibios_add_platform_entries usage

[Sebastian Ott]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2465 bytes --]



On Wed, 23 Apr 2014, Michal Simek wrote:

> On 04/22/2014 10:26 AM, Sebastian Ott wrote:
> > On Sat, 19 Apr 2014, Benjamin Herrenschmidt wrote:
> >> On Fri, 2014-04-18 at 12:10 +0200, Sebastian Ott wrote:
> >>> On Fri, 18 Apr 2014, Benjamin Herrenschmidt wrote:
> >>>> On Thu, 2014-04-17 at 19:46 +0200, Sebastian Ott wrote:
> >>>>
> >>>>> pci: move open fabric devspec attribute to pci common code
> >>>>>
> >>>>> Move the devspec OF attribute to pci common code's set of device
> >>>>> attributes since it's not architecture dependent.
> >>>>> As a side effect microblaze and powerpc no longer need to use
> >>>>> pcibios_add_platform_entries.
> >>>>>
> >>>>> Link: https://lkml.kernel.org/r/alpine.LFD.2.11.1404141101500.1529@denkbrett
> >>>>> Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
> >>>>
> >>>> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>>>
> >>>> (Pending you've compile tested it on powerpc, I'm on vacation and
> >>>> haven't done it :)
> >>>
> >>> I did:
> >>> make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2- g5_defconfig
> >>> make ARCH=powerpc CROSS_COMPILE=powerpc64-4.3.2-
> >>>
> >>> and failed with:
> >>>   CC      arch/powerpc/lib/xor_vmx.o
> >>> In file included from include/linux/list.h:4,
> >>>                  from include/linux/preempt.h:10,
> >>>                  from arch/powerpc/lib/xor_vmx.c:22:
> >>> include/linux/types.h:29: error: both ‘unsigned’ and ‘_Bool’ in declaration specifiers
> >>> cc1: warnings being treated as errors
> >>> include/linux/types.h:29: error: useless type name in empty declaration
> >>
> >> Not sure what's up here, did you try with a more recent compiler ? 4.3
> >> is pretty ancient by kernel standards.
> >>
> >> You can find cross compilers there:
> >>
> >> https://www.kernel.org/pub/tools/crosstool/
> >>
> >> They aren't the newest either but they should work.
> > 
> > Yes. That did the trick.
> 
> Hope that you did the same for microblaze or just push the repo/branch with these
> patches to zero-day testing system and you got the results.

Yes, I compile-tested microblaze too.

Regards,
Sebastian
> 
> Thanks,
> Michal
> 
> 
> -- 
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
> Maintainer of Linux kernel - Xilinx Zynq ARM architecture
> Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
> 
> 
> 

Re: [Resend] pcibios_add_platform_entries usage

[Bjorn Helgaas]

On Thu, Apr 17, 2014 at 07:46:15PM +0200, Sebastian Ott wrote:
> On Mon, 14 Apr 2014, Sebastian Ott wrote:
> > On Mon, 14 Apr 2014, Bjorn Helgaas wrote:
> > > On Mon, Apr 14, 2014 at 11:03:42AM +0200, Sebastian Ott wrote:
> > > > for pci on s390 we currently use pcibios_add_platform_entries to add
> > > > some arch specific attributes to pdevs. This has 2 downsides - it will
> > > > race with userspace which is triggered by udev events and expecting
> > > > these attributes (but that's a theoretical issue). More important to
> > > > me is that one cannot use attribute_groups with this. Both issues could
> > > > be addressed by using pdev->dev.groups and let the driver core handle
> > > > attribute creation.
> > > > 
> > > > So would it be ok if we set pdev->dev.groups in pcibios_add_device?
> > > > (It should be since it's not used by pci common code which uses bus_type,
> > > > dev_type, and class groups).
> > > 
> > > Hi Sebastian,
> > > 
> > > Sorry, I meant to respond to this earlier, but forgot.  This sounds
> > > reasonable to me, but Greg can give you a much better answer than I can.
> > > 
> > > Documentation/driver-model/device.txt says the dev->groups pointer
> > > should be set before calling device_register().  PCI calls
> > > device_initialize() and device_add() instead of using device_register(),
> > > and pcibios_add_device() looks like it happens at the right time:
> > > 
> > >     pci_scan_root_bus
> > >       pci_scan_child_bus
> > >         pci_scan_slot
> > >           pci_scan_single_device
> > >             pci_device_add
> > >               device_initialize
> > >               pcibios_add_device                # <---
> > >               device_add
> > >       pci_bus_add_devices
> > >         pci_bus_add_device
> > >           pci_create_sysfs_dev_files
> > >             pcibios_add_platform_entries        # 8d4cd0833107 (benh)
> > >           device_attach
> > > 
> > > I'm not sure why pci_create_sysfs_dev_files() is done later.  It seems
> > > like that should be done before device_add() as well.  Maybe it's
> > > because BARs might not be valid yet (that doesn't seem like a very good
> > > excuse, but it's all I can think of).
> > > 
> > > I assume that if you change s390, you'll also change microblaze and
> > > powerpc?  They look structurally similar to s390.
> > 
> > Yes, that sounds like a plan - this way we can get rid of
> > pcibios_add_platform_entries altogether. I'll send these patches soon.
> 
> Hm, pcibios_add_platform_entries for microblaze and power is identical
> and this OF stuff seems not to be arch specific. How about the following
> patch?
> 
> 
> pci: move open fabric devspec attribute to pci common code
> 
> Move the devspec OF attribute to pci common code's set of device
> attributes since it's not architecture dependent.
> As a side effect microblaze and powerpc no longer need to use
> pcibios_add_platform_entries.
> 
> Link: https://lkml.kernel.org/r/alpine.LFD.2.11.1404141101500.1529@denkbrett
> Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>

I applied this with Ben's ack to pci/misc for v3.16.

I don't see a corresponding s390 patch; did I miss it?  I don't want to
apply the "Remove pcibios_add_platform_entries()" patch until s390 is
fixed up too.

Bjorn

> ---
>  arch/microblaze/pci/pci-common.c |   20 --------------------
>  arch/powerpc/kernel/pci-common.c |   20 --------------------
>  drivers/pci/pci-sysfs.c          |   17 +++++++++++++++++
>  3 files changed, 17 insertions(+), 40 deletions(-)
> 
> --- a/arch/microblaze/pci/pci-common.c
> +++ b/arch/microblaze/pci/pci-common.c
> @@ -168,26 +168,6 @@ struct pci_controller *pci_find_hose_for
>  	return NULL;
>  }
>  
> -static ssize_t pci_show_devspec(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct pci_dev *pdev;
> -	struct device_node *np;
> -
> -	pdev = to_pci_dev(dev);
> -	np = pci_device_to_OF_node(pdev);
> -	if (np == NULL || np->full_name == NULL)
> -		return 0;
> -	return sprintf(buf, "%s", np->full_name);
> -}
> -static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> -
> -/* Add sysfs properties */
> -int pcibios_add_platform_entries(struct pci_dev *pdev)
> -{
> -	return device_create_file(&pdev->dev, &dev_attr_devspec);
> -}
> -
>  void pcibios_set_master(struct pci_dev *dev)
>  {
>  	/* No special bus mastering setup handling */
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -201,26 +201,6 @@ struct pci_controller* pci_find_hose_for
>  	return NULL;
>  }
>  
> -static ssize_t pci_show_devspec(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct pci_dev *pdev;
> -	struct device_node *np;
> -
> -	pdev = to_pci_dev (dev);
> -	np = pci_device_to_OF_node(pdev);
> -	if (np == NULL || np->full_name == NULL)
> -		return 0;
> -	return sprintf(buf, "%s", np->full_name);
> -}
> -static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> -
> -/* Add sysfs properties */
> -int pcibios_add_platform_entries(struct pci_dev *pdev)
> -{
> -	return device_create_file(&pdev->dev, &dev_attr_devspec);
> -}
> -
>  /*
>   * Reads the interrupt pin to determine if interrupt is use by card.
>   * If the interrupt is used, then gets the interrupt line from the
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -416,6 +416,20 @@ static ssize_t d3cold_allowed_show(struc
>  static DEVICE_ATTR_RW(d3cold_allowed);
>  #endif
>  
> +#ifdef CONFIG_OF
> +static ssize_t devspec_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct device_node *np = pci_device_to_OF_node(pdev);
> +
> +	if (np == NULL || np->full_name == NULL)
> +		return 0;
> +	return sprintf(buf, "%s", np->full_name);
> +}
> +static DEVICE_ATTR_RO(devspec);
> +#endif
> +
>  #ifdef CONFIG_PCI_IOV
>  static ssize_t sriov_totalvfs_show(struct device *dev,
>  				   struct device_attribute *attr,
> @@ -521,6 +535,9 @@ static struct attribute *pci_dev_attrs[]
>  #if defined(CONFIG_PM_RUNTIME) && defined(CONFIG_ACPI)
>  	&dev_attr_d3cold_allowed.attr,
>  #endif
> +#ifdef CONFIG_OF
> +	&dev_attr_devspec.attr,
> +#endif
>  	NULL,
>  };
>  

Re: [Resend] pcibios_add_platform_entries usage

[Sebastian Ott]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7273 bytes --]

On Tue, 29 Apr 2014, Bjorn Helgaas wrote:
> On Thu, Apr 17, 2014 at 07:46:15PM +0200, Sebastian Ott wrote:
> > On Mon, 14 Apr 2014, Sebastian Ott wrote:
> > > On Mon, 14 Apr 2014, Bjorn Helgaas wrote:
> > > > On Mon, Apr 14, 2014 at 11:03:42AM +0200, Sebastian Ott wrote:
> > > > > for pci on s390 we currently use pcibios_add_platform_entries to add
> > > > > some arch specific attributes to pdevs. This has 2 downsides - it will
> > > > > race with userspace which is triggered by udev events and expecting
> > > > > these attributes (but that's a theoretical issue). More important to
> > > > > me is that one cannot use attribute_groups with this. Both issues could
> > > > > be addressed by using pdev->dev.groups and let the driver core handle
> > > > > attribute creation.
> > > > > 
> > > > > So would it be ok if we set pdev->dev.groups in pcibios_add_device?
> > > > > (It should be since it's not used by pci common code which uses bus_type,
> > > > > dev_type, and class groups).
> > > > 
> > > > Hi Sebastian,
> > > > 
> > > > Sorry, I meant to respond to this earlier, but forgot.  This sounds
> > > > reasonable to me, but Greg can give you a much better answer than I can.
> > > > 
> > > > Documentation/driver-model/device.txt says the dev->groups pointer
> > > > should be set before calling device_register().  PCI calls
> > > > device_initialize() and device_add() instead of using device_register(),
> > > > and pcibios_add_device() looks like it happens at the right time:
> > > > 
> > > >     pci_scan_root_bus
> > > >       pci_scan_child_bus
> > > >         pci_scan_slot
> > > >           pci_scan_single_device
> > > >             pci_device_add
> > > >               device_initialize
> > > >               pcibios_add_device                # <---
> > > >               device_add
> > > >       pci_bus_add_devices
> > > >         pci_bus_add_device
> > > >           pci_create_sysfs_dev_files
> > > >             pcibios_add_platform_entries        # 8d4cd0833107 (benh)
> > > >           device_attach
> > > > 
> > > > I'm not sure why pci_create_sysfs_dev_files() is done later.  It seems
> > > > like that should be done before device_add() as well.  Maybe it's
> > > > because BARs might not be valid yet (that doesn't seem like a very good
> > > > excuse, but it's all I can think of).
> > > > 
> > > > I assume that if you change s390, you'll also change microblaze and
> > > > powerpc?  They look structurally similar to s390.
> > > 
> > > Yes, that sounds like a plan - this way we can get rid of
> > > pcibios_add_platform_entries altogether. I'll send these patches soon.
> > 
> > Hm, pcibios_add_platform_entries for microblaze and power is identical
> > and this OF stuff seems not to be arch specific. How about the following
> > patch?
> > 
> > 
> > pci: move open fabric devspec attribute to pci common code
> > 
> > Move the devspec OF attribute to pci common code's set of device
> > attributes since it's not architecture dependent.
> > As a side effect microblaze and powerpc no longer need to use
> > pcibios_add_platform_entries.
> > 
> > Link: https://lkml.kernel.org/r/alpine.LFD.2.11.1404141101500.1529@denkbrett
> > Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
> 
> I applied this with Ben's ack to pci/misc for v3.16.
> 
> I don't see a corresponding s390 patch; did I miss it?  I don't want to
> apply the "Remove pcibios_add_platform_entries()" patch until s390 is
> fixed up too.

My bad. It's in the feature branch of Martin's s390 tree:
https://git.kernel.org/cgit/linux/kernel/git/s390/linux.git/commit/?h=features&id=8e209e424f8b816f9d957b99ac8d514dc1402f38

>From 8e209e424f8b816f9d957b99ac8d514dc1402f38 Mon Sep 17 00:00:00 2001
From: Sebastian Ott <sebott@linux.vnet.ibm.com>
Date: Wed, 16 Apr 2014 16:11:00 +0200
Subject: [PATCH] s390/pci: use pdev->dev.groups for attribute creation

Let the driver core handle attribute creation by putting all s390
specific pci attributes in an attribute group which is referenced
by pdev->dev.groups in pcibios_add_device.

Link: https://lkml.kernel.org/r/alpine.LFD.2.11.1404141101500.1529@denkbrett
Reviewed-by: Gerald Schaefer <gerald.schaefer@de.ibm.com>
Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/include/asm/pci.h |  6 ++----
 arch/s390/pci/pci.c         |  6 +-----
 arch/s390/pci/pci_sysfs.c   | 44 +++++++++++++-------------------------------
 3 files changed, 16 insertions(+), 40 deletions(-)

diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 2583466..79b5f07 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -120,6 +120,8 @@ static inline bool zdev_enabled(struct zpci_dev *zdev)
 	return (zdev->fh & (1UL << 31)) ? true : false;
 }
 
+extern const struct attribute_group *zpci_attr_groups[];
+
 /* -----------------------------------------------------------------------------
   Prototypes
 ----------------------------------------------------------------------------- */
@@ -166,10 +168,6 @@ static inline void zpci_exit_slot(struct zpci_dev *zdev) {}
 struct zpci_dev *get_zdev(struct pci_dev *);
 struct zpci_dev *get_zdev_by_fid(u32);
 
-/* sysfs */
-int zpci_sysfs_add_device(struct device *);
-void zpci_sysfs_remove_device(struct device *);
-
 /* DMA */
 int zpci_dma_init(void);
 void zpci_dma_exit(void);
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 1df1d29..bdf0257 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -530,11 +530,6 @@ static void zpci_unmap_resources(struct zpci_dev *zdev)
 	}
 }
 
-int pcibios_add_platform_entries(struct pci_dev *pdev)
-{
-	return zpci_sysfs_add_device(&pdev->dev);
-}
-
 static int __init zpci_irq_init(void)
 {
 	int rc;
@@ -671,6 +666,7 @@ int pcibios_add_device(struct pci_dev *pdev)
 	int i;
 
 	zdev->pdev = pdev;
+	pdev->dev.groups = zpci_attr_groups;
 	zpci_map_resources(zdev);
 
 	for (i = 0; i < PCI_BAR_COUNT; i++) {
diff --git a/arch/s390/pci/pci_sysfs.c b/arch/s390/pci/pci_sysfs.c
index ebe2c16..f23da1c 100644
--- a/arch/s390/pci/pci_sysfs.c
+++ b/arch/s390/pci/pci_sysfs.c
@@ -51,36 +51,18 @@ static ssize_t recover_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_WO(recover);
 
-static struct device_attribute *zpci_dev_attrs[] = {
-	&dev_attr_function_id,
-	&dev_attr_function_handle,
-	&dev_attr_pchid,
-	&dev_attr_pfgid,
-	&dev_attr_recover,
+static struct attribute *zpci_dev_attrs[] = {
+	&dev_attr_function_id.attr,
+	&dev_attr_function_handle.attr,
+	&dev_attr_pchid.attr,
+	&dev_attr_pfgid.attr,
+	&dev_attr_recover.attr,
+	NULL,
+};
+static struct attribute_group zpci_attr_group = {
+	.attrs = zpci_dev_attrs,
+};
+const struct attribute_group *zpci_attr_groups[] = {
+	&zpci_attr_group,
 	NULL,
 };
-
-int zpci_sysfs_add_device(struct device *dev)
-{
-	int i, rc = 0;
-
-	for (i = 0; zpci_dev_attrs[i]; i++) {
-		rc = device_create_file(dev, zpci_dev_attrs[i]);
-		if (rc)
-			goto error;
-	}
-	return 0;
-
-error:
-	while (--i >= 0)
-		device_remove_file(dev, zpci_dev_attrs[i]);
-	return rc;
-}
-
-void zpci_sysfs_remove_device(struct device *dev)
-{
-	int i;
-
-	for (i = 0; zpci_dev_attrs[i]; i++)
-		device_remove_file(dev, zpci_dev_attrs[i]);
-}
-- 
1.8.5.5