Patchwork [Resend] pcibios_add_platform_entries usage

login
register
mail settings
Submitter Sebastian Ott
Date April 17, 2014, 5:46 p.m.
Message ID <alpine.LFD.2.11.1404171938190.1588@denkbrett>
Download mbox | patch
Permalink /patch/339994/
State Accepted
Headers show

Comments

Sebastian Ott - April 17, 2014, 5:46 p.m.
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(-)
Benjamin Herrenschmidt - April 17, 2014, 9:02 p.m.
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,
>  };
>  


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Ott - April 18, 2014, 10:10 a.m.
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,
> >  };
> >  
> 
> 
>
Benjamin Herrenschmidt - April 18, 2014, 9:31 p.m.
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,
> > >  };
> > >  
> > 
> > 
> > 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Ott - April 22, 2014, 8:26 a.m.
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,
> > > >  };
> > > >  
> > > 
> > > 
> > > 
> 
> 
>
Michal Simek - April 23, 2014, 6:48 a.m.
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
Sebastian Ott - April 23, 2014, 9 a.m.
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
> 
> 
>
Bjorn Helgaas - April 29, 2014, 11:30 p.m.
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,
>  };
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- 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,
 };