linux-kernel - Re: [PATCH] mmc: pwrseq: Use proper reboot notifier path

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPDyKFpc38-CFrzhnhutS7c78tZTLM6Bg6XsTKENP8oVT6SQXg@mail.gmail.com>
Date: Tue, 30 Jan 2024 13:04:44 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Andrew Davis <afd@...com>
Cc: Marek Szyprowski <m.szyprowski@...sung.com>, Yangtao Li <frank.li@...o.com>, 
	linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mmc: pwrseq: Use proper reboot notifier path

On Fri, 26 Jan 2024 at 20:01, Andrew Davis <afd@...com> wrote:
>
> This driver registers itself as a reboot handler, which means it claims
> it can reboot the system. It does this so it is called during the system
> reboot sequence. The correct way to be notified during the reboot
> sequence is to register a notifier with register_reboot_notifier().
> Do this here.
>
> Note this will be called during normal reboots but not emergency reboots.
> This is the expected behavior, emergency reboot means emergency, not go
> do some cleanup with emmc pins.. The reboot notifiers are intentionally
> not called in the emergency path for a reason and working around that by
> pretending to be a reboot handler is a hack.

I understand the reason for the $subject patch, but it will not work,
unfortunately.

For eMMC we need to manage emergency reboots too. The fiddling with
GPIOs isn't a "cleanup", but tries to move the eMMC into a clean reset
state. This is needed on some platforms with broken bootloaders (ROM
code), that is expecting the eMMC to always start in a clean reset
state.

So, we need both parts, as was discussed here [1] too.

Kind regards
Uffe

[1]
https://lore.kernel.org/all/1445440540-21525-1-git-send-email-javier@osg.samsung.com/

>
> Signed-off-by: Andrew Davis <afd@...com>
> ---
>  drivers/mmc/core/pwrseq_emmc.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
> index 3b6d69cefb4eb..d5045fd1a02c1 100644
> --- a/drivers/mmc/core/pwrseq_emmc.c
> +++ b/drivers/mmc/core/pwrseq_emmc.c
> @@ -70,14 +70,8 @@ static int mmc_pwrseq_emmc_probe(struct platform_device *pdev)
>                 return PTR_ERR(pwrseq->reset_gpio);
>
>         if (!gpiod_cansleep(pwrseq->reset_gpio)) {
> -               /*
> -                * register reset handler to ensure emmc reset also from
> -                * 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 = 255;
> -               register_restart_handler(&pwrseq->reset_nb);
> +               register_reboot_notifier(&pwrseq->reset_nb);
>         } else {
>                 dev_notice(dev, "EMMC reset pin tied to a sleepy GPIO driver; reset on emergency-reboot disabled\n");
>         }
> --
> 2.39.2
>