Re: [PATCH 20/34] iio: inkern: only relase the device node when done with it

[Jonathan Cameron <jic23@kernel.org>]

+Cc Mark Brown for a query on ordering in device tree based SPI setup.

On Fri, 10 Jun 2022 22:08:41 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Fri, 2022-06-10 at 16:56 +0200, Andy Shevchenko wrote:
> > On Fri, Jun 10, 2022 at 10:48 AM Nuno Sá <nuno.sa@analog.com> wrote:  
> > > 
> > > 'of_node_put()' can potentially release the memory pointed to by
> > > 'iiospec.np' which would leave us with an invalid pointer (and we
> > > would
> > > still pass it in 'of_xlate()'). As such, we can only release the
> > > node
> > > after we are done with it.  
> > 
> > The question you should answer in the commit message is the
> > following:
> > "Can an OF node, attached to a struct device, be gone before the
> > device itself?" If it so, then patch is good, otherwise there is no
> > point in this patch in the first place.
> >   
> 
> Yeah, I might be wrong but from a quick look... yes, I think the node
> can be gone before the device. Take a look on the spi or i2c of_notify
> handling and you can see that the nodes are get/put on the add/remove
> notifcation. Meaning that the node lifespan is not really attached to
> the device lifespan. If it was, I would expect to see of_node_put() on
> the device release() function...

I had a look at spi_of_notify() and indeed via spi_unregister_device()
the node is put just before device_del() so I agree that at first glance
it seems like there may be a race there against the useage here.
Mark (+CC) out of interest why are the node gets before the device_add()
in spi_add_device() called from of_register_spi_device() but the matching
node puts before the device_del() in spi_unregister_device()?
Seems like inconsistent ordering...

Which is not to say we shouldn't fix the IIO usage as this patch does!

> 
> Again, I might be wrong and I admit I was not sure about including this
> patch because it's a very unlikely scenario even though I think, in
> theory, a possible one.

The patch is currently valid even if it's not a 'real' bug.
Given we are doing a put on that device_node, it makes sense for that
to occur after the local use has finished - we shouldn't be relying on
what happens to be the case for lifetimes today.

Now, I did wonder if any drivers actually use it in their xlate callbacks.
One does for an error print, so this is potentially real (if very unlikely!)

This isn't a 'fix' I'd expect to rush in, or necessarily backport to stable
but I think it's a valid fix.

Perhaps add a little more detail to the patch description for v2.

Thanks,

Jonathan


> 
> - Nuno Sá
>