On Thu, Jan 14, 2021 at 03:19:52PM +0200, Heikki Krogerus wrote:

On Wed, Jan 13, 2021 at 05:30:03PM +0200, Andy Shevchenko wrote:

quoted

On Wed, Jan 13, 2021 at 01:39:18PM +0200, Heikki Krogerus wrote:

quoted

On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote:

quoted

On 11/01/2021 14:10, Heikki Krogerus wrote:

...

quoted

quoted

quoted

+/**
+ * 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.

quoted

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).

I studied the code again, and it actually looks like this is only a
problem when device_add_properties() is used and there is an
expectation that the node/properties are removed automatically in
device_del().

When this new API is used, the only place that needs to access the
swnode using the secondary pointer is software_node_notify(), so if we
simply handle that separately here, we should be able to clear the
secondary pointer after all. It would look something like this:

        void device_remove_software_node(struct device *dev)
        {
        	struct swnode *swnode;
        
        	swnode = dev_to_swnode(dev);
        	if (!swnode)
        		return;
        
                software_node_notify(dev, KOBJ_REMOVE);
                set_secondary_fwnode(dev, NULL);
        	kobject_put(&swnode->kobj);
        }

I'll test that, and if it works, and you guys don't see any problems
with it, I'll use it in v3.


Br,

-- 
heikki