On Fri, Nov 18, 2022 at 08:54:53AM +0000, Lee Jones wrote:
> 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."
It's worth noting that the review comment goes on to say:
"But, the cdev api is tricky and messy and not really set up to control
the lifecycle of objects it is embedded in."
This is a good indication that a separate reference counter really is
needed (in fact it almost contradicts what was written above).
> > 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.
Okay, let's see what Greg thinks after he reads this discussion.
Alan Stern
