On Fri, Nov 18, 2022 at 10:59:42AM -0500, Alan Stern wrote:
> 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).
I don't think it's at all simple to fix this - I posted a series
addressing the lifetime issues here a few years ago but didn't chase it
up and there was no feedback:
https://lore.kernel.org/linux-usb/20191028114228.3679219-1-john@xxxxxxxxxxxx/
That includes a patch to remove the embedded struct cdev and manage its
lifetime separately, which I think is needed as there are two different
struct device objects here and we cannot tie their lifetimes together.
