Re: [PATCH 6/6] driver core: initialize a default DMA mask for platform device
Hi Christoph,
On Fri, Aug 16, 2019 at 8:30 AM 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_device so that we can initialize
> the dma_mask pointer in struct device and initialize both masks to
> 32-bits by default, replacing similar functionality in m68k and
> powerpc. The arch_setup_pdev_archdata hooks is now unused and removed.
>
> 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
> ---
> arch/m68k/kernel/dma.c | 9 ---
Acked-by: Geert Uytterhoeven
> arch/sh/boards/mach-ecovec24/setup.c | 2 --
> arch/sh/boards/mach-migor/setup.c| 1 -
Acked-by: Geert Uytterhoeven
given "[PATCH 0/2] Remove calls to empty arch_setup_pdev_archdata()"
https://lore.kernel.org/linux-renesas-soc/1526641611-2769-1-git-send-email-geert+rene...@glider.be/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 6/6] driver core: initialize a default DMA mask for platform device
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;
> > > boolid_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
Re: [PATCH 6/6] driver core: initialize a default DMA mask for platform device
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;
> > boolid_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.
Re: [PATCH 6/6] driver core: initialize a default DMA mask for platform device
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 = >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.
Re: [PATCH 6/6] driver core: initialize a default DMA mask for platform device
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
> ---
> 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 = >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(>pdev.dev);
> pa->pdev.dev.release = platform_device_release;
> - arch_setup_pdev_archdata(>pdev);
> + setup_pdev_archdata(>pdev);
> }
>
> return pa ? >pdev : NULL;
> @@ -512,7 +523,7 @@ EXPORT_SYMBOL_GPL(platform_device_del);
> int platform_device_register(struct platform_device *pdev)
> {
> device_initialize(>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;
> boolid_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
Re: [PATCH 6/6] driver core: initialize a default DMA mask for platform device
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
---
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 = >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(>pdev.dev);
pa->pdev.dev.release = platform_device_release;
- arch_setup_pdev_archdata(>pdev);
+ setup_pdev_archdata(>pdev);
}
return pa ? >pdev : NULL;
@@ -512,7 +523,7 @@ EXPORT_SYMBOL_GPL(platform_device_del);
int platform_device_register(struct platform_device *pdev)
{
device_initialize(>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;
boolid_auto;
struct device dev;
+ u64 dma_mask;
u32 num_resources;
struct resource *resource;
