Re: [PATCH v2 1/3] software node: Introduce device_add_software_node()

[Heikki Krogerus]

On Wed, Jan 13, 2021 at 05:30:03PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 13, 2021 at 01:39:18PM +0200, Heikki Krogerus wrote:
> > On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote:
> > > On 11/01/2021 14:10, Heikki Krogerus wrote:
> 
> ...
> 
> > > > +/**
> > > > + * device_remove_software_node - Remove device's software node
> > > > + * @dev: The device with the software node.
> > > > + *
> > > > + * This function will unregister the software node of @dev.
> > > > + */
> > > > +void device_remove_software_node(struct device *dev)
> > > > +{
> > > > +	struct swnode *swnode;
> > > > +
> > > > +	swnode = dev_to_swnode(dev);
> > > > +	if (!swnode)
> > > > +		return;
> > > > +
> > > > +	kobject_put(&swnode->kobj);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(device_remove_software_node);
> > > 
> > > I wonder if this also ought to set dev_fwnode(dev)->secondary back to
> > > ERR_PTR(-ENODEV)?
> 
> Actually it's a good question.
> 
> > We can't do that here unfortunately. Other places still have a
> > reference to the swnode at this point and they may still need to
> > access it using the dev_fwnode(dev)->secondary pointer.
> 
> Yeah, but in this case we potentially leave a dangling pointer when last of the
> user gone and kobject_put() will call for release.

The caller has to be responsible of setting the secondary back to
ERR_PTR(-ENODEV). We can not do anything here like I explained. We can
not even do that in software_node_notify() when the association to the
struct device is removed, because the fwnode->secondary is still
accessed after that. The caller needs to remove both the node and the
device, and only after that it is safe to set the secondary back to
ERR_PTR(-ENODEV).

This clearly needs to explained in the comment... I'll fix it.


thanks,

-- 
heikki