| Message ID | 20240126190110.148599-1-afd@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: pwrseq: Use proper reboot notifier path | expand |
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 >
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
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 >>
+ 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
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"); }
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(-)