Re: [PATCH v3 5/8] platform/chrome: Introduce cros_ec_device_alloc()

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

On Fri, Jul 25, 2025 at 06:58:23AM +0200, Greg KH wrote:
> On Thu, Jul 24, 2025 at 01:32:01PM +0000, Tzung-Bi Shih wrote:
> > On Thu, Jul 24, 2025 at 12:36:18PM +0200, Greg KH wrote:
> > > NEVER attempt to increment/decrement refcounts on open/release.  That
> > > way lies madness and should not be needed at all, the underlying
> > > infrastructure should keep things working properly here, right?
> > [...]
> You are attempting to have one reference count for one object that has
> different lifecycles.  That doesn't work well, if at all.  See the
> numerous talks at the Linux Plumbers conference over the past few years
> about why this is a "bad idea" and for ideas on how to redesign it to
> fix the issue.
> 
> Hint, 2 objects, different reference counts :)

Referenced the talk [1] and its related works and hope I understand it
correctly.

[1] https://lpc.events/event/17/contributions/1627/

Summarize the proposed fix and I hope it makes sense:

Introduce a new helper: ref_proxy for managing "weak" references to the
object that may be gone at any time.  `ref` is the target pointer (i.e.
the resource) which could be released asynchronously (from the resource
consumers' point of view).  The struct ref_proxy_struct is for resource
providers and is only freed on dropping the final reference.

  +struct ref_proxy_struct {
  +       struct srcu_struct srcu;
  +       void __rcu *ref;
  +       struct kref kref;
  +};
  +
  +struct ref_proxy_struct *ref_proxy_struct_alloc(void *ref)
  +{
  +       struct ref_proxy_struct *rp;
  +
  +       rp = kzalloc(sizeof(*rp), GFP_KERNEL);
  +       if (!rp)
  +               return NULL;
  +
  +       init_srcu_struct(&rp->srcu);
  +       rcu_assign_pointer(rp->ref, ref);
  +       synchronize_srcu(&rp->srcu);
  +       kref_init(&rp->kref);
  +
  +       return rp;
  +}
  +EXPORT_SYMBOL(ref_proxy_struct_alloc);
  +
  +static void ref_proxy_struct_release(struct kref *kref)
  +{
  +       struct ref_proxy_struct *rp = container_of(kref, struct ref_proxy_struct, kref);
  +
  +       cleanup_srcu_struct(&rp->srcu);
  +       kfree(rp);
  +}
  +
  +void ref_proxy_struct_free(struct ref_proxy_struct *rp)
  +{
  +       rcu_assign_pointer(rp->ref, NULL);
  +       synchronize_srcu(&rp->srcu);
  +       kref_put(&rp->kref, ref_proxy_struct_release);
  +}
  +EXPORT_SYMBOL(ref_proxy_struct_free);

The struct ref_proxy is for resource consumers.  It holds a reference to
the ref_proxy_struct during its lifetime.

  +struct ref_proxy {
  +       struct ref_proxy_struct *rp;
  +       int idx;
  +};
  +
  +struct ref_proxy *ref_proxy_alloc(struct ref_proxy_struct *rp)
  +{
  +       struct ref_proxy *proxy;
  +
  +       proxy = kzalloc(sizeof(*proxy), GFP_KERNEL);
  +       if (!proxy)
  +               return NULL;
  +
  +       proxy->rp = rp;
  +       kref_get(&rp->kref);
  +
  +       return proxy;
  +}
  +EXPORT_SYMBOL(ref_proxy_alloc);
  +
  +void ref_proxy_free(struct ref_proxy *proxy)
  +{
  +       struct ref_proxy_struct *rp = proxy->rp;
  +
  +       kref_put(&rp->kref, ref_proxy_struct_release);
  +       kfree(proxy);
  +}
  +EXPORT_SYMBOL(ref_proxy_free);

When a resource consumer needs to reference the resource, it gets the
pointer via ref_proxy_get() which entering a RCU critical section and
puts the pointer via ref_proxy_put() which leaving the critical section
afterward.

  +void *ref_proxy_get(struct ref_proxy *proxy)
  +{
  +       struct ref_proxy_struct *rp = proxy->rp;
  +       void *ref;
  +
  +       proxy->idx = srcu_read_lock(&rp->srcu);
  +
  +       ref = rcu_dereference(rp->ref);
  +       if (!ref)
  +               srcu_read_unlock(&rp->srcu, proxy->idx);
  +
  +       return ref;
  +}
  +EXPORT_SYMBOL(ref_proxy_get);
  +
  +void ref_proxy_put(struct ref_proxy *proxy)
  +{
  +       struct ref_proxy_struct *rp = proxy->rp;
  +
  +       srcu_read_unlock(&rp->srcu, proxy->idx);
  +}
  +EXPORT_SYMBOL(ref_proxy_put);

The resource provider (e.g. cros_ec_spi) should provide a ref_proxy on
probing and free the ref_proxy on removing.

  @@ -770,0 +772,4 @@ static int cros_ec_spi_probe(struct spi_device *spi)
  +       ec_dev->ref_proxy = ref_proxy_struct_alloc(ec_dev);
  +       if (!ec_dev->ref_proxy)
  +               return -ENOMEM;
  +

   static void cros_ec_spi_remove(struct spi_device *spi)
   {
          struct cros_ec_device *ec_dev = spi_get_drvdata(spi);
  
  +       ref_proxy_struct_free(ec_dev->ref_proxy);
          cros_ec_unregister(ec_dev);
   }

The resource consumer (e.g. cros_ec_chardev) should create an instance of
ref_proxy and tie to its local lifecycle.  It should get the pointer before
accessing.  It indicates the resource is no longer available if the
returning pointer is NULL.

  @@ -166,7 +187,10 @@ static int cros_ec_chardev_open(struct inode *inode, struct file *filp)
          if (!priv)
                  return -ENOMEM;
  
  -       priv->ec_dev = ec_dev;
  +       priv->ec_dev_proxy = ref_proxy_alloc(ec_dev->ref_proxy);
  +       if (!priv->ec_dev_proxy)
  +               return -ENOMEM;
  +
          priv->cmd_offset = ec->cmd_offset;
          filp->private_data = priv;
          INIT_LIST_HEAD(&priv->events);

  @@ -251,11 +276,16 @@ static ssize_t cros_ec_chardev_read(struct file *filp, char __user *buffer,
   static int cros_ec_chardev_release(struct inode *inode, struct file *filp)
   {
          struct chardev_priv *priv = filp->private_data;
  -       struct cros_ec_device *ec_dev = priv->ec_dev;
  +       struct cros_ec_device *ec_dev;
          struct ec_event *event, *e;
  
  -       blocking_notifier_chain_unregister(&ec_dev->event_notifier,
  -                                          &priv->notifier);
  +       ec_dev = ref_proxy_get(priv->ec_dev_proxy);
  +       if (ec_dev) {
  +               blocking_notifier_chain_unregister(&ec_dev->event_notifier,
  +                                                  &priv->notifier);
  +               ref_proxy_put(priv->ec_dev_proxy);
  +       }
  +       ref_proxy_free(priv->ec_dev_proxy);

  @@ -64,7 +66,11 @@ static int ec_get_version(struct chardev_priv *priv, char *str, int maxlen)
          msg->command = EC_CMD_GET_VERSION + priv->cmd_offset;
          msg->insize = sizeof(*resp);

  -       ret = cros_ec_cmd_xfer_status(priv->ec_dev, msg);
  +       ec_dev = ref_proxy_get(priv->ec_dev_proxy);
  +       if (!ec_dev)
  +               return -ENODEV;
  +       ret = cros_ec_cmd_xfer_status(ec_dev, msg);
  +       ref_proxy_put(priv->ec_dev_proxy);
          if (ret < 0) {
                  snprintf(str, maxlen,
                           "Unknown EC version, returned error: %d\n",