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

Hi John

On Mon, Nov 27, 2023 at 12:17=E2=80=AFAM John Ogness <john.ogness@linutroni=
x.de> wrote:
>
> [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.

Thanks for the suggestion, I would use printk_deferred in our tree to
retest the case.

And I also notice the warning was reported by syzbot:
https://lore.kernel.org/all/0000000000006f01f00608a16cea@google.com/
https://lore.kernel.org/all/000000000000e7765006072e9591@google.com/

>
> > (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.

I would also be very grateful if could give us more suggestions.

Thanks!

BR
---
xuewen
>
> John Ogness