Message-ID: <CAJ_BA_CZWvC2=i8riNe5LReLKzPXK1vPwymiG2dzLEntda7TRg@mail.gmail.com>
Date: Tue, 1 Jul 2025 12:29:51 +0200
From: Dawid Niedźwiecki <dawidn@...gle.com>
To: Tzung-Bi Shih <tzungbi@...nel.org>
Cc: Benson Leung <bleung@...omium.org>, chrome-platform@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, chromeos-krk-upstreaming@...gle.com, 
	Łukasz Bartosik <ukaszb@...omium.org>
Subject: Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

>What are the shortcomings if it re-creates /dev/cros_X everytime?

In that case, to avoid memory leakage, you would need to free the
cros_ec_device structure every disconnect.
An application with an opened cros_ec file would try to access the
freed resources (use-after-free) via the char driver [1], e.g.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/chrome/cros_ec_chardev.c#n345

It would also require additional work from the userspace application
e.g. reopen the cros_ec file every time there was a communication
problem. I think recovering an already created file sounds like a
better idea.

> Isn't it also a way for userland programs to be aware of the EC device crashes?

No. First of all, we don't know a reason for the disconnect / reprobe,
so it is not always a crash. Also we signal that there is a problem
with the connection by returning -ENODEV when an EC device is
disconnected.
There are dedicated commands to check reboot reason, and detect
potential crashes.

> Why other cros_ec_X drivers doesn't need the mechanism?

It is a very USB specific problem. USB host/hub detects a new device
on the bus and starts probing procedure.
It is not a case for other drivers, e.g. SPI probes only once, at the
beginning of the boot, based on ACPI entry, there is no hardware
detection that a device is alive after reboot.
Additionally, the USB related structures are new every probe, so we
need to replace those.

On Tue, Jul 1, 2025 at 10:56 AM Tzung-Bi Shih <tzungbi@...nel.org> wrote:
>
> On Mon, Jun 30, 2025 at 01:59:39PM +0200, Dawid Niedźwiecki wrote:
> > On Fri, Jun 27, 2025 at 9:53 AM Tzung-Bi Shih <tzungbi@...nel.org> wrote:
> > > On Tue, Jun 24, 2025 at 11:00:28AM +0000, Dawid Niedzwiecki wrote:
> > > > +     /*
> > > > +      * Do not register the same EC device twice. The probing is performed every
> > > > +      * reboot, sysjump, crash etc. Recreating the /dev/cros_X file every time
> > > > +      * would force all application to reopen the file, which is not a case for
> > > > +      * other cros_ec_x divers. Instead, keep the cros_ec_device and cros_ec_usb
> > > > +      * structures constant and replace USB related structures for the same EC
> > > > +      * that is reprobed.
> > > > +      *
> > > > +      * The driver doesn't support handling two devices with the same idProduct,
> > > > +      * but it will never be a real usecase.
> > > > +      */
> > >
> > > I don't quite understand why does it need to memorize the registered ECs.
> > > Supposedly, the probe function is only called few times during booting, and
> > > gets success once. Hot-plugs?
> > >
> >
> > The probe is called every time the EC device boots from the beginning
> > - sysjumps, crashes, reboots etc. It succeeds the first time.
> > Once the /dev/cros_X file is created, we need the possibility to
> > access the same EC device, with the same, previously created file.
> > The only way to do that is to reused the already created
> > cros_ec_device structure.
>
> What are the shortcomings if it re-creates /dev/cros_X everytime? Isn't it
> also a way for userland programs to be aware of the EC device crashes?
>
> Why other cros_ec_X drivers doesn't need the mechanism?

Powered by blists - more mailing lists