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 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"?

> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
> Changes from v2 (https://patchwork.kernel.org/project/chrome-platform/patch/20250708080034.3425427-4-tzungbi@kernel.org/):
> - Use atomic_t for `registered` so that it doesn't need to be handled with
>   locks.
> - Add new helper function cros_ec_device_registered().
> 
>  drivers/platform/chrome/cros_ec.c           |  5 +++++
>  drivers/platform/chrome/cros_ec_proto.c     | 17 +++++++++++++++++
>  include/linux/platform_data/cros_ec_proto.h |  4 ++++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index fd58781a2fb7..42512e2b4f65 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -9,6 +9,7 @@
>   * battery charging and regulator control, firmware update.
>   */
>  
> +#include <linux/atomic.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/of_platform.h>
> @@ -200,6 +201,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  	if (!ec_dev->dout)
>  		return -ENOMEM;
>  
> +	atomic_set(&ec_dev->registered, 1);
>  	lockdep_register_key(&ec_dev->lockdep_key);
>  	mutex_init(&ec_dev->lock);
>  	lockdep_set_class(&ec_dev->lock, &ec_dev->lockdep_key);
> @@ -300,6 +302,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>  
>  	return 0;
>  exit:
> +	atomic_set(&ec_dev->registered, 0);
>  	platform_device_unregister(ec_dev->ec);
>  	platform_device_unregister(ec_dev->pd);
>  	mutex_destroy(&ec_dev->lock);
> @@ -318,6 +321,8 @@ EXPORT_SYMBOL(cros_ec_register);
>   */
>  void cros_ec_unregister(struct cros_ec_device *ec_dev)
>  {
> +	atomic_set(&ec_dev->registered, 0);
> +
>  	if (ec_dev->mkbp_event_supported)
>  		blocking_notifier_chain_unregister(&ec_dev->event_notifier,
>  						   &ec_dev->notifier_ready);
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 3e94a0a82173..ee3050ffe226 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -3,6 +3,7 @@
>  //
>  // Copyright (C) 2015 Google, Inc
>  
> +#include <linux/atomic.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/limits.h>
> @@ -656,6 +657,11 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
>  {
>  	int ret;
>  
> +	if (!cros_ec_device_registered(ec_dev)) {
> +		dev_err(ec_dev->dev, "unregistered ec_dev; cannot send command\n");
> +		return -ENODEV;
> +	}
> +
>  	mutex_lock(&ec_dev->lock);
>  	if (ec_dev->proto_version == EC_PROTO_VERSION_UNKNOWN) {
>  		ret = cros_ec_query_all(ec_dev);
> @@ -1153,5 +1159,16 @@ int cros_ec_get_cmd_versions(struct cros_ec_device *ec_dev, u16 cmd)
>  }
>  EXPORT_SYMBOL_GPL(cros_ec_get_cmd_versions);
>  
> +/**
> + * 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.

If you really need this (and huge hint, I do not think you do, as
something must be wrong if you ever need it), use a lock properly, not
just an atomic value, as that doesn't work here at all, and is kind of
pointless.

So step back, what exactly is the problem here that you are trying to
solve with this?

thanks,

greg k-h