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

On Thu, May 21, 2026 at 10:58:30AM -0300, Jason Gunthorpe wrote:
> On Sat, May 16, 2026 at 10:30:17PM +0800, Tzung-Bi Shih wrote:
> > @@ -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.

Fixed those in the next version [1].

Just a note, the code still uses -ENODEV instead of -ENXIO if you have no
objection.

> > @@ -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.

Fixed in the next version [1].

[1] https://lore.kernel.org/all/[email protected]