Re: [PATCH 1/4] serial: 8250: Avoid error message on reprobe

From: Lukas Wunner <lukas@wunner.de>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jslaby@suse.com>,
	"Matwey V. Kornilov" <matwey@sai.msu.ru>,
	Giulio Benetti <giulio.benetti@micronovasrl.com>,
	Heiko Stuebner <heiko@sntech.de>,
	Christoph Muellner <christoph.muellner@theobroma-systems.com>,
	linux-serial@vger.kernel.org
Subject: Re: [PATCH 1/4] serial: 8250: Avoid error message on reprobe
Date: Wed, 6 May 2020 08:06:23 +0200	[thread overview]
Message-ID: <20200506060623.sf3kh3fwhoawawsd@wunner.de> (raw)
In-Reply-To: <20200505160101.GV185537@smile.fi.intel.com>

On Tue, May 05, 2020 at 07:01:01PM +0300, Andy Shevchenko wrote:
> On Tue, May 05, 2020 at 04:42:01PM +0200, Lukas Wunner wrote:
> > If the call to uart_add_one_port() in serial8250_register_8250_port()
> > fails, a half-initialized entry in the serial_8250ports[] array is left
> > behind.
> > 
> > A subsequent reprobe of the same serial port causes that entry to be
> > reused.  Because uart->port.dev is set, uart_remove_one_port() is called
> > for the half-initialized entry and bails out with an error message:
> > 
> > bcm2835-aux-uart 3f215040.serial: Removing wrong port: (null) != (ptrval)
> > 
> > The same happens on failure of mctrl_gpio_init() since commit
> > 4a96895f74c9 ("tty/serial/8250: use mctrl_gpio helpers").
> > 
> > Fix by zeroing the uart->port.dev pointer in the probe error path.
> >  
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > Cc: stable@vger.kernel.org # v2.6.10+
> 
> Fixes tag?

The bug was introduced in the pre-git era, so I can't provide a Fixes tag:

https://git.kernel.org/tglx/history/c/befff6f5bf5f

This commit, which went into v2.6.10, added an unconditional
uart_remove_one_port() in serial8250_register_port().

In 2012, commit 835d844d1a28 ("8250_pnp: do pnp probe before legacy probe")
made the call to uart_remove_one_port() conditional on uart->port.dev
being non-NULL and that allows me to fix the issue by setting that
pointer to NULL in the error path.  The commit went into v3.7, so it
should be possible to fix the problem going back to v3.7 with my patch.
And before that one needs to additionally make the call to
uart_remove_one_port() conditional.

However, according to www.kernel.org the oldest LTS kernel is v3.16.
So I've given you the nitty-gritty details but it's all fairly
irrelevant and the Cc: stable tag I've put into the commit seems the
best I can do in this case.

As for your other comment:

> >  			ret = uart_add_one_port(&serial8250_reg,
> >  						&uart->port);
> > -			if (ret == 0)
> > +			if (ret)
> > +				goto err;
> 
> > +			else
> 
> Redundant.
> 
> >  				ret = uart->port.line;

Sure, I can change that.

Thanks!

Lukas