Re: [PATCH v3 4/8] platform/chrome: Disallow sending commands through unregistered ec_dev

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Jul 21, 2025 at 09:31:05AM +0000, Tzung-Bi Shih wrote:
> On Mon, Jul 21, 2025 at 07:47:47AM +0200, Greg KH wrote:
> > On Mon, Jul 21, 2025 at 04:44:52AM +0000, Tzung-Bi Shih wrote:
> > > Return earlier if attempting to send commands through an unregistered
> > > struct cros_ec_device.
> > 
> > Why would it ever be "uregistered"?
> 
> If the EC rebooted while a userland program opened a file of the
> chardev in cros_ec_chardev, the struct cros_ec_device [1][2] may be
> unregistered due to some drivers re-probe the device.

Ah, the fun "disconnect the device while the file is open" issue we have
in many subsystems :(

> The issue mainly comes from the discussion of cros_ec_usb [3]: using a
> `registered_list` vs. calling cros_ec_{un,}register() everytime in
> usb_driver's .probe()/.disconnect().
> 
> [1] https://elixir.bootlin.com/linux/v6.15/source/drivers/platform/chrome/cros_ec_chardev.c#L241
> [2] https://elixir.bootlin.com/linux/v6.15/source/drivers/platform/chrome/cros_ec_chardev.c#L71
> [3] https://lore.kernel.org/chrome-platform/20250624110028.409318-1-dawidn@google.com/T/#u
> 
> > > +/**
> > > + * cros_ec_device_registered - Check if the ec_dev is registered.
> > > + *
> > > + * @ec_dev: EC device
> > > + */
> > > +bool cros_ec_device_registered(struct cros_ec_device *ec_dev)
> > > +{
> > > +	return atomic_read(&ec_dev->registered) == 1;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cros_ec_device_registered);
> > 
> > This isn't going to do what you think it does :(
> > 
> > Hint, the state can change right after you call this, making it
> > pointless.
> 
> In current use cases, once it turns from "registered" to "unregistered", it
> should never back to "registered".  The flag is more or less to let the
> kernel know the struct cros_ec_device is stale.  The object is still valid
> for accessing because an opening file instance is referencing.

But again, you don't know if it just went from "registered" ->
"unregistered" after calling this function and getting a return value of
"I am registered!".

Do it right, with a real lock, an atomic value provides nothing here
other than the potential to reduce a race window, not actually prevent
it from happening at all.

thanks,

greg k-h