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

[Andrew Davis <afd@ti.com>]

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
>>