From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5CF93C433DF for ; Wed, 12 Aug 2020 15:29:33 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 164CA20885 for ; Wed, 12 Aug 2020 15:29:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="QQS88MH1"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="OD+PPyXQ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 164CA20885 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=xXoPbtYQujQNmE4ZeZkedgmUhV887tJSTdGyIJD/D1s=; b=QQS88MH18heUIbtMB4tLdB60X gHp+RGu2E6/F5PsBD96tj5B49j4ysqXfa69qLX+EGHDLRTEP2dBAsoNgwX22rSg9U+uaCZHZBbA6z eeIEJcKt6jfd2RnIu8g/tepY+ohB8Ut8sJnUbCv9q0t1nrw6BZRT3w1CB0G9njYI7uN6TtgjGGEa5 vo0cozhLTBvgbAvjLBxXbXUd+TC9rOHg2ZCrtC5qYml4y5edbHTvNx9+6wMD5r3GqU30AlPpi+61B A1kaUSYXiYVWGFZ3d6Iy/+y8h6KJJVOsJHboV7fPQnUpqrrQY9iSkP6TX4w8TG3J+lKgOx8PhfVnv pQtxIrVVA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k5sey-0001Be-Ot; Wed, 12 Aug 2020 15:27:24 +0000 Received: from mail-ua1-x943.google.com ([2607:f8b0:4864:20::943]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k5sev-0001B6-0Q for linux-arm-kernel@lists.infradead.org; Wed, 12 Aug 2020 15:27:22 +0000 Received: by mail-ua1-x943.google.com with SMTP id g20so713825uap.8 for ; Wed, 12 Aug 2020 08:27:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jr6fmB1bCqolizwxBYidbm0DK7lilrbAIzTvtSAUPwc=; b=OD+PPyXQihFrcz74PmOCztCBdly6lDfViBfUJ8bCYSihAf+NTeDjNlW27957oiYU2y vaECRi/hoMtqa6dmNzCM0lUUK9dKf9YF1RJK40M+BIu3PCmhMkCgJp4KkehnGF4pFDDc FtsT8LlG/wJQeKainLa9Xcab8pOVK4bYOAb1c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jr6fmB1bCqolizwxBYidbm0DK7lilrbAIzTvtSAUPwc=; b=i/NAsYwdwEMqT0jKqlpvAqjcxG4WM0pOPV1JVg0c9QnZFeZczMhWMB/oxEezNBCShk FooX/0S1DqKnPb8GjFW7FjUJKLSblyw8EjvXrpkfgxEd8XA5r5ZRNdwHfdoNiY12UNyY m4I0F3kqsuffFval5fSbDw+VxrzTuMtKItYRHAFdeLpdmlcZWwK0olnz2YtbAi0hpNrd 0MDA8alaugOmIqhDUYSL+VdkEwtcyqdDnZjRDZ9lzwdRbdiI6w+RfoCcqkUv2Y4KinQ+ qbxpdgHSHB2XorTCsYcydS2ZqIc/Hfqxu296eoxFfkISwQafDsweoHVbJo2jj/+742w1 YnGg== X-Gm-Message-State: AOAM532IEdyW039FZRJfAOHL8pThzYGn1YreXcDXeD0uomumUN9YuRO5 m72rOGe/hsDXAcFaUrUv/8a5KlU0PZY= X-Google-Smtp-Source: ABdhPJzWDNNX9G66dP5BMaFbiI6HmDDan34dWcWz6fb6UdGmyyzQ8WRymmBVblJcoB6DA7jb0KFJnw== X-Received: by 2002:ab0:2705:: with SMTP id s5mr10584487uao.126.1597246037984; Wed, 12 Aug 2020 08:27:17 -0700 (PDT) Received: from mail-ua1-f47.google.com (mail-ua1-f47.google.com. [209.85.222.47]) by smtp.gmail.com with ESMTPSA id s144sm348839vkd.25.2020.08.12.08.27.16 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 12 Aug 2020 08:27:16 -0700 (PDT) Received: by mail-ua1-f47.google.com with SMTP id b12so713983uae.9 for ; Wed, 12 Aug 2020 08:27:16 -0700 (PDT) X-Received: by 2002:ab0:623:: with SMTP id f32mr12995280uaf.121.1597246036006; Wed, 12 Aug 2020 08:27:16 -0700 (PDT) MIME-Version: 1.0 References: <1595333413-30052-1-git-send-email-sumit.garg@linaro.org> <20200811135801.GA416071@kroah.com> <20200811145816.GA424033@kroah.com> In-Reply-To: From: Doug Anderson Date: Wed, 12 Aug 2020 08:27:04 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC 0/5] Introduce NMI aware serial drivers To: Sumit Garg X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200812_112721_110418_43780C03 X-CRM114-Status: GOOD ( 64.71 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Daniel Thompson , Greg Kroah-Hartman , Jason Wessel , Russell King - ARM Linux admin , Linux Kernel Mailing List , linux-serial@vger.kernel.org, Jiri Slaby , kgdb-bugreport@lists.sourceforge.net, linux-arm-kernel Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, On Wed, Aug 12, 2020 at 7:53 AM Sumit Garg wrote: > > Hi Doug, > > On Tue, 11 Aug 2020 at 22:46, Doug Anderson wrote: > > > > Hi, > > > > On Tue, Aug 11, 2020 at 7:58 AM Greg Kroah-Hartman > > wrote: > > > > > > On Tue, Aug 11, 2020 at 07:59:24PM +0530, Sumit Garg wrote: > > > > Hi Greg, > > > > > > > > Thanks for your comments. > > > > > > > > On Tue, 11 Aug 2020 at 19:27, Greg Kroah-Hartman > > > > wrote: > > > > > > > > > > On Tue, Aug 11, 2020 at 07:20:26PM +0530, Sumit Garg wrote: > > > > > > On Tue, 21 Jul 2020 at 17:40, Sumit Garg wrote: > > > > > > > > > > > > > > Make it possible for UARTs to trigger magic sysrq from an NMI. With the > > > > > > > advent of pseudo NMIs on arm64 it became quite generic to request serial > > > > > > > device interrupt as an NMI rather than IRQ. And having NMI driven serial > > > > > > > RX will allow us to trigger magic sysrq as an NMI and hence drop into > > > > > > > kernel debugger in NMI context. > > > > > > > > > > > > > > The major use-case is to add NMI debugging capabilities to the kernel > > > > > > > in order to debug scenarios such as: > > > > > > > - Primary CPU is stuck in deadlock with interrupts disabled and hence > > > > > > > doesn't honor serial device interrupt. So having magic sysrq triggered > > > > > > > as an NMI is helpful for debugging. > > > > > > > - Always enabled NMI based magic sysrq irrespective of whether the serial > > > > > > > TTY port is active or not. > > > > > > > > > > > > > > Currently there is an existing kgdb NMI serial driver which provides > > > > > > > partial implementation in upstream to have a separate ttyNMI0 port but > > > > > > > that remained in silos with the serial core/drivers which made it a bit > > > > > > > odd to enable using serial device interrupt and hence remained unused. It > > > > > > > seems to be clearly intended to avoid almost all custom NMI changes to > > > > > > > the UART driver. > > > > > > > > > > > > > > But this patch-set allows the serial core/drivers to be NMI aware which > > > > > > > in turn provides NMI debugging capabilities via magic sysrq and hence > > > > > > > there is no specific reason to keep this special driver. So remove it > > > > > > > instead. > > > > > > > > > > > > > > Approach: > > > > > > > --------- > > > > > > > > > > > > > > The overall idea is to intercept serial RX characters in NMI context, if > > > > > > > those are specific to magic sysrq then allow corresponding handler to run > > > > > > > in NMI context. Otherwise, defer all other RX and TX operations onto IRQ > > > > > > > work queue in order to run those in normal interrupt context. > > > > > > > > > > > > > > This approach is demonstrated using amba-pl011 driver. > > > > > > > > > > > > > > Patch-wise description: > > > > > > > ----------------------- > > > > > > > > > > > > > > Patch #1 prepares magic sysrq handler to be NMI aware. > > > > > > > Patch #2 adds NMI framework to serial core. > > > > > > > Patch #3 and #4 demonstrates NMI aware uart port using amba-pl011 driver. > > > > > > > Patch #5 removes kgdb NMI serial driver. > > > > > > > > > > > > > > Goal of this RFC: > > > > > > > ----------------- > > > > > > > > > > > > > > My main reason for sharing this as an RFC is to help decide whether or > > > > > > > not to continue with this approach. The next step for me would to port > > > > > > > the work to a system with an 8250 UART. > > > > > > > > > > > > > > > > > > > A gentle reminder to seek feedback on this series. > > > > It's been on my list for a while. I started it Friday but ran out of > > time. This week hasn't been going as smoothly as I hoped but I'll > > prioritize this since it's been too long. > > > > No worries and thanks for your feedback. > > > > > > > > It's the middle of the merge window, and I can't do anything. > > > > > > > > > > Also, I almost never review RFC patches as I have have way too many > > > > > patches that people think are "right" to review first... > > > > > > > > > > > > > Okay, I understand and I can definitely wait for your feedback. > > > > > > My feedback here is this: > > > > > > > > I suggest you work to flesh this out first and submit something that you > > > > > feels works properly. > > > > > > :) > > > > > > > IIUC, in order to make this approach substantial I need to make it > > > > work with 8250 UART (major serial driver), correct? As currently it > > > > works properly for amba-pl011 driver. > > > > > > Yes, try to do that, or better yet, make it work with all serial drivers > > > automatically. > > > > A bit of early feedback... > > > > Although I'm not sure we can do Greg's "make it work everywhere > > automatically", it's possible you could get half of your patch done > > automatically. Specifically, your patch really does two things: > > > > a) It leaves the serial port "active" all the time to look for sysrq. > > In other words even if there is no serial client it's always reading > > the port looking for characters. IMO this concept should be separated > > out from the NMI concept and _could_ automatically work for all serial > > drivers. You'd just need something in the serial core that acted like > > a default client if nobody else opened the serial port. The nice > > thing here is that we go through all the normal code paths and don't > > need special cases in the driver. > > Okay, will try to explore this option to have default serial port > client. Would this client be active in normal serial operation or only > active when we have kgdb active? One drawback I see for normal > operation could be power management as if user is not using serial > port and would like to disable corresponding clock in order to reduce > power consumption. If I could pick the ideal, I'd say we'd do it any time the console is configured for that port and magic sysrq is enabled. Presumably if they're already choosing to output kernel log messages to the serial port and they've enabled magic sysrq they're in a state where they'd be OK with the extra power of also listening for characters? > > b) It enables NMI for your particular serial driver. This seems like > > it'd be hard to do automatically because you can't do the same things > > at NMI that you could do in a normal interrupt handler. > > Agree. > > > > > NOTE: to me, a) is more important than b) (though it'd be nice to have > > both). This would be especially true the earlier you could make a) > > work since the main time when an "agetty" isn't running on my serial > > port to read characters is during bootup. > > > > Why is b) less important to me? Sure, it would let you drop into the > > debugger in the case where the CPU handling serial port interrupts is > > hung with IRQs disabled, but it _woudln't_ let you drop into the > > debugger in the case where a different CPU is hung with IRQs disabled. > > To get that we need NMI roundup (which, I know, you are also working > > on for arm64). ...and, if we've got NMI roundup, presumably we can > > find our way into the debugger by either moving the serial interrupt > > to a different CPU ahead of time or using some type of lockup detector > > (which I know you are also working on for arm64). > > > > Thanks for sharing your preferences. I will try to get a) sorted out first. > > Overall I agree with your approaches to debug hard-lockup scenarios > but they might not be so trivial for kernel engineers who doesn't > posses kernel debugging experience as you do. :) > > And I still think NMI aware magic sysrq is useful for scenarios such as: > - Try to get system information during hard-lockup rather than just > panic via hard-lockup detection. > - Do normal start/stop debugger activity on a core which was stuck in > hard-lockup. > - Random boot freezes which are not easily reproducible. Don't get me wrong. Having sysrq from NMI seems like a good feature to me. That being said, it will require non-trivial changes to each serial driver to support it and that means that not all serial drivers will support it. It also starts requiring knowledge of how NMIs work (what's allowed in NMI mode / not allowed / how to avoid races) for authors of serial drivers. I have a bit of a worry that the benefit won't outweigh the extra complexity, but I guess time will tell. One last worry is that I assume that most people testing (and even automated testing labs) will either always enable NMI or won't enable NMI. That means that everyone will be only testing one codepath or the other and (given the complexity) the non-tested codepath will break. Hrm. Along the lines of the above, though: almost no modern systems are uniprocessor. That means that even if one CPU is stuck with IRQs off it's fairly likely that some other CPU is OK. Presumably you'd get almost as much benefit as your patch but with more done automatically if you could figure out how to detect that the serial interrupt isn't being serviced and re-route it to a different CPU. ...or possibly you could use some variant of the hard lockup detector and move all interrupts off a locked up CPU? You could make this an option that's "default Y" when kgdb is turned on or something? > > One last bit of feedback is that I noticed that you didn't try to > > implement the old "knock" functionality of the old NMI driver that's > > being deleted. That is: your new patches don't provide an alternate > > way to drop into the debugger for systems where BREAK isn't hooked up. > > That's not a hard requirement, but I was kinda hoping for it since I > > have some systems that haven't routed BREAK properly. ;-) > > > > Yeah, this is on my TODO list to have a kgdb "knock" functionality to > be implemented via a common hook in serial core. > > > > > I'll try to get some more detailed feedback in the next few days. > > Thanks. I do look forward to your feedback. > > -Sumit > > > > > -Doug _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel