| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 22 Oct 2015 12:07:14 +0200 From: Marek Szyprowski <m.szyprowski@...sung.com> To: Alim Akhtar <alim.akhtar@...sung.com>, Javier Martinez Canillas <javier@....samsung.com>, Krzysztof Kozlowski <k.kozlowski@...sung.com>, linux-kernel@...r.kernel.org Cc: Markus Reichl <m.reichl@...etechno.de>, Anand Moon <linux.amoon@...il.com>, linux-samsung-soc@...r.kernel.org, linux-mmc@...r.kernel.org, Alexandre Courbot <acourbot@...dia.com>, Ulf Hansson <ulf.hansson@...aro.org>, Douglas Anderson <dianders@...omium.org>, heiko@...ech.de, enric.balletbo@...labora.com Subject: Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler Hello, On 2015-10-22 06:14, Alim Akhtar wrote: > CCing Doug, Heiko and Enric Balletbo > To help us by testing on rk3288-veyron and am335x-sl50 boards. > > On 10/22/2015 08:22 AM, Javier Martinez Canillas wrote: >> Hello Krzysztof, >> >> On 10/22/2015 03:43 AM, Krzysztof Kozlowski wrote: >>> On 22.10.2015 10:20, Javier Martinez Canillas wrote:> Hello Krzysztof, >>>> >>>> Thanks for your feedback. >>>> >>>> On 10/22/2015 02:36 AM, Krzysztof Kozlowski wrote: >>>>> On 22.10.2015 00:15, Javier Martinez Canillas wrote: >>>>>> The pwrseq_emmc driver does a eMMC card reset before a system >>>>>> reboot to >>>>>> allow broken or limited ROM boot-loaders (that don't have an eMMC >>>>>> reset >>>>>> logic) to be able to read the second stage from the eMMC. >>>>>> >>>>>> But this has to be called before a system reboot handler and >>>>>> while most >>>>>> of them use the priority 128, there are other restart handlers >>>>>> (such as >>>>>> the syscon-reboot one) that use a higher priority. So, use the >>>>>> highest >>>>>> priority to make sure that the eMMC hw is reset before a system >>>>>> reboot. >>>>>> >>>>>> Signed-off-by: Javier Martinez Canillas <javier@....samsung.com> >>>>>> Tested-by: Markus Reichl <m.reichl@...etechno.de> >>>>>> Tested-by: Anand Moon <linux.amoon@...il.com> >>>>>> Reviewed-by: Alim Akhtar <alim.akhtar@...sung.com> >>>>>> >>>>>> --- >>>>>> Hello, >>>>>> >>>>>> This patch was needed since a recent series from Alim [0] added >>>>>> syscon reboot and poweroff support to Exynos SoCs and removed >>>>>> the reset handler in the Exynos Power Management Unit (PMU) code. >>>>>> >>>>>> But the PMU and syscon-reboot restart handler have a different >>>>>> priority so [0] breaks restart when eMMC is used on these boards. >>>>>> >>>>>> [0]: http://www.spinics.net/lists/arm-kernel/msg454396.html >>>>>> >>>>>> So this patch must be merged before [0] to avoid regressions. >>>>>> >>>>>> Best regards, >>>>>> Javier >>>>>> >>>>>> drivers/mmc/core/pwrseq_emmc.c | 6 +++--- >>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/mmc/core/pwrseq_emmc.c >>>>>> b/drivers/mmc/core/pwrseq_emmc.c >>>>>> index 137c97fb7aa8..ad4f94ec7e8d 100644 >>>>>> --- a/drivers/mmc/core/pwrseq_emmc.c >>>>>> +++ b/drivers/mmc/core/pwrseq_emmc.c >>>>>> @@ -84,11 +84,11 @@ struct mmc_pwrseq >>>>>> *mmc_pwrseq_emmc_alloc(struct mmc_host *host, >>>>>> >>>>>> /* >>>>>> * register reset handler to ensure emmc reset also from >>>>>> - * emergency_reboot(), priority 129 schedules it just before >>>>>> - * system reboot >>>>>> + * emergency_reboot(), priority 255 is the highest priority >>>>>> + * so it will be executed before any system reboot handler. >>>>>> */ >>>>>> pwrseq->reset_nb.notifier_call = mmc_pwrseq_emmc_reset_nb; >>>>>> - pwrseq->reset_nb.priority = 129; >>>>>> + pwrseq->reset_nb.priority = 255; >>>>> >>>>> I see the problem which you are trying to solve but this may be >>>>> tricker >>>>> then just kicking the number. Some of restart handlers are registered >>>>> with priority 192. I found few of such, like: at91_restart_nb, >>>>> zynq_slcr_restart_nb, rmobile_reset_nb (maybe more, I did not grep >>>>> too >>>>> much). >>>>> >>>> >>>> Yes, the syscon-reboot restart handler also uses a priority 192 and >>>> that >>>> is why reboot with eMMC broke with Alim's patches since the PMU >>>> restart >>>> handler priority is 128. >>>> >>>>> I guess they chose the "192" priority on purpose. >>>>> >>>> >>>> I tried to understand what's the policy w.r.t priority numbering for >>>> restart handlers but only found this in the register_restart_handler >>>> kernel-doc [0]: >>>> >>>> /** >>>> * register_restart_handler - Register function to be called to >>>> reset >>>> * the system >>>> * @nb: Info about handler function to be called >>>> * @nb->priority: Handler priority. Handlers should follow the >>>> * following guidelines for setting priorities. >>>> * 0: Restart handler of last resort, >>>> * with limited restart capabilities >>>> * 128: Default restart handler; use if no other >>>> * restart handler is expected to be available, >>>> * and/or if restart functionality is >>>> * sufficient to restart the entire system >>>> * 255: Highest priority restart handler, will >>>> * preempt all other restart handlers >>>> >>>> So, reading that is not clear to me if only the values 0, 128 and 255 >>>> should be used or any value from 0-255. >>>> >>>> What's clear to me is that restart handlers to reset a specific hw >>>> block >>>> should be called before the restart handler that resets the whole >>>> system. >>>> >>>> The 192 seems to be used because there are other default restart >>>> handlers >>>> that are using a prio of 128. See for example the commit that >>>> changed the >>>> syscon-reboot prio from 128 to 192: >>>> >>>> b81180b3fd48 power: reset: adjust priority of simple syscon reboot >>>> driver >>> >>> But were are here not talking about syscon handler but the others. Now >>> you will be ahead of them. >>> >> >> Yes, I know that. My point was that the platforms were either not >> using the >> mmc-pwrseq-emmc or their system restart handler already had a lower >> priority >> but that is not true for at least rk3288-veyron as you said. >> >>>> >>>> So probably the 192 value was chosen because is in the middle of >>>> 128 and >>>> 255 but it seems to me a rather arbitrary value and I would prefer >>>> it to >>>> be documented in some place. >>>> >>>>> Effectively, now the emmc handler will be executed before their >>>>> handlers... is it an issue? Maybe some testing on these platforms is >>>>> necessary? >>>>> >>>> >>>> I don't think is an issue, the reason why I chose 255 is that it is >>>> a documented value in the kernel-doc and since is the highest prio, >>>> it makes sure the eMMC will be reset before any system restart >>>> handler. >>>> >>>> Also, the pwrseq_emmc driver is only used in platforms whose SoC ROM >>>> can either leave the eMMC in an unknown state so the kernel needs to >>>> hw reset the eMMC or does not have a reset logic so it can only read >>>> from an eMMC if is in a known state (i.e: after a reset from kernel). >>> >>> I think at least one platform may be affected because it used >>> mmc-pwrseq-emmc and gpio-restart. >>> >>> Look at rk3288-veyron.dtsi. >>> >>> Both of restart handlers had the priority of 129 which means that the >>> order of execution depends on probing sequence. Now you will make the >>> sequence strict - first mmc then gpio. >>> >> >> The behavior is going to change indeed in that board but no due probe >> order but because the gpio-restart handler dev node has priority = <200> >> which overrides the default 129 in the gpio-restart driver. >> >> So before $SUBJECT the eMMC restart handler was not executed but now it >> will be after this change. >> >>> You seems convinced that this is not a problem... I don't know. I would >>> prefer test this on affected platforms before risking to break them. >>> It's annoying if fix for one SoC breaks another. >>> >> >> Agreed. >> >>>> >>>> Since the current mmc_pwrseq_emmc_reset_nb notifier priority is 129, >>>> eMMC reset will not work if one of the platforms you mentioned needs >>>> this since the system restart handler with prio 192 will be executed >>>> before the eMMC one, leaving the eMMC in an unknown state on reboot. >>> >>> And now you will "fix this" by making eMMC working correctly. So let's >>> make it straight: >>> 1. Previously the eMMC could be left on these platforms in an unknown >>> state (because emmc handler was not executed). >>> 2. No one complained! Which could mean that in fact this was working >>> fine... >>> 3. Now you will change it. >>> 4. Maybe someone will complain? >>> >>> Just test it (or get an ack/tested tag). That's all what is needed. >>> >> >> Yes, I never meant that the patch should be merged without testing... >> >>> >>>> And $SUBJECT should not cause any regressions for the platforms that >>>> are currently using the pwrseq_emmc, since the restart handler was >>>> already being called before the system restart handler so bumping >>>> the priority should not cause any effect. >>> >>> I found at least one platform where the sequence *might* change. There >>> could be more of them. >>> >> >> Agreed, I missed that rk3288-veyron is using a restart handler with >> higher >> priority and could be other boards too as you said. >> >> Let's see what is Marek's opinion since he added the pwrseq_emmc support >> and also what Ulf thinks about always doing a eMMC reset before reboot. >> >> I can't think how doing a eMMC card reset before reboot could affect a >> board but you are right that we don't know without testing. >> > git grep result shows: > ====== > git grep mmc-pwrseq-emmc > Documentation/devicetree/bindings/mmc/mmc-pwrseq-emmc.txt:- compatible > : contains "mmc-pwrseq-emmc". > Documentation/devicetree/bindings/mmc/mmc-pwrseq-emmc.txt: compatible > = "mmc-pwrseq-emmc"; > arch/arm/boot/dts/am335x-sl50.dts: compatible = > "mmc-pwrseq-emmc"; > arch/arm/boot/dts/exynos4412-odroid-common.dtsi: compatible = > "mmc-pwrseq-emmc"; > arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi: compatible = > "mmc-pwrseq-emmc"; > arch/arm/boot/dts/rk3288-veyron.dtsi: compatible = > "mmc-pwrseq-emmc"; > arch/arm64/boot/dts/rockchip/rk3368-r88.dts: compatible = > "mmc-pwrseq-emmc"; > drivers/mmc/core/pwrseq.c: .compatible = "mmc-pwrseq-emmc", > ===== > So apart from exynos, there are rk3xxx and am335x which used > pwrseq-emmc-restart. So lets wait for the feedback from these guys as > well. > Thanks. > The priority was initially chosen in such a way to do the emmc reset just before performing system reboot. I wanted let other possible handlers potentially use mmc if needed (although there is no such case atm). Now it turns that this approach was not the best idea, because there are other board-specific restart handlers with higher priority than the default I was using. IMHO the change proposed by Javier seems to be the best solution for now. The other possibility would be to entirely get rid of restart handler usage and wire this logic to mmc_shutdown(). This makes sense from the logical point of view, but the drawback of this solution is the lack of proper reset sequence in case of emergency reboot (shutdown callbacks are not called on emergency reboot). Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists