Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()

From: Johan Hovold <johan@kernel.org>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Johan Hovold <johan@kernel.org>,
	Jacek Anaszewski <jacek.anaszewski@gmail.com>,
	Pavel Machek <pavel@ucw.cz>, Dan Murphy <dmurphy@ti.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jslaby@suse.com>,
	linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
	kernel@pengutronix.de, linux-serial@vger.kernel.org
Subject: Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()
Date: Wed, 19 Feb 2020 18:17:59 +0100	[thread overview]
Message-ID: <20200219171759.GE32540@localhost> (raw)
In-Reply-To: <20200219163758.5rypsol4n6ucost4@pengutronix.de>

On Wed, Feb 19, 2020 at 05:37:58PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 19, 2020 at 02:21:13PM +0100, Johan Hovold wrote:
> > On Thu, Feb 13, 2020 at 10:15:58AM +0100, Uwe Kleine-König wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > 
> > > Introduce a new function tty_kopen_shared() that yields a struct
> > > tty_struct. The semantic difference to tty_kopen() is that the tty is
> > > expected to be used already. So rename tty_kopen() to
> > > tty_kopen_exclusive() for clearness, adapt the single user and put the
> > > common code in a new static helper function.
> > > 
> > > tty_kopen_shared is to be used to implement an LED trigger for tty
> > > devices in one of the next patches.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> >  
> > > -/**
> > > - *	tty_kopen	-	open a tty device for kernel
> > > - *	@device: dev_t of device to open
> > > - *
> > > - *	Opens tty exclusively for kernel. Performs the driver lookup,
> > > - *	makes sure it's not already opened and performs the first-time
> > > - *	tty initialization.
> > > - *
> > > - *	Returns the locked initialized &tty_struct
> > > - *
> > > - *	Claims the global tty_mutex to serialize:
> > > - *	  - concurrent first-time tty initialization
> > > - *	  - concurrent tty driver removal w/ lookup
> > > - *	  - concurrent tty removal from driver table
> > > - */
> > > -struct tty_struct *tty_kopen(dev_t device)
> > > +static struct tty_struct *tty_kopen(dev_t device, int shared)
> > >  {
> > >  	struct tty_struct *tty;
> > >  	struct tty_driver *driver;
> > > @@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
> > >  
> > >  	/* check whether we're reopening an existing tty */
> > >  	tty = tty_driver_lookup_tty(driver, NULL, index);
> > > -	if (IS_ERR(tty))
> > > +	if (IS_ERR(tty) || shared)
> > 
> > So here you skip initialisation and return NULL if the tty isn't already
> > in use (e.g. is open) when shared is set.
> 
> Which is good, right? If I remember my tests correctly this even works
> if the tty isn't opened but just "exists".

No, this means that your trigger will never be installed unless the port
is already open, yet the sysfs interface still returns success (see
patch 4/4 dev_store()).

Note that the struct tty doesn't exist until the port is opened; it's
allocated in tty_init_dev() that you skip above when "shared" is set.

Johan