Set the device's coherent_dma_mask to avoid a WARNING splat.
Please see commit 205e1b7f51e4 ("dma-mapping: warn when there is
no coherent_dma_mask").
Cc: [email protected]
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/net/ethernet/apple/macmace.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/apple/macmace.c b/drivers/net/ethernet/apple/macmace.c
index 137cbb470af2..98292c49ecf0 100644
--- a/drivers/net/ethernet/apple/macmace.c
+++ b/drivers/net/ethernet/apple/macmace.c
@@ -203,6 +203,10 @@ static int mace_probe(struct platform_device *pdev)
unsigned char checksum = 0;
int err;
+ err = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+ if (err)
+ return err;
+
dev = alloc_etherdev(PRIV_BYTES);
if (!dev)
return -ENOMEM;
--
2.16.1
Hi Finn,
On Thu, May 3, 2018 at 6:23 AM, Finn Thain <[email protected]> wrote:
> Set the device's coherent_dma_mask to avoid a WARNING splat.
> Please see commit 205e1b7f51e4 ("dma-mapping: warn when there is
> no coherent_dma_mask").
>
> Cc: [email protected]
> Tested-by: Stan Johnson <[email protected]>
> Signed-off-by: Finn Thain <[email protected]>
Thanks for your patch!
> --- a/drivers/net/ethernet/apple/macmace.c
> +++ b/drivers/net/ethernet/apple/macmace.c
> @@ -203,6 +203,10 @@ static int mace_probe(struct platform_device *pdev)
> unsigned char checksum = 0;
> int err;
>
> + err = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> + if (err)
> + return err;
> +
> dev = alloc_etherdev(PRIV_BYTES);
> if (!dev)
> return -ENOMEM;
Shouldn't this be handled in the platform code that instantiates the device,
i.e. in arch/m68k/mac/config.c:mac_platform_init()?
Cfr. commit f61e64310b75733d ("m68k: set dma and coherent masks for platform
FEC ethernets").
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Thu, 3 May 2018, Geert Uytterhoeven wrote:
> > --- a/drivers/net/ethernet/apple/macmace.c
> > +++ b/drivers/net/ethernet/apple/macmace.c
> > @@ -203,6 +203,10 @@ static int mace_probe(struct platform_device *pdev)
> > unsigned char checksum = 0;
> > int err;
> >
> > + err = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > + if (err)
> > + return err;
> > +
> > dev = alloc_etherdev(PRIV_BYTES);
> > if (!dev)
> > return -ENOMEM;
>
> Shouldn't this be handled in the platform code that instantiates the
> device, i.e. in arch/m68k/mac/config.c:mac_platform_init()?
>
I wondered about that too. The downside is that I'd have to convert
platform_device_register_simple() into platform_device_register() and add
all of the boilerplate that goes with that, for little gain.
> Cfr. commit f61e64310b75733d ("m68k: set dma and coherent masks for
> platform FEC ethernets").
>
Yes, I looked at that patch before I sent this one. It makes sense to set
the mask when defining the device since some devices tend to have inherent
limitations (but that's not really applicable here).
Moreover, it turns out that a number of platform drivers already call
dma_set_mask_and_coherent() or dma_coerce_mask_and_coherent() or similar.
I figured that platform drivers aren't expected to be particularly
portable. Well, I'd expect macmace and macsonic to be portable to NuBus
PowerMacs, but AFAIK the correct mask would remain DMA_BIT_MASK(32).
So that's how I ended up with this patch. But if you are not pursuaded by
my reasoning then just say the word and I'll take another approach.
--
> Gr{oetje,eeting}s,
>
> Geert
>
Hi Finn,
CC Christoph
On Thu, May 3, 2018 at 10:38 AM, Finn Thain <[email protected]> wrote:
> On Thu, 3 May 2018, Geert Uytterhoeven wrote:
>> > --- a/drivers/net/ethernet/apple/macmace.c
>> > +++ b/drivers/net/ethernet/apple/macmace.c
>> > @@ -203,6 +203,10 @@ static int mace_probe(struct platform_device *pdev)
>> > unsigned char checksum = 0;
>> > int err;
>> >
>> > + err = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>> > + if (err)
>> > + return err;
>> > +
>> > dev = alloc_etherdev(PRIV_BYTES);
>> > if (!dev)
>> > return -ENOMEM;
>>
>> Shouldn't this be handled in the platform code that instantiates the
>> device, i.e. in arch/m68k/mac/config.c:mac_platform_init()?
>
> I wondered about that too. The downside is that I'd have to convert
> platform_device_register_simple() into platform_device_register() and add
> all of the boilerplate that goes with that, for little gain.
>
>> Cfr. commit f61e64310b75733d ("m68k: set dma and coherent masks for
>> platform FEC ethernets").
>
> Yes, I looked at that patch before I sent this one. It makes sense to set
> the mask when defining the device since some devices tend to have inherent
> limitations (but that's not really applicable here).
>
> Moreover, it turns out that a number of platform drivers already call
> dma_set_mask_and_coherent() or dma_coerce_mask_and_coherent() or similar.
>
> I figured that platform drivers aren't expected to be particularly
> portable. Well, I'd expect macmace and macsonic to be portable to NuBus
> PowerMacs, but AFAIK the correct mask would remain DMA_BIT_MASK(32).
>
> So that's how I ended up with this patch. But if you are not pursuaded by
> my reasoning then just say the word and I'll take another approach.
Perhaps you can add a new helper (platform_device_register_simple_dma()?)
that takes the DMA mask, too?
With people setting the mask to kill the WARNING splat, this may become
more common.
struct platform_device_info already has a dma_mask field, but
platform_device_register_resndata() explicitly sets it to zero.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Thu, May 03, 2018 at 10:46:56AM +0200, Geert Uytterhoeven wrote:
> Perhaps you can add a new helper (platform_device_register_simple_dma()?)
> that takes the DMA mask, too?
> With people setting the mask to kill the WARNING splat, this may become
> more common.
>
> struct platform_device_info already has a dma_mask field, but
> platform_device_register_resndata() explicitly sets it to zero.
Yes, that would be useful. The other assumption could be that
platform devices always allow an all-0xff dma mask.
Hi Christoph,
On Thu, May 3, 2018 at 8:51 PM, Christoph Hellwig <[email protected]> wrote:
> On Thu, May 03, 2018 at 10:46:56AM +0200, Geert Uytterhoeven wrote:
>> Perhaps you can add a new helper (platform_device_register_simple_dma()?)
>> that takes the DMA mask, too?
>> With people setting the mask to kill the WARNING splat, this may become
>> more common.
>>
>> struct platform_device_info already has a dma_mask field, but
>> platform_device_register_resndata() explicitly sets it to zero.
>
> Yes, that would be useful. The other assumption could be that
> platform devices always allow an all-0xff dma mask.
That's not always true (Atari NCR5380 SCSI and floppy would use a 24
bit DMA mask). We use bounce buffers allocated from a dedicated lowmem
pool there currently, and for all I know don't use the DMA API yet.
I bet that is a rare exception though. Setting the default DMA mask
for platform devices to all-0xff and letting the few odd drivers force
a different setting seems the best way forward.
Cheers,
Michael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Michael,
On Thu, May 3, 2018 at 10:24 PM, Michael Schmitz <[email protected]> wrote:
> On Thu, May 3, 2018 at 8:51 PM, Christoph Hellwig <[email protected]> wrote:
>> On Thu, May 03, 2018 at 10:46:56AM +0200, Geert Uytterhoeven wrote:
>>> Perhaps you can add a new helper (platform_device_register_simple_dma()?)
>>> that takes the DMA mask, too?
>>> With people setting the mask to kill the WARNING splat, this may become
>>> more common.
>>>
>>> struct platform_device_info already has a dma_mask field, but
>>> platform_device_register_resndata() explicitly sets it to zero.
>>
>> Yes, that would be useful. The other assumption could be that
>> platform devices always allow an all-0xff dma mask.
>
> That's not always true (Atari NCR5380 SCSI and floppy would use a 24
> bit DMA mask). We use bounce buffers allocated from a dedicated lowmem
> pool there currently, and for all I know don't use the DMA API yet.
>
> I bet that is a rare exception though. Setting the default DMA mask
> for platform devices to all-0xff and letting the few odd drivers force
> a different setting seems the best way forward.
I'd say that's usually a property of the platform, not of the device?
So IMHO it belongs in the platform code, not in the device driver code.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi Geert,
Am 04.05.2018 um 19:24 schrieb Geert Uytterhoeven:
> Hi Michael,
>
>>> Yes, that would be useful. The other assumption could be that
>>> platform devices always allow an all-0xff dma mask.
>>
>> That's not always true (Atari NCR5380 SCSI and floppy would use a 24
>> bit DMA mask). We use bounce buffers allocated from a dedicated lowmem
>> pool there currently, and for all I know don't use the DMA API yet.
>>
>> I bet that is a rare exception though. Setting the default DMA mask
>> for platform devices to all-0xff and letting the few odd drivers force
>> a different setting seems the best way forward.
>
> I'd say that's usually a property of the platform, not of the device?
Right - I was thinking 'm68k' as platform, not a particular machine like
Mac or Falcon (the 24 bit mask only applies to that particular model
anyway).
> So IMHO it belongs in the platform code, not in the device driver code.
OK - let's have a default mask of 64 bit, and allow machine specific
platform_init() to override using a new helper function.
Cheers,
Michael
> Gr{oetje,eeting}s,
>
> Geert
>
On Thu, 3 May 2018, Geert Uytterhoeven wrote:
>
> Perhaps you can add a new helper
> (platform_device_register_simple_dma()?) that takes the DMA mask, too?
Would there be enough potential callers in future to justify that API?
It seems that there haven't been many in the past. I found four users of
platform_device_register_simple() which might benefit. Mostly these call
dma_set_coherent_mask() in the platform driver probe routine.
drivers/gpu/drm/etnaviv/etnaviv_drv.c
drivers/gpu/drm/exynos/exynos_drm_drv.c
drivers/gpu/drm/omapdrm/omap_drv.c
drivers/parport/parport_pc.c
(Am I missing any others?)
To actually hoist the dma mask setup out of existing platform drivers
would have implications for every device that matches with those drivers.
That's a bit risky since I can't test those devices -- that's assuming I
could identify them all; sometimes platform device matching is not well
defined at build time (see loongson_sysconf.ecname).
So far, it looks like macmace and macsonic would be the only callers of
this new API call.
What's worse, if you do pass a dma_mask in struct platform_device_info,
you end up with this problem in platform_device_register_full():
if (pdevinfo->dma_mask) {
/*
* This memory isn't freed when the device is put,
* I don't have a nice idea for that though. Conceptually
* dma_mask in struct device should not be a pointer.
* See http://thread.gmane.org/gmane.linux.kernel.pci/9081
*/
pdev->dev.dma_mask =
kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
Most of the platform drivers that call dma_coerce_mask_and_coherent() are
using pdev->of_match_table, not platform_device_register_simple(). Many of
them have a comment like this:
/*
* Right now device-tree probed devices don't get dma_mask set.
* Since shared usb code relies on it, set it here for now.
* Once we have dma capability bindings this can go away.
*/
> With people setting the mask to kill the WARNING splat, this may become
> more common.
Since the commit which introduced the WARNING, only commits f61e64310b75
("m68k: set dma and coherent masks for platform FEC ethernets") and
7bcfab202ca7 ("powerpc/macio: set a proper dma_coherent_mask") seem to be
aimed at squelching that WARNING.
(Am I missing any others?)
So far, this is not looking like a common problem, and I'm having trouble
finding some way to improve on my original patches.
--
On Thu, 3 May 2018, Christoph Hellwig wrote:
> On Thu, May 03, 2018 at 10:46:56AM +0200, Geert Uytterhoeven wrote:
> > Perhaps you can add a new helper
> > (platform_device_register_simple_dma()?) that takes the DMA mask, too?
> > With people setting the mask to kill the WARNING splat, this may
> > become more common.
> >
> > struct platform_device_info already has a dma_mask field, but
> > platform_device_register_resndata() explicitly sets it to zero.
>
> Yes, that would be useful. The other assumption could be that platform
> devices always allow an all-0xff dma mask.
Could that have unintended side effects? The mask is presently unset by
default, and my worry would be that changing it may cause some drivers to
behave differently.
A quick grep turns up this in drivers/spi/spi-au1550.c for example,
if (pdev->dev.dma_mask == NULL)
dev_warn(&pdev->dev, "no dma mask\n");
else
hw->usedma = 1;
Also, if pdev.dev.dma_mask is to get a default value, shouldn't it use the
same default as dma_get_mask, to avoid unintended side effects?
static inline u64 dma_get_mask(struct device *dev)
{
if (dev && dev->dma_mask && *dev->dma_mask)
return *dev->dma_mask;
return DMA_BIT_MASK(32);
}
--
Hi Finn,
On Thu, May 10, 2018 at 1:25 PM, Finn Thain <[email protected]> wrote:
> On Thu, 3 May 2018, Geert Uytterhoeven wrote:
>
>>
>> Perhaps you can add a new helper
>> (platform_device_register_simple_dma()?) that takes the DMA mask, too?
[...]
> To actually hoist the dma mask setup out of existing platform drivers
> would have implications for every device that matches with those drivers.
>
> That's a bit risky since I can't test those devices -- that's assuming I
> could identify them all; sometimes platform device matching is not well
> defined at build time (see loongson_sysconf.ecname).
>
> So far, it looks like macmace and macsonic would be the only callers of
> this new API call.
>
> What's worse, if you do pass a dma_mask in struct platform_device_info,
> you end up with this problem in platform_device_register_full():
>
> if (pdevinfo->dma_mask) {
> /*
> * This memory isn't freed when the device is put,
> * I don't have a nice idea for that though. Conceptually
> * dma_mask in struct device should not be a pointer.
> * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
> */
> pdev->dev.dma_mask =
> kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
Maybe platform_device_register_full() should rather check whether
dev.coherent_dma_mask is set, and make dev.dma_mask point to that?
This is how we solved the warning issue for the Zorro bus devices...
(8614f1b58bd0e920a5859464a500b93152c5f8b1)
Not sure what the ramifications of that change would be in the general
case (i.e. platforms where coherent and non-coherent DMA operations
must use different masks). I'd hope all those platforms explicitly set
up their DMA masks anyway.
Your other comment regarding the default used by dma_get_mask() is spot on.
> Most of the platform drivers that call dma_coerce_mask_and_coherent() are
> using pdev->of_match_table, not platform_device_register_simple(). Many of
> them have a comment like this:
>
> /*
> * Right now device-tree probed devices don't get dma_mask set.
> * Since shared usb code relies on it, set it here for now.
> * Once we have dma capability bindings this can go away.
> */
>
>> With people setting the mask to kill the WARNING splat, this may become
>> more common.
>
> Since the commit which introduced the WARNING, only commits f61e64310b75
> ("m68k: set dma and coherent masks for platform FEC ethernets") and
> 7bcfab202ca7 ("powerpc/macio: set a proper dma_coherent_mask") seem to be
> aimed at squelching that WARNING.
>
> (Am I missing any others?)
Zorro devices :-) Which begs the question: why can' you set up all
Nubus bus devices' DMA masks in nubus_device_register(), or
nubus_add_board()?
> So far, this is not looking like a common problem, and I'm having trouble
> finding some way to improve on my original patches.
Putting this in the core platform device code might have too many
unintended side effects. Platform specific bus drivers or device
drivers might be a safer place to put this. Makes it harder for
Christoph to find all instances of such workarounds though.
Cheers,
Michael
On Fri, 11 May 2018, Michael Schmitz wrote:
> > > Perhaps you can add a new helper
> > > (platform_device_register_simple_dma()?) that takes the DMA mask,
> > > too?
...
> >
> > So far, it looks like macmace and macsonic would be the only callers
> > of this new API call.
> >
> > What's worse, if you do pass a dma_mask in struct
> > platform_device_info, you end up with this problem in
> > platform_device_register_full():
> >
> > if (pdevinfo->dma_mask) {
> > /*
> > * This memory isn't freed when the device is put,
> > * I don't have a nice idea for that though. Conceptually
> > * dma_mask in struct device should not be a pointer.
> > * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
> > */
> > pdev->dev.dma_mask =
> > kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
>
> Maybe platform_device_register_full() should rather check whether
> dev.coherent_dma_mask is set, and make dev.dma_mask point to that? This
> is how we solved the warning issue for the Zorro bus devices...
> (8614f1b58bd0e920a5859464a500b93152c5f8b1)
>
The claim in the comment above that a pointer is the wrong solution
suggests that your proposal won't get far. Also, your proposal doesn't
address the other issues I raised: a new
platform_device_register_simple_dma() API would only have two callers, and
the dma mask setup for device-tree probed platform devices is apparently a
work-in-progress (which I don't want to churn up).
> > > With people setting the mask to kill the WARNING splat, this may
> > > become more common.
> >
> > Since the commit which introduced the WARNING, only commits f61e64310b75
> > ("m68k: set dma and coherent masks for platform FEC ethernets") and
> > 7bcfab202ca7 ("powerpc/macio: set a proper dma_coherent_mask") seem to be
> > aimed at squelching that WARNING.
> >
> > (Am I missing any others?)
>
> Zorro devices :-)
Right, I should add commit 55496d3fe2ac ("zorro: Set up z->dev.dma_mask
for the DMA API") to that list.
> Which begs the question: why can' you set up all Nubus bus devices' DMA
> masks in nubus_device_register(), or nubus_add_board()?
I am expecting to see the same WARNING from the nubus sonic driver but it
hasn't happened yet, so I don't have a patch for it yet. In anycase, the
nubus fix would be a lot like the zorro bus fix, so I don't see a problem.
--
Hi Finn,
On Fri, May 11, 2018 at 11:55 AM, Finn Thain <[email protected]> wrote:
>> > What's worse, if you do pass a dma_mask in struct
>> > platform_device_info, you end up with this problem in
>> > platform_device_register_full():
>> >
>> > if (pdevinfo->dma_mask) {
>> > /*
>> > * This memory isn't freed when the device is put,
>> > * I don't have a nice idea for that though. Conceptually
>> > * dma_mask in struct device should not be a pointer.
>> > * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
>> > */
>> > pdev->dev.dma_mask =
>> > kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
>>
>> Maybe platform_device_register_full() should rather check whether
>> dev.coherent_dma_mask is set, and make dev.dma_mask point to that? This
>> is how we solved the warning issue for the Zorro bus devices...
>> (8614f1b58bd0e920a5859464a500b93152c5f8b1)
>>
>
> The claim in the comment above that a pointer is the wrong solution
> suggests that your proposal won't get far. Also, your proposal doesn't
I read the comment to be mostly concerned about not freeing memory,
and attempted to address that. I won't pretend it's the right thing to
do if the pointer will go away anyway, and I certainly won't submit a
patch. Sorry for muddling the issue.
> address the other issues I raised: a new
> platform_device_register_simple_dma() API would only have two callers, and
> the dma mask setup for device-tree probed platform devices is apparently a
> work-in-progress (which I don't want to churn up).
Yes, and that's why I would prefer your old patch handling this in the
device driver (which Geert didn't like), or in the alternative to set
the mask up when registering a device with its bus where appropriate.
I concede this won't help with pure platform devices but as we can't
test all these, we should leave the fix for platfoem devices up to
Christoph.
>
>> > > With people setting the mask to kill the WARNING splat, this may
>> > > become more common.
>> >
>> > Since the commit which introduced the WARNING, only commits f61e64310b75
>> > ("m68k: set dma and coherent masks for platform FEC ethernets") and
>> > 7bcfab202ca7 ("powerpc/macio: set a proper dma_coherent_mask") seem to be
>> > aimed at squelching that WARNING.
>> >
>> > (Am I missing any others?)
>>
>> Zorro devices :-)
>
> Right, I should add commit 55496d3fe2ac ("zorro: Set up z->dev.dma_mask
> for the DMA API") to that list.
>
>> Which begs the question: why can' you set up all Nubus bus devices' DMA
>> masks in nubus_device_register(), or nubus_add_board()?
>
> I am expecting to see the same WARNING from the nubus sonic driver but it
> hasn't happened yet, so I don't have a patch for it yet. In anycase, the
> nubus fix would be a lot like the zorro bus fix, so I don't see a problem.
That's odd. But what I meant to say is that by setting up
dma_coherent_mask in nubus_add_board(), and pointing dma_mask to that,
ypu won't need any patches to Nubus device drivers.
I must be missing something else...
Cheers,
Michael
>
> --
On Fri, 11 May 2018, Michael Schmitz wrote:
> > > Which begs the question: why can' you set up all Nubus bus devices'
> > > DMA masks in nubus_device_register(), or nubus_add_board()?
> >
> > I am expecting to see the same WARNING from the nubus sonic driver but
> > it hasn't happened yet, so I don't have a patch for it yet. In
> > anycase, the nubus fix would be a lot like the zorro bus fix, so I
> > don't see a problem.
>
> That's odd. But what I meant to say is that by setting up
> dma_coherent_mask in nubus_add_board(), and pointing dma_mask to that,
> ypu won't need any patches to Nubus device drivers.
Right. I think I've already acknowledged that. But it's off-topic, because
the patches under review are for platform drivers. Those patches fix an
actual bug that I've observed. Whereas, the nubus driver dma mask issue
that you raised is purely theoretical at this stage.
--
Hi Finn,
Am 11.05.2018 um 15:28 schrieb Finn Thain:
> On Fri, 11 May 2018, Michael Schmitz wrote:
>
>>>> Which begs the question: why can' you set up all Nubus bus devices'
>>>> DMA masks in nubus_device_register(), or nubus_add_board()?
>>>
>>> I am expecting to see the same WARNING from the nubus sonic driver but
>>> it hasn't happened yet, so I don't have a patch for it yet. In
>>> anycase, the nubus fix would be a lot like the zorro bus fix, so I
>>> don't see a problem.
>>
>> That's odd. But what I meant to say is that by setting up
>> dma_coherent_mask in nubus_add_board(), and pointing dma_mask to that,
>> ypu won't need any patches to Nubus device drivers.
>
> Right. I think I've already acknowledged that. But it's off-topic, because
> the patches under review are for platform drivers. Those patches fix an
> actual bug that I've observed. Whereas, the nubus driver dma mask issue
> that you raised is purely theoretical at this stage.
I had lost track of the fact that macsonic can be probed as either Nubus
or platform device. Sorry for the noise.
I'm afraid using platform_device_register() (which you already use for
the SCC devices) is the only option handling this on a per-device basis
without touching platform core code, while at the same time keeping the
DMA mask setup out of device drivers (I can see Geert's point there -
device driver code might be shared across implementations of the device
on platforms with different DMA mask requirements,, something the driver
can't be expected to know about).
Cheers,
Michael
On Fri, 11 May 2018, Michael Schmitz wrote:
>
> I'm afraid using platform_device_register() (which you already use for
> the SCC devices) is the only option handling this on a per-device basis
> without touching platform core code, while at the same time keeping the
> DMA mask setup out of device drivers
I don't think that will fly. If you call platform_device_register() and
follow that with a dma mask assignment, you could race with the bus
matching and driver probe, and we are back to the same WARNING message.
If you want to use platform_device_register(), you'd have to implement
arch_setup_pdev_archdata() and use that to set up the dma mask.
> (I can see Geert's point there - device driver code might be shared
> across implementations of the device on platforms with different DMA
> mask requirements,, something the driver can't be expected to know
> about).
As I said, these drivers might be expected to be portable between Macs and
early PowerMacs, but the same dma mask would apply AFAIK.
If a platform driver isn't expected to be portable, I think either method
is reasonable: arch_setup_pdev_archdata() or the method in the patch.
Anyway, there is this in arch/powerpc/kernel/setup-common.c:
void arch_setup_pdev_archdata(struct platform_device *pdev)
{
pdev->archdata.dma_mask = DMA_BIT_MASK(32);
pdev->dev.dma_mask = &pdev->archdata.dma_mask;
...
I'm inclined to propose something similar for m68k. That should fix the
problem, since arch_setup_pdev_archdata() is already in the call chain:
platform_device_register_simple()
platform_device_register_resndata()
platform_device_register_full()
platform_device_alloc()
arch_setup_pdev_archdata()
Thoughts? Will this have nasty side effects for m68k platforms that use
smaller dma masks?
--
>
> Cheers,
>
> Michael
>
Hi Finn,
Am 11.05.2018 um 17:28 schrieb Finn Thain:
> On Fri, 11 May 2018, Michael Schmitz wrote:
>
>>
>> I'm afraid using platform_device_register() (which you already use for
>> the SCC devices) is the only option handling this on a per-device basis
>> without touching platform core code, while at the same time keeping the
>> DMA mask setup out of device drivers
>
> I don't think that will fly. If you call platform_device_register() and
> follow that with a dma mask assignment, you could race with the bus
> matching and driver probe, and we are back to the same WARNING message.
I wasn't proposing to follow platform_device_register() with the mask
assignment, but rather to use the same strategy from the Coldfire FEC
patch (f61e64310b75): set pdev.dev.coherent_dma_mask and
pdev.dev.dma_mask _before_ calling platform_device_register().
> If you want to use platform_device_register(), you'd have to implement
> arch_setup_pdev_archdata() and use that to set up the dma mask.
If you want to avoid the overhead of defining the macmace platform
device and using platform_device_register() ... seeing as you would not
be defining just the DMA mask and nothing else, that's probably the
least effort for the MACE and SONIC drivers.
>> (I can see Geert's point there - device driver code might be shared
>> across implementations of the device on platforms with different DMA
>> mask requirements,, something the driver can't be expected to know
>> about).
>
> As I said, these drivers might be expected to be portable between Macs and
> early PowerMacs, but the same dma mask would apply AFAIK.
>
> If a platform driver isn't expected to be portable, I think either method
> is reasonable: arch_setup_pdev_archdata() or the method in the patch.
>
> Anyway, there is this in arch/powerpc/kernel/setup-common.c:
>
> void arch_setup_pdev_archdata(struct platform_device *pdev)
> {
> pdev->archdata.dma_mask = DMA_BIT_MASK(32);
> pdev->dev.dma_mask = &pdev->archdata.dma_mask;
> ...
You would have to be careful not to overwrite a pdev->dev.dma_mask and
pdev->dev.dma_coherent_mask that might have been set in a platform
device passed via platform_device_register here. Coldfire is the only
m68k platform currently using that, but there might be others in future.
> I'm inclined to propose something similar for m68k. That should fix the
> problem, since arch_setup_pdev_archdata() is already in the call chain:
>
> platform_device_register_simple()
> platform_device_register_resndata()
> platform_device_register_full()
> platform_device_alloc()
> arch_setup_pdev_archdata()
>
> Thoughts? Will this have nasty side effects for m68k platforms that use
> smaller dma masks?
These can still set a smaller mask in pdev->dev directly and use
platform_device_register() instead. But I don't think there are smaller
DMA masks used by m68k drivers that use the platform device mechanism at
present. I've only looked at arch/m68k though.
Cheers,
Michael
On Fri, 11 May 2018, Michael Schmitz wrote:
>
> I wasn't proposing to follow platform_device_register() with the mask
> assignment,
I see.
> but rather to use the same strategy from the Coldfire FEC patch
> (f61e64310b75): set pdev.dev.coherent_dma_mask and pdev.dev.dma_mask
> _before_ calling platform_device_register().
>
That patch is attempting to solve the same problem using a different
approach, but I'd prefer to keep using platform_device_register_simple().
>
> If you want to avoid the overhead of defining the macmace platform
> device and using platform_device_register() ... seeing as you would not
> be defining just the DMA mask and nothing else, that's probably the
> least effort for the MACE and SONIC drivers.
>
I don't mind the effort. I want to avoid all the boilerplate.
>
> You would have to be careful not to overwrite a pdev->dev.dma_mask and
> pdev->dev.dma_coherent_mask that might have been set in a platform
> device passed via platform_device_register here. Coldfire is the only
> m68k platform currently using that, but there might be others in future.
>
That Coldfire patch could be reverted if this is a better solution.
> ... But I don't think there are smaller DMA masks used by m68k drivers
> that use the platform device mechanism at present. I've only looked at
> arch/m68k though.
So we're back at the same problem that Geert's suggestion also raised: how
to identify potentially affected platform devices and drivers?
Maybe we can take a leaf out of Christoph's book, and leave a noisy
WARNING splat in the log.
void arch_setup_pdev_archdata(struct platform_device *pdev)
{
WARN_ON_ONCE(pdev->dev.coherent_dma_mask != DMA_MASK_NONE ||
pdev->dev.dma_mask != NULL);
pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
}
--
Hi Finn,
Am 11.05.2018 um 22:06 schrieb Finn Thain:
>> You would have to be careful not to overwrite a pdev->dev.dma_mask and
>> pdev->dev.dma_coherent_mask that might have been set in a platform
>> device passed via platform_device_register here. Coldfire is the only
>> m68k platform currently using that, but there might be others in future.
>>
>
> That Coldfire patch could be reverted if this is a better solution.
True, but there might be other uses for deviating from a platform
default (I'm thinking of Atari SCSI and floppy drivers here). But we
could chose the correct mask to set in arch_setup_pdev_archdata()
instead, as it's a platform property not a driver property in that case.
>> ... But I don't think there are smaller DMA masks used by m68k drivers
>> that use the platform device mechanism at present. I've only looked at
>> arch/m68k though.
>
> So we're back at the same problem that Geert's suggestion also raised: how
> to identify potentially affected platform devices and drivers?
>
> Maybe we can take a leaf out of Christoph's book, and leave a noisy
> WARNING splat in the log.
>
> void arch_setup_pdev_archdata(struct platform_device *pdev)
> {
> WARN_ON_ONCE(pdev->dev.coherent_dma_mask != DMA_MASK_NONE ||
> pdev->dev.dma_mask != NULL);
I'd suggest using WARN_ON() so we catch all uses on a particular platform.
I initially thought it necessary to warn on unset mask here, but I see
that would throw up a lot of redundant false positives.
Cheers,
Michael