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

[Johan Hovold]

On Thu, Feb 20, 2020 at 12:04:27PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 19, 2020 at 06:17:59PM +0100, Johan Hovold wrote:
> > 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@xxxxxxxxxxxxxx>
> > > > > 
> > > > > 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@xxxxxxxxxxxxxx>
> > > > > ---
> > > >  
> > > > > -/**
> > > > > - *	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()).
> 
> Ah I think I see. tty_driver_lookup_tty() might return NULL which for
> the trigger driver indicates that tty_kopen_shared should be retried,
> right?

I'm not sure how best to handle this, but yes, your trigger can only be
enabled while the port is open currently. And no error is returned to a
user trying to enable the trigger before it has been opened.

But regardless of the error reporting, I don't think failing when the
port isn't open is the right thing to do as as this makes the interface
rather useless since you cannot enable a trigger from for example a udev
rule.

If this approach is to be used at all, it seems you may need to allocate
the struct tty when the trigger is enabled. And make sure you don't mess
up some other assumption in tty core...
 
> > 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.
> 
> That needs fixing. I will try to resolve the dialog with Andy before
> addressing that in the next iteration.

Johan