Re: [PATCH v2 4/4] platform/chrome: cros_ec_chardev: Introduce rwsem for protecting ec_dev

[Jason Gunthorpe]

On Sat, May 16, 2026 at 10:30:17PM +0800, Tzung-Bi Shih wrote:
> @@ -94,12 +96,19 @@ static int ec_get_version(struct chardev_priv *priv, char *str, int maxlen)
>  	msg->command = EC_CMD_GET_VERSION + priv->pdata->cmd_offset;
>  	msg->insize = sizeof(*resp);
>  
> -	ret = cros_ec_cmd_xfer_status(priv->pdata->ec_dev, msg);
> -	if (ret < 0) {
> -		snprintf(str, maxlen,
> -			 "Unknown EC version, returned error: %d\n",
> -			 msg->result);
> -		goto exit;
> +	scoped_guard(rwsem_read, &priv->pdata->ec_dev_sem) {

I would not use scoped_guard here, especially since it isn't used
consistently

> +		if (!priv->pdata->ec_dev) {
> +			ret = -ENODEV;
> +			goto exit;
> +		}
> +
> +		ret = cros_ec_cmd_xfer_status(priv->pdata->ec_dev, msg);
> +		if (ret < 0) {
> +			snprintf(str, maxlen,
> +				 "Unknown EC version, returned error: %d\n",
> +				 msg->result);
> +			goto exit;
> +		}
>  	}
>  
>  	resp = (struct ec_response_get_version *)msg->data;
> @@ -122,10 +131,18 @@ static int cros_ec_chardev_mkbp_event(struct notifier_block *nb,
>  {
>  	struct chardev_priv *priv = container_of(nb, struct chardev_priv,
>  						 notifier);
> -	struct cros_ec_device *ec_dev = priv->pdata->ec_dev;
> +	struct cros_ec_device *ec_dev;
>  	struct ec_event *event;
> -	unsigned long event_bit = 1 << ec_dev->event_data.event_type;
> -	int total_size = sizeof(*event) + ec_dev->event_size;
> +	unsigned long event_bit;
> +	int total_size;
> +
> +	guard(rwsem_read)(&priv->pdata->ec_dev_sem);
> +	if (!priv->pdata->ec_dev)
> +		return NOTIFY_DONE;
> +	ec_dev = priv->pdata->ec_dev;
> +
> +	event_bit = 1 << ec_dev->event_data.event_type;
> +	total_size = sizeof(*event) + ec_dev->event_size;
>  
>  	if (!(event_bit & priv->event_mask) ||
>  	    (priv->event_len + total_size) > CROS_MAX_EVENT_LEN)
> @@ -206,8 +223,11 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
>  	ret = blocking_notifier_chain_register(&pdata->subscribers,
>  					       &priv->notifier);
>  	if (ret) {
> -		dev_err(pdata->ec_dev->dev,
> -			"failed to register event notifier\n");
> +		scoped_guard(rwsem_read, &pdata->ec_dev_sem) {
> +			if (pdata->ec_dev)
> +				dev_err(pdata->ec_dev->dev,
> +					"failed to register event notifier\n");
> +		}

open is in a context where dev_dev has to be valid, don't add
pointless locking.

If you want to have an assertion it should just be this at the top of the function:

	if (WARN_ON(!priv->pdata->ec_dev))
		return -ENXIO;

> @@ -330,10 +350,18 @@ static long cros_ec_chardev_ioctl_xcmd(struct chardev_priv *priv, void __user *a
>  	}
>  
>  	s_cmd->command += priv->pdata->cmd_offset;
> -	ret = cros_ec_cmd_xfer(priv->pdata->ec_dev, s_cmd);
> -	/* Only copy data to userland if data was received. */
> -	if (ret < 0)
> -		goto exit;
> +
> +	scoped_guard(rwsem_read, &priv->pdata->ec_dev_sem) {
> +		if (!priv->pdata->ec_dev) {
> +			ret = -ENODEV;
> +			goto exit;
> +		}

Same remark, don't use scoped_guard. Each fops should simply start
with:

	guard(rwsem_read)(&priv->pdata->ec_dev_sem);
	if (!priv->pdata->ec_dev)
		return -ENXIO;

There is no point in trying to carefully partially do some part of the
ioctl of the driver has been removed.

>  static long cros_ec_chardev_ioctl_readmem(struct chardev_priv *priv, void __user *arg)
>  {
> -	struct cros_ec_device *ec_dev = priv->pdata->ec_dev;
> +	struct cros_ec_device *ec_dev;
>  	struct cros_ec_readmem s_mem = { };
>  	long num;
>  
> +	guard(rwsem_read)(&priv->pdata->ec_dev_sem);
> +	if (!priv->pdata->ec_dev)
> +		return -ENODEV;
> +	ec_dev = priv->pdata->ec_dev;

And it should be placed close to the top of the fop, not down inside
every child function.

Your mental model is still thinking about "revocable" this is really
entering a driver bound context at the start of every fop. Then
everything during the fop gets to assume the driver bound invariant.


> @@ -451,6 +485,8 @@ static void cros_ec_chardev_remove(struct platform_device *pdev)
>  
>  	blocking_notifier_chain_unregister(&pdata->ec_dev->event_notifier,
>  					   &pdata->relay);
> +	scoped_guard(rwsem_write, &pdata->ec_dev_sem)
> +		pdata->ec_dev = NULL;

This seems out of order.

  	misc_deregister(&pdata->misc);

^^ Is first because it stops new fops from being created

 +	scoped_guard(rwsem_write, &pdata->ec_dev_sem)
 +		pdata->ec_dev = NULL;

^^ Stops existing fops from running

Then you can go on to destroy the notifier chain and so on as there is
now no concurrent touches to pdata.

Jason