LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Michael Schmitz <schmitzmic@gmail.com>
To: Finn Thain <fthain@telegraphics.com.au>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>,
	"David S. Miller" <davem@davemloft.net>,
	linux-m68k <linux-m68k@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH net] macmace: Set platform device coherent_dma_mask
Date: Fri, 11 May 2018 08:27:01 +1200	[thread overview]
Message-ID: <CAOmrzkKNPemq5RySvza+Y8_jgwg2fkUZodR794cgOxpQpfh+SA@mail.gmail.com> (raw)
In-Reply-To: <alpine.LNX.2.21.1805091805130.72@nippy.intranet>

Hi Finn,

On Thu, May 10, 2018 at 1:25 PM, Finn Thain <fthain@telegraphics.com.au> 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

  reply	other threads:[~2018-05-10 20:27 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <S1751632AbeECEYA/20180503042400Z+254@vger.kernel.org>
2018-05-03  7:25 ` Geert Uytterhoeven
2018-05-03  8:38   ` Finn Thain
2018-05-03  8:46     ` Geert Uytterhoeven
2018-05-03  8:51       ` Christoph Hellwig
2018-05-03 20:24         ` Michael Schmitz
2018-05-04  7:24           ` Geert Uytterhoeven
2018-05-04  8:16             ` Michael Schmitz
2018-05-10  1:25         ` Finn Thain
2018-05-10  1:25       ` Finn Thain
2018-05-10 20:27         ` Michael Schmitz [this message]
2018-05-10 23:55           ` Finn Thain
2018-05-11  2:11             ` Michael Schmitz
2018-05-11  3:28               ` Finn Thain
2018-05-11  4:18                 ` Michael Schmitz
2018-05-11  5:28                   ` Finn Thain
2018-05-11  9:30                     ` Michael Schmitz
2018-05-11 10:06                       ` Finn Thain
2018-05-11 22:02                         ` Michael Schmitz
2018-05-03  4:23 Finn Thain

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAOmrzkKNPemq5RySvza+Y8_jgwg2fkUZodR794cgOxpQpfh+SA@mail.gmail.com \
    --to=schmitzmic@gmail.com \
    --cc=davem@davemloft.net \
    --cc=fthain@telegraphics.com.au \
    --cc=geert@linux-m68k.org \
    --cc=hch@lst.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-m68k@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH net] macmace: Set platform device coherent_dma_mask' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).