diff mbox

[v3,2/4] PCI: Allow driver-specific data in host bridge

Message ID 20160815153647.1075-2-thierry.reding@gmail.com
State Superseded
Headers show

Commit Message

Thierry Reding Aug. 15, 2016, 3:36 p.m. UTC 
From: Thierry Reding <treding@nvidia.com>

Provide a way to allocate driver-specific data along with a PCI host
bridge structure. The bridge's ->private field points to this data.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/pci/probe.c | 9 ++++++---
 include/linux/pci.h | 1 +
 2 files changed, 7 insertions(+), 3 deletions(-)

Comments

Lorenzo Pieralisi Aug. 19, 2016, 4:55 p.m. UTC | #1 
On Mon, Aug 15, 2016 at 05:36:45PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Provide a way to allocate driver-specific data along with a PCI host
> bridge structure. The bridge's ->private field points to this data.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/pci/probe.c | 9 ++++++---
>  include/linux/pci.h | 1 +
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 93583b389058..ecf543014da3 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -521,16 +521,19 @@ static void pci_release_host_bridge_dev(struct device *dev)
>  	kfree(bridge);
>  }
>  
> -static struct pci_host_bridge *pci_alloc_host_bridge(void)
> +static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
>  {
>  	struct pci_host_bridge *bridge;
>  
> -	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> +	bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
>  	if (!bridge)
>  		return NULL;
>  
>  	INIT_LIST_HEAD(&bridge->windows);
>  
> +	if (priv)
> +		bridge->private = &bridge[1];

How about making private a zero length array ?

Lorenzo

> +
>  	return bridge;
>  }
>  
> @@ -2249,7 +2252,7 @@ static struct pci_bus *pci_create_root_bus_msi(struct device *parent,
>  	int error;
>  	struct pci_host_bridge *bridge;
>  
> -	bridge = pci_alloc_host_bridge();
> +	bridge = pci_alloc_host_bridge(0);
>  	if (!bridge)
>  		return NULL;
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index ccf298fad9e7..3aa7240800c8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -421,6 +421,7 @@ struct pci_host_bridge {
>  			resource_size_t start,
>  			resource_size_t size,
>  			resource_size_t align);
> +	void *private;
>  };
>  
>  #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
> -- 
> 2.9.0
> 
> --
> 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
> 
--
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
Arnd Bergmann Aug. 22, 2016, 2 p.m. UTC | #2 
On Friday, August 19, 2016 5:55:06 PM CEST Lorenzo Pieralisi wrote:
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 93583b389058..ecf543014da3 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -521,16 +521,19 @@ static void pci_release_host_bridge_dev(struct device *dev)
> >       kfree(bridge);
> >  }
> >  
> > -static struct pci_host_bridge *pci_alloc_host_bridge(void)
> > +static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> >  {
> >       struct pci_host_bridge *bridge;
> >  
> > -     bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> > +     bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
> >       if (!bridge)
> >               return NULL;
> >  
> >       INIT_LIST_HEAD(&bridge->windows);
> >  
> > +     if (priv)
> > +             bridge->private = &bridge[1];
> 
> How about making private a zero length array ?

Right, the member can actually be removed here if we want to, this
was just meant as a shorthand so we can avoid the cast or function
call in device drivers. I think either way makes sense.

Also, someone commented on a related issue in the previous version, and I
recommended to align it to ARCH_SLAB_MINALIGN. This might be done more
easily with the zero-length array using

struct pci_host_bridge {
	...

	unsigned long private[0] __attribute__((aligned(ARCH_SLAB_MINALIGN)));
};

Without this, we need a bit more computation, like
	
	bridge = kzalloc(ALIGN(sizeof(*bridge), ARCH_SLAB_MINALIGN) + priv, GFP_KERNEL);
	bridge->private = (void *)bridge + ALIGN(sizeof(*bridge);

or

static inline void *pci_bridge_private(struct pci_host_bridge *bridge)
{
	return (void *)bridge + ALIGN(sizeof(*bridge);
}
static inline struct pci_host_bridge *pci_bridge_from_private(void *priv)
{
	return priv - ALIGN(sizeof(*bridge);
}

The alignment is needed if the private structure contains any data that
we may want to transfer using DMA.

	Arnd
--
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
Lorenzo Pieralisi Aug. 23, 2016, 1:59 p.m. UTC | #3 
On Mon, Aug 22, 2016 at 04:00:42PM +0200, Arnd Bergmann wrote:
> On Friday, August 19, 2016 5:55:06 PM CEST Lorenzo Pieralisi wrote:
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 93583b389058..ecf543014da3 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -521,16 +521,19 @@ static void pci_release_host_bridge_dev(struct device *dev)
> > >       kfree(bridge);
> > >  }
> > >  
> > > -static struct pci_host_bridge *pci_alloc_host_bridge(void)
> > > +static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> > >  {
> > >       struct pci_host_bridge *bridge;
> > >  
> > > -     bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> > > +     bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
> > >       if (!bridge)
> > >               return NULL;
> > >  
> > >       INIT_LIST_HEAD(&bridge->windows);
> > >  
> > > +     if (priv)
> > > +             bridge->private = &bridge[1];
> > 
> > How about making private a zero length array ?
> 
> Right, the member can actually be removed here if we want to, this
> was just meant as a shorthand so we can avoid the cast or function
> call in device drivers. I think either way makes sense.
> 
> Also, someone commented on a related issue in the previous version, and I
> recommended to align it to ARCH_SLAB_MINALIGN. This might be done more
> easily with the zero-length array using
> 
> struct pci_host_bridge {
> 	...
> 
> 	unsigned long private[0] __attribute__((aligned(ARCH_SLAB_MINALIGN)));
> };

Yes, it looks simpler and I personally prefer it but it is more style
than anything else, I thought it was worth mentioning it though.

Thanks,
Lorenzo

> Without this, we need a bit more computation, like
> 	
> 	bridge = kzalloc(ALIGN(sizeof(*bridge), ARCH_SLAB_MINALIGN) + priv, GFP_KERNEL);
> 	bridge->private = (void *)bridge + ALIGN(sizeof(*bridge);
> 
> or
> 
> static inline void *pci_bridge_private(struct pci_host_bridge *bridge)
> {
> 	return (void *)bridge + ALIGN(sizeof(*bridge);
> }
> static inline struct pci_host_bridge *pci_bridge_from_private(void *priv)
> {
> 	return priv - ALIGN(sizeof(*bridge);
> }
> 
> The alignment is needed if the private structure contains any data that
> we may want to transfer using DMA.
> 
> 	Arnd
> 
--
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
Thierry Reding Nov. 25, 2016, 7:26 a.m. UTC | #4 
On Mon, Aug 22, 2016 at 04:00:42PM +0200, Arnd Bergmann wrote:
> On Friday, August 19, 2016 5:55:06 PM CEST Lorenzo Pieralisi wrote:
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 93583b389058..ecf543014da3 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -521,16 +521,19 @@ static void pci_release_host_bridge_dev(struct device *dev)
> > >       kfree(bridge);
> > >  }
> > >  
> > > -static struct pci_host_bridge *pci_alloc_host_bridge(void)
> > > +static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> > >  {
> > >       struct pci_host_bridge *bridge;
> > >  
> > > -     bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> > > +     bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
> > >       if (!bridge)
> > >               return NULL;
> > >  
> > >       INIT_LIST_HEAD(&bridge->windows);
> > >  
> > > +     if (priv)
> > > +             bridge->private = &bridge[1];
> > 
> > How about making private a zero length array ?
> 
> Right, the member can actually be removed here if we want to, this
> was just meant as a shorthand so we can avoid the cast or function
> call in device drivers. I think either way makes sense.
> 
> Also, someone commented on a related issue in the previous version, and I
> recommended to align it to ARCH_SLAB_MINALIGN. This might be done more
> easily with the zero-length array using
> 
> struct pci_host_bridge {
> 	...
> 
> 	unsigned long private[0] __attribute__((aligned(ARCH_SLAB_MINALIGN)));
> };
> 
> Without this, we need a bit more computation, like
> 	
> 	bridge = kzalloc(ALIGN(sizeof(*bridge), ARCH_SLAB_MINALIGN) + priv, GFP_KERNEL);
> 	bridge->private = (void *)bridge + ALIGN(sizeof(*bridge);
> 
> or
> 
> static inline void *pci_bridge_private(struct pci_host_bridge *bridge)
> {
> 	return (void *)bridge + ALIGN(sizeof(*bridge);
> }
> static inline struct pci_host_bridge *pci_bridge_from_private(void *priv)
> {
> 	return priv - ALIGN(sizeof(*bridge);
> }
> 
> The alignment is needed if the private structure contains any data that
> we may want to transfer using DMA.

Looks like I never replied to this, though I seem to remember that I
did...

My question is: why would we want the private structure to be aligned?
Surely if somebody has data in the private structure that needs to be
transferred using DMA then they need to make sure that that field will
be properly aligned. Aligning the private structure itself won't ensure
that, right?

Anyway, it seems like there's very little contentious about this other
than the minor issue of how to get at the private data. I'd like to make
progress on this because we're now three generations into 64-bit with
Tegra and still missing PCI support on all of them.

Thierry
Arnd Bergmann Nov. 25, 2016, 9:53 a.m. UTC | #5 
On Friday, November 25, 2016 8:26:29 AM CET Thierry Reding wrote:
> On Mon, Aug 22, 2016 at 04:00:42PM +0200, Arnd Bergmann wrote:
> > On Friday, August 19, 2016 5:55:06 PM CEST Lorenzo Pieralisi wrote:
> > > > 
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 93583b389058..ecf543014da3 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -521,16 +521,19 @@ static void pci_release_host_bridge_dev(struct device *dev)
> > > >       kfree(bridge);
> > > >  }
> > > >  
> > > > -static struct pci_host_bridge *pci_alloc_host_bridge(void)
> > > > +static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> > > >  {
> > > >       struct pci_host_bridge *bridge;
> > > >  
> > > > -     bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> > > > +     bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
> > > >       if (!bridge)
> > > >               return NULL;
> > > >  
> > > >       INIT_LIST_HEAD(&bridge->windows);
> > > >  
> > > > +     if (priv)
> > > > +             bridge->private = &bridge[1];
> > > 
> > > How about making private a zero length array ?
> > 
> > Right, the member can actually be removed here if we want to, this
> > was just meant as a shorthand so we can avoid the cast or function
> > call in device drivers. I think either way makes sense.
> > 
> > Also, someone commented on a related issue in the previous version, and I
> > recommended to align it to ARCH_SLAB_MINALIGN. This might be done more
> > easily with the zero-length array using
> > 
> > struct pci_host_bridge {
> > 	...
> > 
> > 	unsigned long private[0] __attribute__((aligned(ARCH_SLAB_MINALIGN)));
> > };
> > 
> > Without this, we need a bit more computation, like
> > 	
> > 	bridge = kzalloc(ALIGN(sizeof(*bridge), ARCH_SLAB_MINALIGN) + priv, GFP_KERNEL);
> > 	bridge->private = (void *)bridge + ALIGN(sizeof(*bridge);
> > 
> > or
> > 
> > static inline void *pci_bridge_private(struct pci_host_bridge *bridge)
> > {
> > 	return (void *)bridge + ALIGN(sizeof(*bridge);
> > }
> > static inline struct pci_host_bridge *pci_bridge_from_private(void *priv)
> > {
> > 	return priv - ALIGN(sizeof(*bridge);
> > }
> > 
> > The alignment is needed if the private structure contains any data that
> > we may want to transfer using DMA.
> 
> Looks like I never replied to this, though I seem to remember that I
> did...
> 
> My question is: why would we want the private structure to be aligned?
> Surely if somebody has data in the private structure that needs to be
> transferred using DMA then they need to make sure that that field will
> be properly aligned. Aligning the private structure itself won't ensure
> that, right?

You need both of them. Having the field aligned in the structure when
the structure itself is misaligned will break in surprising ways.

It's possible that we don't ever need this for PCI hosts the way we
do need it for ethernet drivers.
 
> Anyway, it seems like there's very little contentious about this other
> than the minor issue of how to get at the private data. I'd like to make
> progress on this because we're now three generations into 64-bit with
> Tegra and still missing PCI support on all of them.


Yes, that would be good.

	Arnd
--
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
diff mbox

Patch

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 93583b389058..ecf543014da3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -521,16 +521,19 @@  static void pci_release_host_bridge_dev(struct device *dev)
 	kfree(bridge);
 }
 
-static struct pci_host_bridge *pci_alloc_host_bridge(void)
+static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
 {
 	struct pci_host_bridge *bridge;
 
-	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
+	bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
 	if (!bridge)
 		return NULL;
 
 	INIT_LIST_HEAD(&bridge->windows);
 
+	if (priv)
+		bridge->private = &bridge[1];
+
 	return bridge;
 }
 
@@ -2249,7 +2252,7 @@  static struct pci_bus *pci_create_root_bus_msi(struct device *parent,
 	int error;
 	struct pci_host_bridge *bridge;
 
-	bridge = pci_alloc_host_bridge();
+	bridge = pci_alloc_host_bridge(0);
 	if (!bridge)
 		return NULL;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ccf298fad9e7..3aa7240800c8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -421,6 +421,7 @@  struct pci_host_bridge {
 			resource_size_t start,
 			resource_size_t size,
 			resource_size_t align);
+	void *private;
 };
 
 #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)