Received-SPF: pass (google.com: domain of linux-kernel+bounces-48968-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240126190110.148599-1-afd@ti.com> <CAPDyKFpc38-CFrzhnhutS7c78tZTLM6Bg6XsTKENP8oVT6SQXg@mail.gmail.com>
 <d81a060f-3d8f-44b2-8140-eb8e7ce35d93@ti.com>
In-Reply-To: <d81a060f-3d8f-44b2-8140-eb8e7ce35d93@ti.com>
From: Ulf Hansson <ulf.hansson@linaro.org>
Date: Thu, 1 Feb 2024 23:56:37 +0100
Message-ID: <CAPDyKFonWCJNWcBcUTW2xrqDwJAh0fvg8Ane-phWz64GZ6tcRQ@mail.gmail.com>
Subject: Re: [PATCH] mmc: pwrseq: Use proper reboot notifier path
To: Andrew Davis <afd@ti.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Yangtao Li <frank.li@vivo.com>, 
	linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Oleksij Rempel <o.rempel@pengutronix.de>
Content-Type: text/plain; charset="UTF-8"

+ 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