linux-kernel - Re: [PATCH] fix serdev bind/unbind

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YgNkoegU7/NoAE6v@google.com>
Date:   Tue, 8 Feb 2022 22:52:17 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Rob Herring <robh@...nel.org>
Cc:     julian schroeder <julianmarcusschroeder@...il.com>,
        bhanumaiya@...gle.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
        Johan Hovold <johan@...nel.org>
Subject: Re: [PATCH] fix serdev bind/unbind

[ resending as I managed to lose folks from CC list ]

Hi Rob,

On Fri, Jan 21, 2022 at 11:51 AM Rob Herring <robh@...nel.org> wrote:
>
> +Johan
>
> On Tue, Jan 18, 2022 at 1:47 PM julian schroeder
> <julianmarcusschroeder@...il.com> wrote:
> >
> > On some chromebooks, the serdev is used to communicate with
> > an embedded controller. When the controller is updated, the
> > regular ttyS* is needed. Therefore unbind/bind needs to work
> > to be able to switch between the two modes without having to
> > reboot. In the case of ACPI enabled platforms, the underlying
> > serial device is marked as enumerated but this is not cleared
> > upon remove (unbind). In this state it can not be bound as
> > serdev.
>
> 'fix' implies this was supposed to work and doesn't, but unbind/bind
> was never a feature of serdev. Or more specifically, switching between
> serdev and tty was not a feature. There have been some attempts to add
> that. I suspect it is more than a 4 line change based on those, but
> maybe I'm wrong.
>
> For your usecase, how does a given piece of h/w that needs and/or
> provides kernel support continue to work when the driver is unbound.
> Are you leaving any power controls that the serdev driver configured
> enabled so that the tty happens to keep working? What happens to

Because we are dealing with EC it stays powered up even when main CPU
is powered down, so for the core EC there are no concerns with power
management in the absence of a [dedicated] driver.

> interfaces the EC provides? The kernel doesn't deal with resources
> going away too well. I have to wonder if the existing serdev EC driver
> should learn to handle the 'update mode' itself or provide some sort
> of raw/passthru mode to userspace. A TTY, while standard, brings a lot
> of complexities.

I think these are all very good questions and from what I see in
drivers/platform/chrome/cros_ec_uart.c we will simply yank services
that the EC provides while it is being updated (which is quite
reasonable behavior as we can not be sure what configuration we will end
up with once firmware is updated, so new discovery of interfaces and
their characteristics is needed and is prudent). So from the outside a
dedicated update mode or attempting to switch to using tty interface
would look pretty similar.

That said, we can forget about EC and switching from serdev to tty here
and concentrate on the simple fact that for serdev a simple bind/unbind
sequence is not working, and that is a basic functionality for pretty
much every bus that we have in the kernel and the patch does address
this deficiency.

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@...il.com>

>
> > Signed-off-by: julian schroeder <julianmarcusschroeder@...il.com>
> > ---
> >  drivers/tty/serdev/core.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> > index 92e3433276f8..668fa570bc07 100644
> > --- a/drivers/tty/serdev/core.c
> > +++ b/drivers/tty/serdev/core.c
> > @@ -138,7 +138,11 @@ EXPORT_SYMBOL_GPL(serdev_device_add);
> >  void serdev_device_remove(struct serdev_device *serdev)
> >  {
> >         struct serdev_controller *ctrl = serdev->ctrl;
> > +       struct acpi_device *adev;
> >
> > +       adev = ACPI_COMPANION(&serdev->dev);
> > +       if (adev)
> > +               acpi_device_clear_enumerated(adev);
> >         device_unregister(&serdev->dev);
> >         ctrl->serdev = NULL;
> >  }
> > --
> > 2.20.1
> >

Thanks.

-- 
Dmitry