From: Jon Hunter <jonathanh@nvidia.com> To: Thierry Reding <thierry.reding@gmail.com> Cc: Philipp Zabel <p.zabel@pengutronix.de>, Stephen Warren <swarren@wwwdotorg.org>, Alexandre Courbot <gnurou@gmail.com>, Rafael Wysocki <rjw@rjwysocki.net>, Kevin Hilman <khilman@kernel.org>, Ulf Hansson <ulf.hansson@linaro.org>, Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>, Ian Campbell <ijc+devicetree@hellion.org.uk>, Kumar Gala <galak@codeaurora.org>, Vince Hsu <vinceh@nvidia.com>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org, linux-pm@vger.kernel.org Subject: Re: [PATCH V4 05/16] soc: tegra: pmc: Avoid extra remapping of PMC registers Date: Thu, 14 Jan 2016 19:02:05 +0000 [thread overview] Message-ID: <5697F0AD.9080601@nvidia.com> (raw) In-Reply-To: <20160114172459.GA32767@ulmo> On 14/01/16 17:24, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Thu, Jan 14, 2016 at 04:35:28PM +0000, Jon Hunter wrote: >> >> On 14/01/16 13:45, Thierry Reding wrote: >>>> Old Signed by an unknown key >>> >>> On Fri, Dec 04, 2015 at 02:57:06PM +0000, Jon Hunter wrote: >>>> During early initialisation, the PMC registers are mapped and the PMC SoC >>>> data is populated in the PMC data structure. This allows other drivers >>>> access the PMC register space, via the public tegra PMC APIs, prior to >>>> probing the PMC device. >>>> >>>> When the PMC device is probed, the PMC registers are mapped again and if >>>> successful the initial mapping is freed. If the probing of the PMC device >>>> fails after the registers are remapped, then the registers will be >>>> unmapped and hence the pointer to the PMC registers will be invalid. This >>>> could lead to a potential crash, because once the PMC SoC data pointer is >>>> populated, the driver assumes that the PMC register mapping is also valid >>>> and a user calling any of the public tegra PMC APIs could trigger an >>>> exception because these APIs don't check that the mapping is still valid. >>>> >>>> Rather than adding a test to see if the PMC register mapping is valid, >>>> fix this by removing the second mapping of the PMC registers and reserve >>>> the memory region for the PMC registers during early initialisation where >>>> the initial mapping is created. During the probing of the PMC simply check >>>> that the PMC registers have been mapped. >>>> >>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>>> --- >>>> drivers/soc/tegra/pmc.c | 19 +++++++++---------- >>>> 1 file changed, 9 insertions(+), 10 deletions(-) [snip] > Ah yes, of course. You could still do it with just the two pointers if > you keep the code as-is and revert back to the backed up value in case > of errors. Along this line: > > base = pmc->base; > > pmc->base = devm_ioremap_resource(&pdev->dev, res); > > /* on success */ > iounmap(base); > > /* on error */ > pmc->base = base; > > These pointer assignments should be atomic, so no potential for races > there. I've attached a patch which should do the trick, though I have > not tested it. Right, but I am concerned about someone calling tegra_powergate_set() (which with patch 6 of this series) will poll for the state to change. I am not sure we can guarantee the pointer does not change while this is happening. >> Even so, I was not sure if there could be a race here. >> Ideally, you would lock, but then you need to lock everywhere that you >> use base. Given that my patch still provides a /proc/iomem entry with a >> valid name, it seems best to me. > > The primary reason for the "takeover" is that except for the extra > iounmap() and pointer swapping the tegra_pmc_probe() function is really > a standard driver implementation. The idea behind this had always been > that it should be possible to easily convert this to a proper driver if > either we ended up with PSCI exclusively for SMP or defer SMP setup > until the PMC driver had been probed, so that we could get rid of the > early_initcall(). I agree it is cleaner, however, I am still concerned there could still be a race. Jon
WARNING: multiple messages have this Message-ID (diff)
From: Jon Hunter <jonathanh@nvidia.com> To: Thierry Reding <thierry.reding@gmail.com> Cc: Philipp Zabel <p.zabel@pengutronix.de>, Stephen Warren <swarren@wwwdotorg.org>, Alexandre Courbot <gnurou@gmail.com>, Rafael Wysocki <rjw@rjwysocki.net>, Kevin Hilman <khilman@kernel.org>, Ulf Hansson <ulf.hansson@linaro.org>, Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>, Ian Campbell <ijc+devicetree@hellion.org.uk>, Kumar Gala <galak@codeaurora.org>, Vince Hsu <vinceh@nvidia.com>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-tegra@vger.kernel.org>, <linux-pm@vger.kernel.org> Subject: Re: [PATCH V4 05/16] soc: tegra: pmc: Avoid extra remapping of PMC registers Date: Thu, 14 Jan 2016 19:02:05 +0000 [thread overview] Message-ID: <5697F0AD.9080601@nvidia.com> (raw) In-Reply-To: <20160114172459.GA32767@ulmo> On 14/01/16 17:24, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Thu, Jan 14, 2016 at 04:35:28PM +0000, Jon Hunter wrote: >> >> On 14/01/16 13:45, Thierry Reding wrote: >>>> Old Signed by an unknown key >>> >>> On Fri, Dec 04, 2015 at 02:57:06PM +0000, Jon Hunter wrote: >>>> During early initialisation, the PMC registers are mapped and the PMC SoC >>>> data is populated in the PMC data structure. This allows other drivers >>>> access the PMC register space, via the public tegra PMC APIs, prior to >>>> probing the PMC device. >>>> >>>> When the PMC device is probed, the PMC registers are mapped again and if >>>> successful the initial mapping is freed. If the probing of the PMC device >>>> fails after the registers are remapped, then the registers will be >>>> unmapped and hence the pointer to the PMC registers will be invalid. This >>>> could lead to a potential crash, because once the PMC SoC data pointer is >>>> populated, the driver assumes that the PMC register mapping is also valid >>>> and a user calling any of the public tegra PMC APIs could trigger an >>>> exception because these APIs don't check that the mapping is still valid. >>>> >>>> Rather than adding a test to see if the PMC register mapping is valid, >>>> fix this by removing the second mapping of the PMC registers and reserve >>>> the memory region for the PMC registers during early initialisation where >>>> the initial mapping is created. During the probing of the PMC simply check >>>> that the PMC registers have been mapped. >>>> >>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>>> --- >>>> drivers/soc/tegra/pmc.c | 19 +++++++++---------- >>>> 1 file changed, 9 insertions(+), 10 deletions(-) [snip] > Ah yes, of course. You could still do it with just the two pointers if > you keep the code as-is and revert back to the backed up value in case > of errors. Along this line: > > base = pmc->base; > > pmc->base = devm_ioremap_resource(&pdev->dev, res); > > /* on success */ > iounmap(base); > > /* on error */ > pmc->base = base; > > These pointer assignments should be atomic, so no potential for races > there. I've attached a patch which should do the trick, though I have > not tested it. Right, but I am concerned about someone calling tegra_powergate_set() (which with patch 6 of this series) will poll for the state to change. I am not sure we can guarantee the pointer does not change while this is happening. >> Even so, I was not sure if there could be a race here. >> Ideally, you would lock, but then you need to lock everywhere that you >> use base. Given that my patch still provides a /proc/iomem entry with a >> valid name, it seems best to me. > > The primary reason for the "takeover" is that except for the extra > iounmap() and pointer swapping the tegra_pmc_probe() function is really > a standard driver implementation. The idea behind this had always been > that it should be possible to easily convert this to a proper driver if > either we ended up with PSCI exclusively for SMP or defer SMP setup > until the PMC driver had been probed, so that we could get rid of the > early_initcall(). I agree it is cleaner, however, I am still concerned there could still be a race. Jon
next prev parent reply other threads:[~2016-01-14 19:02 UTC|newest] Thread overview: 108+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-12-04 14:57 [PATCH V4 00/16] Add generic PM domain support for Tegra Jon Hunter 2015-12-04 14:57 ` Jon Hunter 2015-12-04 14:57 ` [PATCH V4 01/16] reset: add of_reset_control_get_by_index() Jon Hunter 2015-12-04 14:57 ` [PATCH V4 02/16] soc: tegra: pmc: Add missing structure members to kernel-doc Jon Hunter [not found] ` <1449241037-22193-3-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-01-25 13:20 ` Thierry Reding 2016-01-25 13:20 ` Thierry Reding 2015-12-04 14:57 ` [PATCH V4 03/16] soc: tegra: pmc: Fix sparse warning for tegra_pmc_init_tsense_reset Jon Hunter 2016-01-25 13:21 ` Thierry Reding 2015-12-04 14:57 ` [PATCH V4 04/16] soc: tegra: pmc: Remove debugfs entry on probe failure Jon Hunter [not found] ` <1449241037-22193-5-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-01-25 13:25 ` Thierry Reding 2016-01-25 13:25 ` Thierry Reding 2015-12-04 14:57 ` [PATCH V4 05/16] soc: tegra: pmc: Avoid extra remapping of PMC registers Jon Hunter 2016-01-14 13:45 ` Thierry Reding 2016-01-14 16:35 ` Jon Hunter 2016-01-14 16:35 ` Jon Hunter 2016-01-14 17:24 ` Thierry Reding 2016-01-14 19:02 ` Jon Hunter [this message] 2016-01-14 19:02 ` Jon Hunter 2015-12-04 14:57 ` [PATCH V4 06/16] soc: tegra: pmc: Wait for powergate state to change Jon Hunter [not found] ` <1449241037-22193-7-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-01-14 14:01 ` Thierry Reding 2016-01-14 14:01 ` Thierry Reding 2016-01-15 9:06 ` Jon Hunter 2016-01-15 9:06 ` Jon Hunter 2015-12-04 14:57 ` [PATCH V4 07/16] soc: tegra: pmc: Remove non-existing power partitions for T210 Jon Hunter 2016-01-25 13:27 ` Thierry Reding 2015-12-04 14:57 ` [PATCH V4 08/16] soc: tegra: pmc: Fix checking of valid partitions Jon Hunter 2016-01-14 14:11 ` Thierry Reding 2016-01-15 9:08 ` Jon Hunter 2016-01-15 9:08 ` Jon Hunter 2015-12-04 14:57 ` [PATCH V4 09/16] soc: tegra: pmc: Ensure partitions can be toggled on/off by PMC Jon Hunter [not found] ` <1449241037-22193-10-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-01-14 14:14 ` Thierry Reding 2016-01-14 14:14 ` Thierry Reding 2016-01-15 9:32 ` Jon Hunter 2016-01-15 9:32 ` Jon Hunter 2015-12-04 14:57 ` [PATCH V4 10/16] PM / Domains: Add function to remove a pm-domain Jon Hunter 2015-12-04 14:57 ` [PATCH V4 11/16] Documentation: DT: bindings: Update NVIDIA PMC for Tegra210 Jon Hunter 2015-12-06 0:31 ` Rob Herring 2015-12-07 9:54 ` Jon Hunter 2015-12-07 9:54 ` Jon Hunter 2015-12-04 14:57 ` [PATCH V4 12/16] Documentation: DT: bindings: Add power domain info for NVIDIA PMC Jon Hunter [not found] ` <1449241037-22193-13-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-12-06 0:37 ` Rob Herring 2015-12-06 0:37 ` Rob Herring 2015-12-07 9:56 ` Jon Hunter 2015-12-07 9:56 ` Jon Hunter 2016-01-14 14:41 ` Thierry Reding 2016-01-14 14:41 ` Thierry Reding 2016-01-15 9:43 ` Jon Hunter 2016-01-15 9:43 ` Jon Hunter 2015-12-08 19:07 ` Kevin Hilman [not found] ` <7h4mfslpyd.fsf-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org> 2015-12-09 12:23 ` Jon Hunter 2015-12-09 12:23 ` Jon Hunter [not found] ` <56681D53.9090600-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-12-09 12:33 ` Jon Hunter 2015-12-09 12:33 ` Jon Hunter [not found] ` <56681F9A.6080200-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-12-15 0:42 ` Kevin Hilman 2015-12-15 0:42 ` Kevin Hilman 2015-12-15 0:34 ` Kevin Hilman 2015-12-15 0:34 ` Kevin Hilman 2015-12-04 14:57 ` [PATCH V4 13/16] soc: tegra: pmc: Add generic PM domain support Jon Hunter 2016-01-14 14:39 ` Thierry Reding 2016-01-15 9:42 ` Jon Hunter 2016-01-15 9:42 ` Jon Hunter [not found] ` <5698BF00.6090102-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2016-01-15 10:01 ` Lucas Stach 2016-01-15 10:01 ` Lucas Stach 2015-12-04 14:57 ` [PATCH V4 14/16] clk: tegra210: Add the APB2APE audio clock Jon Hunter [not found] ` <1449241037-22193-1-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-12-04 14:57 ` [PATCH V4 15/16] ARM64: tegra: Add audio PM domain device node for Tegra210 Jon Hunter 2015-12-04 14:57 ` Jon Hunter 2015-12-04 14:57 ` [PATCH V4 16/16] ARM64: tegra: select PM_GENERIC_DOMAINS Jon Hunter 2015-12-04 14:57 ` Jon Hunter [not found] ` <1449241037-22193-17-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-12-15 19:54 ` Ulf Hansson 2015-12-15 19:54 ` Ulf Hansson [not found] ` <CAPDyKFrcuK4iCo7Tg1Q_XjB7Y-+=s-ax1vzK4UEu9FfPnuProw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-12-16 9:40 ` Jon Hunter 2015-12-16 9:40 ` Jon Hunter 2015-12-16 9:47 ` Ulf Hansson [not found] ` <CAPDyKFpZoZZapqanFyUzT_jZ6DLBnGXOXEyDHosWpphAjzhy7A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-12-16 11:40 ` Jon Hunter 2015-12-16 11:40 ` Jon Hunter 2015-12-16 12:51 ` Ulf Hansson 2016-01-13 17:03 ` Thierry Reding 2016-01-13 17:03 ` Thierry Reding 2016-01-13 17:03 ` Thierry Reding 2016-01-13 20:43 ` Arnd Bergmann 2016-01-13 20:43 ` Arnd Bergmann 2016-01-14 8:57 ` Ulf Hansson 2016-01-14 8:57 ` Ulf Hansson 2016-01-14 8:57 ` Ulf Hansson 2016-01-14 9:21 ` Arnd Bergmann 2016-01-14 9:21 ` Arnd Bergmann 2016-01-14 9:21 ` Arnd Bergmann 2016-01-14 10:29 ` Thierry Reding 2016-01-14 10:29 ` Thierry Reding 2016-01-14 10:29 ` Thierry Reding 2016-01-14 11:11 ` Arnd Bergmann 2016-01-14 11:11 ` Arnd Bergmann 2016-01-14 11:11 ` Arnd Bergmann 2016-01-26 17:30 ` Thierry Reding 2016-01-26 17:30 ` Thierry Reding 2016-01-26 17:30 ` Thierry Reding [not found] ` <20160126173001.GA11062-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org> 2016-01-26 21:52 ` Kevin Hilman 2016-01-26 21:52 ` Kevin Hilman 2016-01-26 21:52 ` Kevin Hilman 2016-01-14 17:16 ` Jon Hunter 2016-01-14 17:16 ` Jon Hunter 2016-01-14 17:16 ` Jon Hunter 2016-01-26 17:01 ` Jon Hunter 2016-01-26 17:01 ` Jon Hunter 2016-01-26 17:01 ` Jon Hunter 2016-01-27 9:43 ` Ulf Hansson 2016-01-27 9:43 ` Ulf Hansson 2016-01-27 9:43 ` Ulf Hansson
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=5697F0AD.9080601@nvidia.com \ --to=jonathanh@nvidia.com \ --cc=devicetree@vger.kernel.org \ --cc=galak@codeaurora.org \ --cc=gnurou@gmail.com \ --cc=ijc+devicetree@hellion.org.uk \ --cc=khilman@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=linux-tegra@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=p.zabel@pengutronix.de \ --cc=pawel.moll@arm.com \ --cc=rjw@rjwysocki.net \ --cc=robh+dt@kernel.org \ --cc=swarren@wwwdotorg.org \ --cc=thierry.reding@gmail.com \ --cc=ulf.hansson@linaro.org \ --cc=vinceh@nvidia.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.