[PATCH] fix serdev bind/unbind

[PATCH] fix serdev bind/unbind

[julian schroeder]

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.

Signed-off-by: julian schroeder <julianmarcusschroeder@gmail.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

Re: [PATCH] fix serdev bind/unbind

[Rob Herring]

+Johan

On Tue, Jan 18, 2022 at 1:47 PM julian schroeder
<julianmarcusschroeder@gmail.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
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.

> Signed-off-by: julian schroeder <julianmarcusschroeder@gmail.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
>

Re: [PATCH] fix serdev bind/unbind

[Andy Shevchenko]

On Fri, Jan 21, 2022 at 8:55 AM julian schroeder
<julianmarcusschroeder@gmail.com> wrote:
>
> On some chromebooks, the serdev is used to communicate with

Chromebooks ?

> 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.

Seems legit (we do this for i2c and spi serial buses in ACPI case).
After addressing the following nit-pick
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

...

>  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);

As I mentioned i2c and SPI cases, I think it would be nice to use same
pattern of this code, i.e.


  if (ACPI_COMPANION(&serdev->dev))
    acpi_device_clear_enumerated(ACPI_COMPANION(&serdev->dev));

drivers/i2c/i2c-core-base.c, line 1007
drivers/spi/spi.c, line 779

>         device_unregister(&serdev->dev);
>         ctrl->serdev = NULL;
>  }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] fix serdev bind/unbind

[Dmitry Torokhov]

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

Hi Rob,

On Fri, Jan 21, 2022 at 11:51 AM Rob Herring <robh@kernel.org> wrote:
>
> +Johan
>
> On Tue, Jan 18, 2022 at 1:47 PM julian schroeder
> <julianmarcusschroeder@gmail.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@gmail.com>

>
> > Signed-off-by: julian schroeder <julianmarcusschroeder@gmail.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