From: Robin Murphy <email@example.com> To: Marek Vasut <firstname.lastname@example.org>, Lorenzo Pieralisi <email@example.com> Cc: Rob Herring <firstname.lastname@example.org>, Geert Uytterhoeven <email@example.com>, PCI <firstname.lastname@example.org>, Geert Uytterhoeven <email@example.com>, Wolfram Sang <firstname.lastname@example.org>, "open list:MEDIA DRIVERS FOR RENESAS - FCP" <email@example.com> Subject: Re: [PATCH V3 2/3] PCI: rcar: Do not abort on too many inbound dma-ranges Date: Tue, 19 Nov 2019 12:10:52 +0000 Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <email@example.com> On 18/10/2019 7:44 pm, Marek Vasut wrote: [...] >>>> Say you have a single hardware window, and this DT property (1-cell >>>> numbers for simplicity: >>>> >>>> dma-ranges = <0x00000000 0x00000000 0x80000000>; >>>> >>>> Driver reads one entry and programs the window to 2GB@0, DMA setup >>>> parses the first entry and sets device masks to 0x7fffffff, and >>>> everything's fine. >>>> >>>> Now say we describe the exact same address range this way instead: >>>> >>>> dma-ranges = <0x00000000 0x00000000 0x40000000, >>>> 0x40000000 0x40000000 0x40000000>; >>>> >>>> Driver reads one entry and programs the window to 1GB@0, DMA setup >>>> parses the first entry and sets device masks to 0x3fffffff, and *today*, >>>> things are suboptimal but happen to work. >>>> >>>> Now say we finally get round to fixing the of_dma code to properly >>>> generate DMA masks that actually include all usable address bits, a user >>>> upgrades their kernel package, and reboots with that same DT... >>>> >>>> Driver reads one entry and programs the window to 1GB@0, DMA setup >>>> parses all entries and sets device masks to 0x7fffffff, devices start >>>> randomly failing or throwing DMA errors half the time, angry user looks >>>> at the changelog to find that somebody decided their now-corrupted >>>> filesystem is less important than the fact that hey, at least the >>>> machine didn't refuse to boot because the DT was obviously wrong. Are >>>> you sure that shouldn't be a problem? >>> >>> I think you picked a rather special case here and arrived as a DMA mask >>> which just fails in this special case. Such special case doesn't happen >>> here, and even if it did, I would expect Linux to merge those two ranges >>> or do something sane ? If the DMA mask is set incorrectly, that's a bug >>> of the DMA code I would think. >> >> The mask is not set incorrectly - DMA masks represent the number of >> address bits the device (or intermediate interconnect in the case of the >> bus mask) is capable of driving. Thus when DMA is limited to a specific >> address range, the masks should be wide enough to cover the topmost >> address of that range (unless the device's own capability is inherently >> narrower). > > Then the mask should be 0x7fffffff in both cases I'd say. Yes, *that's my whole point*. It *should* be that for both cases, but today it works out not to be because the current code is a bit crap. >>> What DMA mask would you get if those two entries had a gap inbetween >>> them ? E.g.: >>> >>> dma-ranges = <0x00000000 0x00000000 0x20000000, >>> 0x40000000 0x40000000 0x20000000>; The gap itself is immaterial - the highest address described by those ranges is 0x5fffffff, which requires 31 bits to drive, thus the appropriate mask would be 0x7fffffff. >> OK, here's an real non-simplified example > > I would really like an answer to the simple example above before we > start inventing convoluted ones. Sigh... I did say "real". However convoluted it may look to you, this window configuration was "invented" ~6 years ago when the Arm Juno SoCs were first brought up and has been shipping ever since. >> (note that these windows are fixed and not programmed by Linux): >> >> dma-ranges = <0x02000000 0x0 0x2c1c0000 0x0 0x2c1c0000 0x0 0x00040000>, >> <0x02000000 0x0 0x80000000 0x0 0x80000000 0x0 0x80000000>, >> <0x43000000 0x8 0x80000000 0x8 0x80000000 0x2 0x00000000>; >> >> The DMA masks for the devices behind this bridge *should* be 35 bits, >> because that's the size of the largest usable address. Currently, >> however, because of the of_dma code's deficiency they would end up being >> an utterly useless 30 bits, which isn't even enough to reach the start >> of RAM. Thus I can't actually have this property in my DT, and as a >> result I can't enable the IOMMU, because *that* also needs to know the >> ranges in order to reserve the unusable gaps between the windows once >> address translation is in play. > > How is this related to this particular patch ? This looks more like some > DMA internal topic. It's trying to get across the point that "dma-ranges" is consumed by multiple separate parts of the kernel, and implicitly relying on the way they currently interact is a mistake which will come back to bite you. Changing one consumer (your driver) to allow devices to exist despite describing unusable DMA ranges might happen to seem OK today due to shortcomings in another consumer (the DMA API layer), but once the DMA API layer gets improved (necessarily, as above), your changes will cause subtle DMA breakage on R-Car systems with more populated DRAM banks than available PCI windows, which is arguably even worse than having PCI simply refuse to probe in that situation. Robin.
next prev parent reply index Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-09 17:57 [PATCH V3 1/3] PCI: rcar: Move the inbound index check marek.vasut 2019-08-09 17:57 ` [PATCH V3 2/3] PCI: rcar: Do not abort on too many inbound dma-ranges marek.vasut 2019-08-16 13:23 ` Simon Horman 2019-08-16 13:28 ` Marek Vasut 2019-08-16 13:38 ` Simon Horman 2019-08-16 17:41 ` Marek Vasut 2019-10-21 10:18 ` Andrew Murray 2019-10-26 18:03 ` Marek Vasut 2019-10-26 20:36 ` Andrew Murray 2019-10-26 21:06 ` Andrew Murray 2019-11-06 23:37 ` Marek Vasut 2019-11-07 14:19 ` Andrew Murray 2019-11-16 15:48 ` Marek Vasut 2019-11-18 18:42 ` Robin Murphy 2019-10-16 15:00 ` Lorenzo Pieralisi 2019-10-16 15:10 ` Marek Vasut 2019-10-16 15:26 ` Lorenzo Pieralisi 2019-10-16 15:29 ` Marek Vasut 2019-10-16 16:18 ` Lorenzo Pieralisi 2019-10-16 18:12 ` Rob Herring 2019-10-16 18:17 ` Marek Vasut 2019-10-16 20:25 ` Rob Herring 2019-10-16 21:15 ` Marek Vasut 2019-10-16 22:26 ` Rob Herring 2019-10-16 22:33 ` Marek Vasut 2019-10-17 7:06 ` Geert Uytterhoeven 2019-10-17 10:55 ` Marek Vasut 2019-10-17 13:06 ` Robin Murphy 2019-10-17 14:00 ` Marek Vasut 2019-10-17 14:36 ` Rob Herring 2019-10-17 15:01 ` Marek Vasut 2019-10-18 9:53 ` Lorenzo Pieralisi 2019-10-18 12:22 ` Marek Vasut 2019-10-18 12:53 ` Robin Murphy 2019-10-18 14:26 ` Marek Vasut 2019-10-18 15:44 ` Robin Murphy 2019-10-18 16:44 ` Marek Vasut 2019-10-18 17:35 ` Robin Murphy 2019-10-18 18:44 ` Marek Vasut 2019-10-21 8:32 ` Geert Uytterhoeven 2019-11-19 12:10 ` Robin Murphy [this message] 2019-10-18 10:06 ` Andrew Murray 2019-10-18 10:17 ` Geert Uytterhoeven 2019-10-18 11:40 ` Andrew Murray 2019-08-09 17:57 ` [PATCH V3 3/3] PCI: rcar: Recalculate inbound range alignment for each controller entry marek.vasut 2019-10-21 10:39 ` Andrew Murray 2019-08-16 10:52 ` [PATCH V3 1/3] PCI: rcar: Move the inbound index check Lorenzo Pieralisi 2019-08-16 10:59 ` Marek Vasut 2019-08-16 11:10 ` Lorenzo Pieralisi 2019-10-15 20:14 ` Marek Vasut 2019-10-21 10:11 ` Andrew Murray
Reply instructions: You may reply publically 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 \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ /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
Linux-PCI Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \ firstname.lastname@example.org public-inbox-index linux-pci Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci AGPL code for this site: git clone https://public-inbox.org/public-inbox.git