On Thu, 17 Nov 2022, Alan Stern wrote:
> On Thu, Nov 17, 2022 at 01:46:26PM +0000, Lee Jones wrote:
> > On Thu, 17 Nov 2022, Greg KH wrote:
> >
> > > On Thu, Nov 17, 2022 at 12:08:13PM +0000, Lee Jones wrote:
> > > > +static inline bool f_hidg_is_open(struct f_hidg *hidg)
> > > > +{
> > > > + return !!kref_read(&hidg->cdev.kobj.kref);
> > > > +}
> > >
> > > Ick, sorry, no, that's not going to work and is not allowed at all.
> > > That's some major layering violations there, AND it can change after you
> > > get the value as well.
> >
> > This cdev belongs solely to this driver. Hence the *.*.* and not
> > *->*->*. What is preventing us from reading our own data? If we
> > cannot do this directly, can I create an API to do it 'officially'?
> >
> > I do, however, appreciate that a little locking wouldn't go amiss.
> >
> > If this solution is not acceptable either, then we're left up the
> > creak without a paddle. The rules you've communicated are not
> > compatible with each other.
> >
> > Rule 1: Only one item in a data structure can reference count.
> >
> > Due to the embedded cdev struct, this rules out my first solution of
> > giving f_hidg its own kref so that it can conduct its own life-time
> > management.
> >
> > A potential option to satisfy this rule would be to remove the cdev
> > attribute and create its data dynamically instead. However, the
> > staticness of cdev is used to obtain f_hidg (with container_of()) in
> > the character device handling component, so it cannot be removed.
>
> You have not understood this rule correctly. Only one item in a data
> structure can hold a reference count _for that structure_. But several
> items in a structure can hold reference counts for themselves.
Here was the review comment I was working to on this patch [0]:
"While at first glance, it seems that f_hidg is not reference
counted, it really is, with the embedded "struct cdev" a few lines
above this.
That is the reference count that should control the lifecycle of
this object, not another reference here in the "outer layer"
structure."
> So for example, you could put a kref in f_hidg which would hold the
> reference count for the f_hidg structure, while at the same time
> including an embedded cdev with its own reference counter. The point is
> that the refcount in the embedded cdev refers to the lifetime of the
> cdev, not the lifetime of the f_hidg.
This was the approach in the original submission [1], which during
review I was told was unacceptable for the aforementioned reason.
[0] https://lore.kernel.org/all/Y1PnoMvDmZMqXScw@xxxxxxxxx/
[1] https://lore.kernel.org/all/20221017112737.230772-1-lee@xxxxxxxxxx/
> To make this work properly, you have to do two additional things:
>
> When the cdev's refcount is initialized, increment the kref
> in f_hidg.
>
> When the cdev's refcount drops to 0, decrement the kref (and
> release f_hidg if the kref hits 0).
More than happy to revisit the first solution with Greg's blessing.
--
Lee Jones [李琼斯]
