Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
From:   John Ogness <john.ogness@linutronix.de>
To:     Xuewen Yan <xuewen.yan@unisoc.com>, gregkh@linuxfoundation.org,
        jirislaby@kernel.org, ilpo.jarvinen@linux.intel.com,
        tony@atomide.com
Cc:     tglx@linutronix.de, andriy.shevchenko@linux.intel.com,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        ke.wang@unisoc.com, xuewen.yan94@gmail.com,
        Petr Mladek <pmladek@suse.com>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [RFC PATCH] serial: core: Use pm_runtime_get_sync() in
 uart_start()
In-Reply-To: <20231124122258.1050-1-xuewen.yan@unisoc.com>
References: <20231124122258.1050-1-xuewen.yan@unisoc.com>
Date:   Sun, 26 Nov 2023 17:23:26 +0106
Message-ID: <87il5o32w9.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

[Added printk maintainers CC.]

On 2023-11-24, Xuewen Yan <xuewen.yan@unisoc.com> wrote:
> The commit 84a9582fd203("serial: core: Start managing serial
> controllers to enable runtime PM") use the pm_runtime_get() after
> uart_port_lock() which would close the irq and disable preement.  At
> this time, pm_runtime_get may cause the following two problems:
>
> (1) deadlock in try_to_wake_up:
>
> uart_write()
> uart_port_lock() <<< get lock
> __uart_start
> __pm_runtime_resume
> rpm_resume
> queue_work_on
> try_to_wake_up
> _printk
> uart_console_write
> ...
> uart_port_lock() <<< wait forever

I suppose you got this because of the lockdep message generated by
#2. It probably would make sense to call __printk_safe_enter() inside
uart_port_lock(). This would allow printk() to automatically defer the
printing for that CPU until the port lock is released.

> (2) scheduling while atomic:
> uart_write()
> uart_port_lock() <<< get lock
> __uart_start
> __pm_runtime_resume
> rpm_resume
> schedule() << sleep

rpm_resume() is a fascinating function. It requires the caller to hold a
spin_lock (dev->power.lock) with interrupts disabled. But it seems to
believe that this is the *only* spin_lock held so that it can
temporarily spin_unlock and call might_sleep() functions. In the case of
uart_write(), it certainly is not the only spin_lock held.

I do not know enough about the internals of RPM to suggest a proper
solution. But it looks like rpm_resume() cannot assume dev->power.lock
is the only spin_lock held by the caller.

John Ogness