On Wed, Nov 26, 2025 at 04:16:29AM +0000, Tzung-Bi Shih wrote:
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.
Yes, through misc_mtx.
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.
open can be called during the entire lifecycle of the driver, misc_deregister() is called during remove. So this is a meaningless distinction.
ec_dev cannot become invalid while the driver is bound.
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.
How does this detail matter? It is still created by the wrong 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.
I understand what this does and why it works, I am saying it is an anti-pattern bad design to cross a revocable between two drivers like this.
You want the driver creating the fops to revoke a pointer from its own fops - not span across multiple drivers to achieve the same thing. It significantly confuses what the actual lifecycle rules are.
Jason