From: Robin Murphy <email@example.com> To: Rob Herring <firstname.lastname@example.org> Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com>, Kishon Vijay Abraham I <email@example.com>, Lorenzo Pieralisi <firstname.lastname@example.org>, Bjorn Helgaas <email@example.com>, Jingoo Han <firstname.lastname@example.org>, Gustavo Pimentel <email@example.com>, PCI <firstname.lastname@example.org>, linux-omap <email@example.com>, "firstname.lastname@example.org" <email@example.com>, linux-arm-kernel <firstname.lastname@example.org> Subject: Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume Date: Wed, 14 Oct 2020 15:49:59 +0100 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <CAL_JsqLi09RTyiVLcyp1K4MNBggTvs3wqVqihpV2QhuePa3u9w@mail.gmail.com> On 2020-10-14 15:15, Rob Herring wrote: > On Mon, Oct 12, 2020 at 6:37 AM Robin Murphy <firstname.lastname@example.org> wrote: >> >> On 2020-10-09 08:55, Jisheng Zhang wrote: >>> Currently, dw_pcie_msi_init() allocates and maps page for msi, then >>> program the PCIE_MSI_ADDR_LO and PCIE_MSI_ADDR_HI. The Root Complex >>> may lose power during suspend-to-RAM, so when we resume, we want to >>> redo the latter but not the former. If designware based driver (for >>> example, pcie-tegra194.c) calls dw_pcie_msi_init() in resume path, the >>> msi page will be leaked. >>> >>> As pointed out by Rob and Ard, there's no need to allocate a page for >>> the MSI address, we could use an address in the driver data. >>> >>> To avoid map the MSI msg again during resume, we move the map MSI msg >>> from dw_pcie_msi_init() to dw_pcie_host_init(). >> >> You should move the unmap there as well. As soon as you know what the >> relevant address would be if you *were* to do DMA to this location, then >> the exercise is complete. Leaving it mapped for the lifetime of the >> device in order to do not-DMA to it seems questionable (and represents >> technically incorrect API usage without at least a sync_for_cpu call >> before any other access to the data). >> >> Another point of note is that using streaming DMA mappings at all is a >> bit fragile (regardless of this change). If the host controller itself >> has a limited DMA mask relative to physical memory (which integrators >> still seem to keep doing...) then you could end up punching your MSI >> hole right in the middle of the SWIOTLB bounce buffer, where it's then >> almost *guaranteed* to interfere with real DMA :( > > Couldn't that happen with the current code too? alloc_page() isn't > guaranteed to be DMA'able, right? Indeed that's what I meant by "regardless of this change". >> If no DWC users have that problem and the current code is working well >> enough, then I see little reason not to make this partucular change to >> tidy up the implementation, just bear in mind that there's always the >> possibility of having to come back and change it yet again in future to >> make it more robust. I had it in mind that this trick was done with a >> coherent DMA allocation, which would be safe from addressing problems >> but would need to be kept around for the lifetime of the device, but >> maybe that was a different driver :/ > > Well, we're wasting 4K or 64K of memory and then leaking it is the > main reason to change it. > > We just need any address that's not memory which PCI could access. We > could possibly just take the end of (outbound) PCI memory space. Note > that the DWC driver never sets up inbound translations, so it's all > 1:1 mapping (though upstream could have some translation). Right, this patch is undeniably a better implementation of the existing approach, I just felt it worth pointing out that that approach itself has fundamental flaws which may or may not be relevant to some current and/or future users. I know for a fact that there are platforms which cripple their PCIe host bridge to 32-bit physical addressing but support having RAM above that; I don't *think* any of the ones I know of are using the dw_pcie driver, but hey, how much do I know? ;) Robin.
next prev parent reply other threads:[~2020-10-14 14:50 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-09 7:53 [PATCH v7 0/2] PCI: dwc: fix two MSI issues Jisheng Zhang 2020-10-09 7:54 ` [PATCH v7 1/2] PCI: dwc: Skip PCIE_MSI_INTR0* programming if MSI is disabled Jisheng Zhang 2020-10-09 7:55 ` [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume Jisheng Zhang 2020-10-12 11:37 ` Robin Murphy 2020-10-12 15:35 ` Vidya Sagar 2020-10-14 14:15 ` Rob Herring 2020-10-14 14:49 ` Robin Murphy [this message] 2020-10-14 16:52 ` Ard Biesheuvel 2020-10-14 17:08 ` Robin Murphy 2020-10-09 8:49 ` [PATCH v7 0/2] PCI: dwc: fix two MSI issues Lorenzo Pieralisi
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 \ --email@example.com \ --firstname.lastname@example.org \ --cc=Jisheng.Zhang@synaptics.com \ --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 \ --firstname.lastname@example.org \ --subject='Re: [PATCH v7 2/2] PCI: dwc: Fix MSI page leakage in suspend/resume' \ /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).