From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Okash Khawaja <okash.khawaja@gmail.com>
Cc: devel@driverdev.osuosl.org, Kirk Reiser <kirk@reisers.ca>,
Alan Cox <gnomes@lxorguk.ukuu.org.uk>,
"Speakup is a screen review system for Linux."
<speakup@linux-speakup.org>,
linux-kernel@vger.kernel.org, Jiri Slaby <jslaby@suse.com>,
Samuel Thibault <samuel.thibault@ens-lyon.org>,
Chris Brannon <chris@the-brannons.com>
Subject: Re: [patch 2/6] tty: export tty_open_by_driver
Date: Mon, 15 May 2017 13:29:45 +0200 [thread overview]
Message-ID: <20170515112945.GA7807@kroah.com> (raw)
In-Reply-To: <CAOtcWM3zVhGyvvLm3f-7PuFr4p07e1CdWA0fXBZz38hGC=h5-Q@mail.gmail.com>
On Mon, May 15, 2017 at 12:10:06PM +0100, Okash Khawaja wrote:
> Hi,
>
> On Mon, May 15, 2017 at 11:31 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Apr 29, 2017 at 08:52:59PM +0100, Okash Khawaja wrote:
> >> This applies on top of the changes already in staging-next branch which allow
> >> kernel access to TTY dev.
> >>
> >> Signe-doff-by: Okash Khawaja <okash.khawaja@gmail.com>
> >> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> >>
> >> Index: linux-staging/drivers/tty/tty_io.c
> >> ===================================================================
> >> --- linux-staging.orig/drivers/tty/tty_io.c
> >> +++ linux-staging/drivers/tty/tty_io.c
> >> @@ -1369,7 +1369,10 @@ static struct tty_struct *tty_driver_loo
> >> struct tty_struct *tty;
> >>
> >> if (driver->ops->lookup)
> >> - tty = driver->ops->lookup(driver, file, idx);
> >> + if (!file)
> >> + tty = ERR_PTR(-EIO);
> >> + else
> >> + tty = driver->ops->lookup(driver, file, idx);
> >
> > Why make this change? Shouldn't the lookup function allow a NULL file
> > pointer? Or is the problem that they do not?
> >
> >> else
> >> tty = driver->ttys[idx];
> >>
> >> @@ -2001,7 +2004,7 @@ static struct tty_driver *tty_lookup_dri
> >> struct tty_driver *console_driver = console_device(index);
> >> if (console_driver) {
> >> driver = tty_driver_kref_get(console_driver);
> >> - if (driver) {
> >> + if (driver && filp) {
> >
> > Why change this too?
> >
> > Your changelog does not explain any of this, please do so.
> >
> > thanks,
> >
> > greg k-h
>
>
> Sorry, I should have been more descriptive here. The changes which
> check file pointer for null are basically from Alan Cox's patch here:
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1215095.html.
> The description from that patch is quoted below:
>
> "[RFC] tty_port: allow a port to be opened with a tty that has no file handle
>
> Let us create tty objects entirely in kernel space. Untested proposal to
> show why all the ideas around rewriting half the uart stack are not needed.
>
> With this a kernel created non file backed tty object could be used to
> handle
> data, and set terminal modes. Not all ldiscs can cope with this as N_TTY in
> particular has to work back to the fs/tty layer.
>
> The tty_port code is however otherwise clean of file handles as far as I can
> tell as is the low level tty port write path used by the ldisc, the
> configuration low level interfaces and most of the ldiscs.
>
> Currently you don't have any exposure to see tty hangups because those are
> built around the file layer. However a) it's a fixed port so you probably
> don't care about that b) if you do we can add a callback and c) you almost
> certainly don't want the userspace tear down/rebuild behaviour anyway.
>
> This should however be sufficient if we wanted for example to enumerate all
> the bluetooth bound fixed ports via ACPI and make them directly available.
>
> It doesn't deal with the case of a user opening a port that's also kernel
> opened and that would need some locking out (so it returned EBUSY if bound
> to a kernel device of some kind). That needs resolving along with how you
> "up" or "down" your new bluetooth device, or enumerate it while providing
> the existing tty API to avoid regressions (and to debug)."
>
> With this patchset tty_open_by_driver is now called from inside kernel
> with file pointer set to null. I can resend this patch with above
> description.
Please fix that up and resend the whole series.
thanks,
greg k-h
next prev parent reply other threads:[~2017-05-15 11:29 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-29 19:52 [patch 0/6] staging: speakup: migrate synths to use TTY-based comms Okash Khawaja
2017-04-29 19:52 ` [patch 1/6] staging: speakup: make input functionality swappable Okash Khawaja
2017-04-29 19:52 ` [patch 2/6] tty: export tty_open_by_driver Okash Khawaja
2017-05-15 10:31 ` Greg Kroah-Hartman
2017-05-15 11:10 ` Okash Khawaja
2017-05-15 11:29 ` Greg Kroah-Hartman [this message]
2017-04-29 19:53 ` [patch 3/6] staging: speakup: add tty-based comms functions Okash Khawaja
2017-04-29 19:53 ` [patch 4/6] staging: speakup: migrate acntsa, bns, dummy and txprt to ttyio Okash Khawaja
2017-04-29 19:53 ` [patch 5/6] staging: speakup: add send_xchar, tiocmset and input functionality for tty Okash Khawaja
2017-04-29 19:53 ` [patch 6/6] staging: speakup: migrate apollo, ltlk, audptr, decext, dectlk and spkout Okash Khawaja
2017-04-29 19:58 ` [patch 0/6] staging: speakup: migrate synths to use TTY-based comms Okash Khawaja
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=20170515112945.GA7807@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=chris@the-brannons.com \
--cc=devel@driverdev.osuosl.org \
--cc=gnomes@lxorguk.ukuu.org.uk \
--cc=jslaby@suse.com \
--cc=kirk@reisers.ca \
--cc=linux-kernel@vger.kernel.org \
--cc=okash.khawaja@gmail.com \
--cc=samuel.thibault@ens-lyon.org \
--cc=speakup@linux-speakup.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).