Message ID | 20240126190110.148599-1-afd@ti.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-40538-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:2395:b0:106:343:edcb with SMTP id gw21csp85025dyb; Fri, 26 Jan 2024 11:01:42 -0800 (PST) X-Google-Smtp-Source: AGHT+IHeSAPCXzeWLvcsoIbM3No6r/imVAA76enZghZNfxn23jxz6d2tD8fwqr2Gl6cuBV1/ODS1 X-Received: by 2002:ac8:594c:0:b0:42a:60fa:ea21 with SMTP id 12-20020ac8594c000000b0042a60faea21mr412584qtz.129.1706295702260; Fri, 26 Jan 2024 11:01:42 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1706295702; cv=pass; d=google.com; s=arc-20160816; b=N8Cr0/kCdm0T6gTc0Dh0pSsylfzI0q0HOh8JSrIJy73MALqDBjfVvL/CL5P+upTSRg qulS8kCay4BXcxdyeRy1kt5upr2ZBl24JWRClHq2lbLF6W26oXKdArDAxtov/MrEALFd 6JNR6ECB83ZabQnsbSb1tIjSW4byPh0Y8ApEk2VwBucWCUS7t/8kPHYnhr1+fBs/TEZQ uN8VyzL5oDNSjp1QYu9hqi9fqDUkqh1aaYstZCly94V9CWyaLBgRZAU3bcFBCfBUvF7V 9gdfGH9VQxMUGEp3vicZNJ0qrH1VBFy7V7BcxPS+2GXUFzaBTqzI0SIZh0tq1lV2gPP5 UVDw== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=NwF79J4QERfbFo+oO9aZ7C3Ltm7a+qI/pEWqX7Hx6fA=; fh=SP2ohPcQqhFv8IQwk3hsvkExFFYPZvL0XjiajCAjjsk=; b=n9lZurr67oj8zsPQeKLc+B71O95oUZ+r0W0+z+LnfvKXVq5ab5IoyaJrbxuKmxXNcd UZ0IQUqIZ2ornLwksmd8To2KOMYVkiytXEAjDsqiwMMDmcfLIgd/nElQ3sMA25SAcvMh 50kSLqMDvrWRYR76S2wERcIXTNeIXXOmSTNHbUg77mbDLmPFWn7XoOdaJuAF8/hYXnpu u4f5RoMOPT+uzLqwMmDa0lDy/trA+jveACKTO1qNsSZeg/wTOFOiEWNFCuJ0wxQkyLar 99MhfQfmD96VCR0tlK/CE3sazi2BdG01cBpwx4GtsHW08ip+gCESnRcdlLjzTTclp/sr b/iQ== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=hnNMvmH6; arc=pass (i=1 spf=pass spfdomain=ti.com dkim=pass dkdomain=ti.com dmarc=pass fromdomain=ti.com); spf=pass (google.com: domain of linux-kernel+bounces-40538-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-40538-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [2604:1380:45d1:ec00::1]) by mx.google.com with ESMTPS id w7-20020ac87e87000000b0042a5bb4613fsi1829016qtj.79.2024.01.26.11.01.42 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 Jan 2024 11:01:42 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-40538-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=hnNMvmH6; arc=pass (i=1 spf=pass spfdomain=ti.com dkim=pass dkdomain=ti.com dmarc=pass fromdomain=ti.com); spf=pass (google.com: domain of linux-kernel+bounces-40538-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-40538-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id 136221C22A35 for <ouuuleilei@gmail.com>; Fri, 26 Jan 2024 19:01:42 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 989AA224DF; Fri, 26 Jan 2024 19:01:28 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ti.com header.i=@ti.com header.b="hnNMvmH6" Received: from lelv0142.ext.ti.com (lelv0142.ext.ti.com [198.47.23.249]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D9A5D21A04; Fri, 26 Jan 2024 19:01:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.47.23.249 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706295685; cv=none; b=FN9OGtKo6IRB3k1Ph/eSRQSKbLGl3oJNi9tghQToS5KUiBCGKleep1HRjTPzkJgNp8L5UheO/wj9bKVNhzYkdhBLEwIjr39Ay17vm/tYVy0js6TM4RRm0/2TM0zMOWFRMD7qOd32f+pfDXpUHhm7ESBYzgN1jJbZQraEvN874CY= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706295685; c=relaxed/simple; bh=85dAb/dDmFrF4oFSp1ZL2gDWKRF1dNwmGB3QvaBcPws=; h=From:To:CC:Subject:Date:Message-ID:MIME-Version:Content-Type; b=gpkP7V6jpk0e6Ml0zC+ZXi+PeW0OIXFMt89cTwYDT9NEbf9YA87kee4KDwnTgnPoQ/cIx0GGg8L6kQWXFPctpBcZc3mawA0GOU4Dah9XcqdGFdV1kaiB2QKeBs1ndOYt83pkQNCPpeHDmioWgV7XNoTXGOsfwqLHID1TWIVNYds= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=ti.com; spf=pass smtp.mailfrom=ti.com; dkim=pass (1024-bit key) header.d=ti.com header.i=@ti.com header.b=hnNMvmH6; arc=none smtp.client-ip=198.47.23.249 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=ti.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ti.com Received: from fllv0034.itg.ti.com ([10.64.40.246]) by lelv0142.ext.ti.com (8.15.2/8.15.2) with ESMTP id 40QJ1BIB095643; Fri, 26 Jan 2024 13:01:11 -0600 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1706295671; bh=NwF79J4QERfbFo+oO9aZ7C3Ltm7a+qI/pEWqX7Hx6fA=; h=From:To:CC:Subject:Date; b=hnNMvmH6b8VmM6hHqk57ZfNx9ezkYLMLYZurIy8++gnVII33BgVVTbJ7vjKFUJoWI GWnU6UQ+CWwvzCGZ2SxER5Jwujgdi+/roMWg1DyjV7yApgsv5h3YTkdDYFm7SwWkZp opxSnwqJZ1BKKmArDT0pqK5yTfoB67hrSVa+hHaA= Received: from DLEE113.ent.ti.com (dlee113.ent.ti.com [157.170.170.24]) by fllv0034.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 40QJ1Blh057870 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 26 Jan 2024 13:01:11 -0600 Received: from DLEE111.ent.ti.com (157.170.170.22) by DLEE113.ent.ti.com (157.170.170.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23; Fri, 26 Jan 2024 13:01:11 -0600 Received: from lelvsmtp6.itg.ti.com (10.180.75.249) by DLEE111.ent.ti.com (157.170.170.22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23 via Frontend Transport; Fri, 26 Jan 2024 13:01:11 -0600 Received: from lelvsmtp5.itg.ti.com ([10.249.42.149]) by lelvsmtp6.itg.ti.com (8.15.2/8.15.2) with ESMTP id 40QJ1A8L013671; Fri, 26 Jan 2024 13:01:11 -0600 From: Andrew Davis <afd@ti.com> To: Marek Szyprowski <m.szyprowski@samsung.com>, Ulf Hansson <ulf.hansson@linaro.org>, Yangtao Li <frank.li@vivo.com> CC: <linux-mmc@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Andrew Davis <afd@ti.com> Subject: [PATCH] mmc: pwrseq: Use proper reboot notifier path Date: Fri, 26 Jan 2024 13:01:10 -0600 Message-ID: <20240126190110.148599-1-afd@ti.com> X-Mailer: git-send-email 2.39.2 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1789180722550571782 X-GMAIL-MSGID: 1789180722550571782 |
Series |
mmc: pwrseq: Use proper reboot notifier path
|
|
Commit Message
Andrew Davis
Jan. 26, 2024, 7:01 p.m. UTC
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(-)
Comments
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"); }