Message ID | 20160815153647.1075-2-thierry.reding@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Delegated to: | Bjorn Helgaas |
Headers | show |
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
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
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
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
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 --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)