Re: [PATCH v6 2/2] platform/chrome: cros_ec_chardev: Consume cros_ec_device via revocable

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Thu, Nov 06, 2025 at 11:59:51AM -0400, Jason Gunthorpe wrote:
> On Thu, Nov 06, 2025 at 11:26:02PM +0800, Tzung-Bi Shih wrote:
> > @@ -166,7 +181,12 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
> >  	if (!priv)
> >  		return -ENOMEM;
> >  
> > -	priv->ec_dev = ec_dev;
> > +	priv->ec_dev_rev = revocable_alloc(ec_dev->revocable_provider);
> > +	if (!priv->ec_dev_rev) {
> > +		ret = -ENOMEM;
> > +		goto free_priv;
> > +	}
> 
> The lifecyle of ec_dev->ec_dev->revocable_provider memory is
> controlled by dev:
> 
> +       ec_dev->revocable_provider = devm_revocable_provider_alloc(dev, ec_dev);
> 
> Under the lifecycle of some other driver.
> 
> The above only works because misc calls open under the misc_mtx so it
> open has "sync" behavior during misc_unregister, and other rules

My understanding is that the file is available to be opened if and only if
the miscdevice is registered.  Are there any other exceptions or scenarios
I might be unaware of?

> ensure that ec_dev is valid during the full lifecycle of this driver.

To clarify, ec_dev is only required to be valid during the .open() call
itself, not for the entire lifecycle of the driver.  Since ec_dev can
become invalid at any other time, the driver uses ec_dev_rev to ensure
safe access.

> So, I think this cross-driver design an abusive use of the revocable
> idea.
> 
> It should not be allocated by the parent driver, it should be fully
> contained to this driver alone and used only to synchronize the
> fops. This would make it clear that the ec_dev pointer must be valid
  ^^^^
ec_dev_rev serves this purpose, not revocable_provider.

> during the *entire* lifecycle of this driver.
> 
> What you have here by putting the providing in another driver is too
> magic and obfuscates what the actual lifetime rules are while
> providing a giant foot gun for someone to think that just because it
> is marked revocable it is fully safe to touch revocable_provider at
> any time.
> 
> Broadly I think embedding a revocable in the memory that it is trying
> to protect is probably an anti-pattern as you must somehow already
> have a valid pointer to thing to get the revocable in the first place.
> This severely muddies the whole notion of when it can actually be
> revoked nor not.

ec_dev->revocable_provider should only be accessed directly within the
.open(), as ec_dev is guaranteed to be valid there.  For all other cases,
it uses ec_dev_rev and checks the validity with revocable_try_access()
to determine if ec_dev has been revoked.