Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
From: Johan Hovold <johan@kernel.org>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Johan Hovold <johan@kernel.org>,
	kernel@pengutronix.de, linux-serial@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, Dan Murphy <dmurphy@ti.com>,
	Pavel Machek <pavel@ucw.cz>, Jiri Slaby <jslaby@suse.com>,
	linux-leds@vger.kernel.org,
	Jacek Anaszewski <jacek.anaszewski@gmail.com>
Subject: Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()
Date: Tue, 25 Feb 2020 09:55:03 +0100
Message-ID: <20200225085503.GP32540@localhost> (raw)
In-Reply-To: <20200220110427.e6jgzvdhh3ysql3n@pengutronix.de>

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

  reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-13  9:15 [PATCH v6 0/4] leds: trigger: implement a tty trigger Uwe Kleine-König
2020-02-13  9:15 ` [PATCH v6 1/4] lib: new helper kstrtodev_t() Uwe Kleine-König
2020-02-19 19:50   ` Andy Shevchenko
2020-02-20  7:49     ` Uwe Kleine-König
2020-02-20 10:22       ` Andy Shevchenko
2020-02-20 10:57         ` Uwe Kleine-König
2020-02-20 11:46           ` Andy Shevchenko
2020-02-20 11:57             ` Andy Shevchenko
2020-02-20 14:01             ` Uwe Kleine-König
2020-02-21  8:42               ` Andy Shevchenko
2020-02-21 10:53                 ` Uwe Kleine-König
2020-02-13  9:15 ` [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared() Uwe Kleine-König
2020-02-19 13:21   ` Johan Hovold
2020-02-19 16:37     ` Uwe Kleine-König
2020-02-19 17:17       ` Johan Hovold
2020-02-20 11:04         ` Uwe Kleine-König
2020-02-25  8:55           ` Johan Hovold [this message]
2020-02-25  9:05             ` Uwe Kleine-König
2020-02-13  9:15 ` [PATCH v6 3/4] tty: new helper function tty_get_icount() Uwe Kleine-König
2020-02-13  9:16 ` [PATCH v6 4/4] leds: trigger: implement a tty trigger Uwe Kleine-König
2020-02-19 10:52   ` Johan Hovold
2020-02-19 11:03     ` Uwe Kleine-König
2020-02-19 11:19       ` Johan Hovold
2020-02-19 12:48         ` Johan Hovold
2020-02-19 10:40 ` [PATCH v6 0/4] " Greg Kroah-Hartman
2020-02-26 14:02   ` Pavel Machek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200225085503.GP32540@localhost \
    --to=johan@kernel.org \
    --cc=dmurphy@ti.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jacek.anaszewski@gmail.com \
    --cc=jslaby@suse.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=u.kleine-koenig@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Serial Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-serial/0 linux-serial/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-serial linux-serial/ https://lore.kernel.org/linux-serial \
		linux-serial@vger.kernel.org
	public-inbox-index linux-serial

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-serial


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git