Re: [PATCH v11 4/5] platform/chrome: Protect cros_ec_device lifecycle with revocable

[Jason Gunthorpe <jgg@nvidia.com>]

On Sat, May 16, 2026 at 10:42:09PM +0800, Tzung-Bi Shih wrote:
> On Thu, May 14, 2026 at 01:00:43PM -0300, Jason Gunthorpe wrote:
> > On Thu, May 14, 2026 at 03:34:12AM +0000, Tzung-Bi Shih wrote:
> > 
> > > To help me understand, could you elaborate on why the revocable mechanism
> > > isn't suitable here?
> > 
> > Stay within one driver. Create the revokable is probe, consume it
> > within that drivers fops/etc, destroy it on remove. Do not randomly
> > pass it to other drivers.
> 
> In that sense, after applying [1], does the patch make sense to you?

It is better, but you can see revokable is creating contortions that
don't make sense:

> @@ -223,11 +223,9 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
>  	ret = blocking_notifier_chain_register(&pdata->subscribers,
>  					       &priv->notifier);
>  	if (ret) {
> -		scoped_guard(rwsem_read, &pdata->ec_dev_sem) {
> -			if (pdata->ec_dev)
> -				dev_err(pdata->ec_dev->dev,
> -					"failed to register event notifier\n");
> -		}
> +		revocable_try_access_or_skip_scoped(&pdata->ec_rev, ec_dev)
> +			dev_err(ec_dev->dev,
> +				"failed to register event notifier\n");

It is impossible for ec_dev to be null here, the misc_unregister does
fence open.

> @@ -482,11 +483,12 @@ static int cros_ec_chardev_probe(struct platform_device *pdev)
>  static void cros_ec_chardev_remove(struct platform_device *pdev)
>  {
>  	struct chardev_pdata *pdata = platform_get_drvdata(pdev);
> +	struct cros_ec_device *ec_dev;
>  
> -	blocking_notifier_chain_unregister(&pdata->ec_dev->event_notifier,
> -					   &pdata->relay);
> -	scoped_guard(rwsem_write, &pdata->ec_dev_sem)
> -		pdata->ec_dev = NULL;
> +	revocable_try_access_or_skip_scoped(&pdata->ec_rev, ec_dev)
> +		blocking_notifier_chain_unregister(&ec_dev->event_notifier,
> +						   &pdata->relay);
> +	revocable_revoke(&pdata->ec_rev);

And this is complete garbage nonsense, we are in a driver bound
context about to revoke the revokable, it is not optional, it can't
fail, if it doesn't we can't skip the unregister or it will eventually
crash.

I said it before, but to re-iterate - what this scheme fails to
capture from rust is the most important detail - the driver bound
checking that confirms the content is valid without any need for
locking or possibility of failure.

Open, remove, are both bound contexts that can never fail to obtain
their protected content and don't need srcu locking.

I don't konw what Danilo thinks, but as the rust side has evolved I
think it was a mistake to combine the revocable and SRCU
together. Having two primitives would make more sense

The first is "this value is only valid under driver bound, present
your thing proving driver bound and you can get the value". This would
be fully 0 cost.

The second is "My callchain doesn't have a way to get driver bound,
so this widget will try to open a SRCU critical section that produces
it".

Each driver could have many of the first but needs only one of the
second. The second is the "code smell" that says something is not
great by not properly managing driver bound. Since a driver needs only
one of the widgets it would solve the repeated srcu problem on unbind.

From that lens you can see how troubled this C version is, it promotes
the "code smell" SRCU API into the only API and makes it first class
promoting its use and ignores/obfuscates/worsens the actual API we
want people to use: prove you have a driver bound. :(

Jason