| Message ID | 20190811080520.21712-7-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] usb: don't create dma pools for HCDs with a localmem_pool | expand |
On 11/08/2019 09:05, Christoph Hellwig wrote: > We still treat devices without a DMA mask as defaulting to 32-bits for > both mask, but a few releases ago we've started warning about such > cases, as they require special cases to work around this sloppyness. > Add a dma_mask field to struct platform_object so that we can initialize s/object/device/ > the dma_mask pointer in struct device and initialize both masks to > 32-bits by default. Architectures can still override this in > arch_setup_pdev_archdata if needed. > > Note that the code looks a little odd with the various conditionals > because we have to support platform_device structures that are > statically allocated. This would be a good point to also get rid of the long-standing bodge in platform_device_register_full(). > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/base/platform.c | 15 +++++++++++++-- > include/linux/platform_device.h | 1 + > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index ec974ba9c0c4..b216fcb0a8af 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -264,6 +264,17 @@ struct platform_object { > char name[]; > }; > > +static void setup_pdev_archdata(struct platform_device *pdev) Bikeshed: painting the generic DMA API properties as "archdata" feels a bit off-target :/ > +{ > + if (!pdev->dev.coherent_dma_mask) > + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > + if (!pdev->dma_mask) > + pdev->dma_mask = DMA_BIT_MASK(32); > + if (!pdev->dev.dma_mask) > + pdev->dev.dma_mask = &pdev->dma_mask; > + arch_setup_pdev_archdata(pdev); AFAICS m68k's implementation of that arch hook becomes entirely redundant after this change, so may as well go. That would just leave powerpc's actual archdata, which at a glance looks like it could probably be cleaned up with not *too* much trouble. Robin. > +}; > + > /** > * platform_device_put - destroy a platform device > * @pdev: platform device to free > @@ -310,7 +321,7 @@ struct platform_device *platform_device_alloc(const char *name, int id) > pa->pdev.id = id; > device_initialize(&pa->pdev.dev); > pa->pdev.dev.release = platform_device_release; > - arch_setup_pdev_archdata(&pa->pdev); > + setup_pdev_archdata(&pa->pdev); > } > > return pa ? &pa->pdev : NULL; > @@ -512,7 +523,7 @@ EXPORT_SYMBOL_GPL(platform_device_del); > int platform_device_register(struct platform_device *pdev) > { > device_initialize(&pdev->dev); > - arch_setup_pdev_archdata(pdev); > + setup_pdev_archdata(pdev); > return platform_device_add(pdev); > } > EXPORT_SYMBOL_GPL(platform_device_register); > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 9bc36b589827..a2abde2aef25 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -24,6 +24,7 @@ struct platform_device { > int id; > bool id_auto; > struct device dev; > + u64 dma_mask; > u32 num_resources; > struct resource *resource; > >
On Sun, Aug 11, 2019 at 10:05:20AM +0200, Christoph Hellwig wrote: > We still treat devices without a DMA mask as defaulting to 32-bits for > both mask, but a few releases ago we've started warning about such > cases, as they require special cases to work around this sloppyness. > Add a dma_mask field to struct platform_object so that we can initialize > the dma_mask pointer in struct device and initialize both masks to > 32-bits by default. Architectures can still override this in > arch_setup_pdev_archdata if needed. > > Note that the code looks a little odd with the various conditionals > because we have to support platform_device structures that are > statically allocated. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/base/platform.c | 15 +++++++++++++-- > include/linux/platform_device.h | 1 + > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index ec974ba9c0c4..b216fcb0a8af 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -264,6 +264,17 @@ struct platform_object { > char name[]; > }; > > +static void setup_pdev_archdata(struct platform_device *pdev) > +{ > + if (!pdev->dev.coherent_dma_mask) > + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > + if (!pdev->dma_mask) > + pdev->dma_mask = DMA_BIT_MASK(32); > + if (!pdev->dev.dma_mask) > + pdev->dev.dma_mask = &pdev->dma_mask; > + arch_setup_pdev_archdata(pdev); > +}; > + > /** > * platform_device_put - destroy a platform device > * @pdev: platform device to free > @@ -310,7 +321,7 @@ struct platform_device *platform_device_alloc(const char *name, int id) > pa->pdev.id = id; > device_initialize(&pa->pdev.dev); > pa->pdev.dev.release = platform_device_release; > - arch_setup_pdev_archdata(&pa->pdev); > + setup_pdev_archdata(&pa->pdev); > } > > return pa ? &pa->pdev : NULL; > @@ -512,7 +523,7 @@ EXPORT_SYMBOL_GPL(platform_device_del); > int platform_device_register(struct platform_device *pdev) > { > device_initialize(&pdev->dev); > - arch_setup_pdev_archdata(pdev); > + setup_pdev_archdata(pdev); > return platform_device_add(pdev); > } > EXPORT_SYMBOL_GPL(platform_device_register); > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 9bc36b589827..a2abde2aef25 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -24,6 +24,7 @@ struct platform_device { > int id; > bool id_auto; > struct device dev; > + u64 dma_mask; Why is the dma_mask in 'struct device' which is part of this structure, not sufficient here? Shouldn't the "platform" be setting that up correctly already in the "archdata" type callback? confused, greg k-h
On Wed, Aug 14, 2019 at 04:49:13PM +0100, Robin Murphy wrote: >> because we have to support platform_device structures that are >> statically allocated. > > This would be a good point to also get rid of the long-standing bodge in > platform_device_register_full(). platform_device_register_full looks odd to start with, especially as the coumentation is rather lacking.. >> +static void setup_pdev_archdata(struct platform_device *pdev) > > Bikeshed: painting the generic DMA API properties as "archdata" feels a bit > off-target :/ > >> +{ >> + if (!pdev->dev.coherent_dma_mask) >> + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); >> + if (!pdev->dma_mask) >> + pdev->dma_mask = DMA_BIT_MASK(32); >> + if (!pdev->dev.dma_mask) >> + pdev->dev.dma_mask = &pdev->dma_mask; >> + arch_setup_pdev_archdata(pdev); > > AFAICS m68k's implementation of that arch hook becomes entirely redundant > after this change, so may as well go. That would just leave powerpc's > actual archdata, which at a glance looks like it could probably be cleaned > up with not *too* much trouble. Actually I think we can just kill both off. At the point archdata is indeed entirely misnamed.
On Thu, Aug 15, 2019 at 03:03:25PM +0200, Greg Kroah-Hartman wrote: > > --- a/include/linux/platform_device.h > > +++ b/include/linux/platform_device.h > > @@ -24,6 +24,7 @@ struct platform_device { > > int id; > > bool id_auto; > > struct device dev; > > + u64 dma_mask; > > Why is the dma_mask in 'struct device' which is part of this structure, > not sufficient here? Shouldn't the "platform" be setting that up > correctly already in the "archdata" type callback? Becaus the dma_mask in struct device is a pointer that needs to point to something, and this is the best space we can allocate for 'something'. m68k and powerpc currently do something roughly equivalent at the moment, while everyone else just has horrible, horrible hacks. As mentioned in the changelog the intent of this patch is that we treat platform devices like any other bus, where the bus allocates the space for the dma_mask. The long term plan is to eventually kill that weird pointer indirection that doesn't help anyone, but for that we need to sort out the basics first.
On Thu, Aug 15, 2019 at 03:38:12PM +0200, Christoph Hellwig wrote: > On Thu, Aug 15, 2019 at 03:03:25PM +0200, Greg Kroah-Hartman wrote: > > > --- a/include/linux/platform_device.h > > > +++ b/include/linux/platform_device.h > > > @@ -24,6 +24,7 @@ struct platform_device { > > > int id; > > > bool id_auto; > > > struct device dev; > > > + u64 dma_mask; > > > > Why is the dma_mask in 'struct device' which is part of this structure, > > not sufficient here? Shouldn't the "platform" be setting that up > > correctly already in the "archdata" type callback? > > Becaus the dma_mask in struct device is a pointer that needs to point > to something, and this is the best space we can allocate for 'something'. > m68k and powerpc currently do something roughly equivalent at the moment, > while everyone else just has horrible, horrible hacks. As mentioned in > the changelog the intent of this patch is that we treat platform devices > like any other bus, where the bus allocates the space for the dma_mask. > The long term plan is to eventually kill that weird pointer indirection > that doesn't help anyone, but for that we need to sort out the basics > first. Ah, missed that, sorry. Ok, no objection from me. Might as well respin this series and I can queue it up after 5.3-rc5 is out (which will have your first 2 patches in it.) thanks, greg k-h
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index ec974ba9c0c4..b216fcb0a8af 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -264,6 +264,17 @@ struct platform_object { char name[]; }; +static void setup_pdev_archdata(struct platform_device *pdev) +{ + if (!pdev->dev.coherent_dma_mask) + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); + if (!pdev->dma_mask) + pdev->dma_mask = DMA_BIT_MASK(32); + if (!pdev->dev.dma_mask) + pdev->dev.dma_mask = &pdev->dma_mask; + arch_setup_pdev_archdata(pdev); +}; + /** * platform_device_put - destroy a platform device * @pdev: platform device to free @@ -310,7 +321,7 @@ struct platform_device *platform_device_alloc(const char *name, int id) pa->pdev.id = id; device_initialize(&pa->pdev.dev); pa->pdev.dev.release = platform_device_release; - arch_setup_pdev_archdata(&pa->pdev); + setup_pdev_archdata(&pa->pdev); } return pa ? &pa->pdev : NULL; @@ -512,7 +523,7 @@ EXPORT_SYMBOL_GPL(platform_device_del); int platform_device_register(struct platform_device *pdev) { device_initialize(&pdev->dev); - arch_setup_pdev_archdata(pdev); + setup_pdev_archdata(pdev); return platform_device_add(pdev); } EXPORT_SYMBOL_GPL(platform_device_register); diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 9bc36b589827..a2abde2aef25 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -24,6 +24,7 @@ struct platform_device { int id; bool id_auto; struct device dev; + u64 dma_mask; u32 num_resources; struct resource *resource;
We still treat devices without a DMA mask as defaulting to 32-bits for both mask, but a few releases ago we've started warning about such cases, as they require special cases to work around this sloppyness. Add a dma_mask field to struct platform_object so that we can initialize the dma_mask pointer in struct device and initialize both masks to 32-bits by default. Architectures can still override this in arch_setup_pdev_archdata if needed. Note that the code looks a little odd with the various conditionals because we have to support platform_device structures that are statically allocated. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/base/platform.c | 15 +++++++++++++-- include/linux/platform_device.h | 1 + 2 files changed, 14 insertions(+), 2 deletions(-)