Re: [patch 2/6] tty: export tty_open_by_driver

[Greg Kroah-Hartman]

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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> >> Reviewed-by: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxx/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