Re: [PATCH] software node: only let the owner release a managed node

[Bartosz Golaszewski]

On Mon, Nov 09, 2026 at 03:10:31PM +0100, Danilo Krummrich wrote:
> On Mon Nov 9, 2026 at 2:37 PM CET, Johan Hovold wrote:
> > On Mon, Nov 02, 2026 at 04:25:16PM +0100, Bartosz Golaszewski wrote:
> >> device_create_managed_software_node() creates a node whose lifetime is
> >> tied to the device it was created for. software_node_notify_remove() is
> >> called for every device that has the node as its fwnode (primary OR
> >> secondary) and, if the node is managed, unconditionally detaches it from
> >> that device and drops the creation reference.
> >> 
> >> This breaks when the node is shared with another device. Example: the
> >> i2c-designware copies the parent's fwnode to the adapter's struct device.
> >> When the adapter is deleted, it releases the reference that belongs to
> >> the owner and the latter ends up with a dangling pointer.
> >> 
> >> Record the device's "manager" and only perform the actual release when
> >> notify is called for that "owner" device.
> >> 
> >> Reported-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@xxxxxxxxx>
> >> Closes: https://lore.kernel.org/all/20261009164241.923756-2-balasubramani.vivekanandan@xxxxxxxxx/
> >> Fixes: 151f6ff78cdf ("software node: Provide replacement for device_add_properties()")
> 
> Hm...the documentation of device_create_managed_software_node() does point out
> the limitation that it shouldn't be shared with other devices.
> 
> Now, that's rather fragile and the reason I pointed out [1] it should be
> changed, but it's technically not a fix?
> 
> [1] https://lore.kernel.org/all/DM1FMM3KEYU9.1D26IINII93FO@xxxxxxxxxx/
>

I don't mind dropping the tag.

> >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxxxxxxxx>
> >> ---
> >> Hi!
> >> 
> >> This is my approach at addressing the bug reported by Vivekanandan
> >> directly in driver core. Please test it if you can.
> >
> > This looks good to me, but I think the patch should be simplified somewhat.
> >
> >> @@ -1190,6 +1191,7 @@ void software_node_notify(struct device *dev)
> >>  void software_node_notify_remove(struct device *dev)
> >>  {
> >>  	struct swnode *swnode;
> >> +	bool is_owner;
> >>  
> >>  	swnode = dev_to_swnode(dev);
> >>  	if (!swnode)
> >> @@ -1206,9 +1208,13 @@ void software_node_notify_remove(struct device *dev)
> >>  	if (!device_match_fwnode(dev, &swnode->fwnode) && swnode->fwnode.dev == dev)
> >>  		swnode->fwnode.dev = NULL;
> >>  
> >> +	is_owner = swnode->owner && swnode->owner == dev;
> >
> > The NULL check here is redundant.
> >
> >> +	if (is_owner)
> >> +		swnode->owner = NULL;
> >
> > There is no need to clear the owner field as devices are not allowed to be
> > re-registered.
> 
> I guess that's true, but maybe leaving in a defined state is a bit more robust?
> 
> >> +
> >>  	swnode_put(swnode);
> >>  
> >> -	if (swnode->managed) {
> >> +	if (is_owner) {
> >>  		set_secondary_fwnode(dev, NULL);
> >>  		swnode_put(swnode);
> >>  	}
> >
> > And if we drop the managed reference before the notify one, we can drop
> > the temporary as well and this chunk just becomes:
> >
> > @@ -1206,12 +1207,12 @@ void software_node_notify_remove(struct device *dev)
> >         if (!device_match_fwnode(dev, &swnode->fwnode) && swnode->fwnode.dev == dev)
> >                 swnode->fwnode.dev = NULL;
> >  
> > -       swnode_put(swnode);
> > -
> > -       if (swnode->managed) {
> > +       if (swnode->owner == dev) {
> >                 set_secondary_fwnode(dev, NULL);
> >                 swnode_put(swnode);
> >         }
> > +
> > +       swnode_put(swnode);
> >  }
> >
> > The end-result applied for 7.4 (to fix the 7.3 regression) is here:
> 
> A previous reply from Balasubramani suggested to adjust the doc-comment of
> device_create_managed_software_node(), so I thought Bartosz will send a new
> version.
> 

I will, I was just giving it some time on the list.

> Also, are we sure this is -rc material? I think the original commit wasn't
> technically broken and the bug in Xe was fixed otherwise.

We can queue it for v7.5 alright.

Bart