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

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

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