+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@xxxxxxxxx> 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@xxxxxxxxxx> 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á >
