[PATCH] mmc: pwrseq: Use proper reboot notifier path

[PATCH] mmc: pwrseq: Use proper reboot notifier path

[Andrew Davis]

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.

Signed-off-by: Andrew Davis <afd@ti.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

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

[Ulf Hansson]

On Fri, 26 Jan 2024 at 20:01, Andrew Davis <afd@ti.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@ti.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
>

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

[Andrew Davis]

On 1/30/24 6:04 AM, Ulf Hansson wrote:
> On Fri, 26 Jan 2024 at 20:01, Andrew Davis <afd@ti.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. 

That is by definition a "cleanup", even if the cleanup is really important.

> This is needed on some platforms with broken bootloaders (ROM
> code), that is expecting the eMMC to always start in a clean reset
> state.
> 

I understand the reason, I don't agree with the method used to get
the result.

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

In this thread I see a lot of discussion about the priority of the
handler. You want this to run before any real reboot handlers
are run. Luckily for you, all reboot "notifiers" are run before
any "handlers" are run. So if you register as a "notifier" as
this patch does, you will be run first, no super high priority
settings needed.

The real issue is you want to be called even in the
emergency_restart() path, which is fine. But from the
docs for that function this type of restart is done:

> Without shutting down any hardware 

So we have two options:

1. Add a new notifier list that *does* get called in the
    emergency_restart() path. Then register this driver with
    with that.

2. Remove emergency_restart() from the kernel. It only has a
    couple of callers, and most of those callers look like they
    should instead be using hw_protection_reboot() or panic().
    That way all reboot paths activate the reboot notifiers.
    Kinda wondering why you think you need to handle the
    emergency_restart() case at all, will even be a thing on
    your hardware, i.e. is this a real problem at all?

Having this driver claim to be a real reboot handler to sneak
around doing one of the above is preventing some cleanup I am
working on. So if either of the above two options work for you
just let me know, I'll help out in implementing them for you.

Thanks,
Andrew

> 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@ti.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
>>

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

[Ulf Hansson]

+ Oleksij

On Thu, 1 Feb 2024 at 17:20, Andrew Davis <afd@ti.com> wrote:
>
> On 1/30/24 6:04 AM, Ulf Hansson wrote:
> > On Fri, 26 Jan 2024 at 20:01, Andrew Davis <afd@ti.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.
>
> That is by definition a "cleanup", even if the cleanup is really important.
>
> > This is needed on some platforms with broken bootloaders (ROM
> > code), that is expecting the eMMC to always start in a clean reset
> > state.
> >
>
> I understand the reason, I don't agree with the method used to get
> the result.
>
> > So, we need both parts, as was discussed here [1] too.
> >
>
> In this thread I see a lot of discussion about the priority of the
> handler. You want this to run before any real reboot handlers
> are run. Luckily for you, all reboot "notifiers" are run before
> any "handlers" are run. So if you register as a "notifier" as
> this patch does, you will be run first, no super high priority
> settings needed.

Right, I didn't say the solution we use for mmc is perfect, but it was
the easiest solution at hand at the introduction.

>
> The real issue is you want to be called even in the
> emergency_restart() path, which is fine. But from the
> docs for that function this type of restart is done:
>
> > Without shutting down any hardware
>
> So we have two options:
>
> 1. Add a new notifier list that *does* get called in the
>     emergency_restart() path. Then register this driver with
>     with that.
>
> 2. Remove emergency_restart() from the kernel. It only has a
>     couple of callers, and most of those callers look like they
>     should instead be using hw_protection_reboot() or panic().
>     That way all reboot paths activate the reboot notifiers.
>     Kinda wondering why you think you need to handle the
>     emergency_restart() case at all, will even be a thing on
>     your hardware, i.e. is this a real problem at all?

The "emergency reset" is needed, due to broken bootloaders, as I
pointed out earlier.

That said, I don't have any strong opinions around this, whatever
option you pick to rework the code is fine by me. The important point
is that we can continue to support the use cases we need for MMC.

BTW, there was a recent related discussion [1] that you may want to
catch up with too, before you start doing the restructuring of the
restart/reboot code. See the link below.

>
> Having this driver claim to be a real reboot handler to sneak
> around doing one of the above is preventing some cleanup I am
> working on. So if either of the above two options work for you
> just let me know, I'll help out in implementing them for you.

That would be great, thanks!

>
> Thanks,
> Andrew

Kind regards
Uffe

[1]
PATCH v1 0/3] introduce priority-based shutdown support]
https://lore.kernel.org/lkml/2023112520-paper-image-ef5d@gregkh/T/#mb45749c3bc9b89caecfeca6e66da8721d920191b

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

[Marek Szyprowski]

On 26.01.2024 20:01, Andrew Davis 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.


Well, I'm the author of this 'hack' and unfortunately there was no other 
way to make emergency reboot working on boards requiring the eMMC 
pwrseq. IIRC this has been already discussed and the conclusion was to 
accept the hack with the comments explaining the problem.


> Signed-off-by: Andrew Davis <afd@ti.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");
>   	}

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland