Received-SPF: pass (google.com: domain of linux-kernel+bounces-135527-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 8 Apr 2024 17:36:39 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Yicong Yang <yangyicong@huawei.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	yangyicong@hisilicon.com, Jiri Slaby <jirislaby@kernel.org>,
	Tony Lindgren <tony@atomide.com>,
	kernel test robot <oliver.sang@intel.com>
Subject: Re: [PATCH v1 1/1] serial: core: Clearing the circular buffer before
 NULLifying it
Message-ID: <ZhQA95sHWoaWnq07@smile.fi.intel.com>
References: <20240404150034.41648-1-andriy.shevchenko@linux.intel.com>
 <b3fd1077-b49e-d99b-9cd1-c41bd244f290@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b3fd1077-b49e-d99b-9cd1-c41bd244f290@huawei.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Sun, Apr 07, 2024 at 05:49:19PM +0800, Yicong Yang wrote:
> On 2024/4/4 22:59, Andy Shevchenko wrote:
> > The circular buffer is NULLified in uart_tty_port_shutdown()
> > under the spin lock. However, the PM or other timer based callbacks
> > may still trigger after this event without knowning that buffer pointer
> > is not valid. Since the serial code is a bit inconsistent in checking
> > the buffer state (some rely on the head-tail positions, some on the
> > buffer pointer), it's better to have both aligned, i.e. buffer pointer
> > to be NULL and head-tail possitions to be the same, meaning it's empty.
> > This will prevent asynchronous calls to dereference NULL pointer as
> > reported recently in 8250 case:
> > 
> >   BUG: kernel NULL pointer dereference, address: 00000cf5
> >   Workqueue: pm pm_runtime_work
> >   EIP: serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
> >   ...
> >   ? serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
> >   __start_tx (drivers/tty/serial/8250/8250_port.c:1551)
> >   serial8250_start_tx (drivers/tty/serial/8250/8250_port.c:1654)
> >   serial_port_runtime_suspend (include/linux/serial_core.h:667 drivers/tty/serial/serial_port.c:63)
> >   __rpm_callback (drivers/base/power/runtime.c:393)
> >   ? serial_port_remove (drivers/tty/serial/serial_port.c:50)
> >   rpm_suspend (drivers/base/power/runtime.c:447)
> > 
> > The proposed change will prevent ->start_tx() to be called during
> > suspend on shut down port.
> 
> Just saw the issue and thanks for your timely fix. I didn't got a board with
> 8250 and sorry for didn't found this issue.

But does this change make no regression in your case? Can you test it?

> FYI, I checked device_shutdown() and seems it called pm_runtime_barrier() for waiting all
> the scheduled RPM callbacks finished and keep the device in resume state. So ideally there
> shouldn't be any pending requests later since we handled them before shutdown?
> 
> There's someone encountered the same issue in shutdown() due to runtime pm and fixed it in
> 	af8db1508f2c ("PM / driver core: disable device's runtime PM during shutdown")
> patch above seems to still have some problem and later fixed by:
> 	fe6b91f47080 ("PM / Driver core: leave runtime PM enabled during system shutdown")

Ah, yes, thanks for reminding (yeah, I saw those patches, let me test it on my setup.

> But seems the handling in the driver core doesn't cover the case here..

Of course, since we have our own shutdown on the upper level, we don't kill the
device, but we do release _some_ resources.

-- 
With Best Regards,
Andy Shevchenko