| Message ID | 20210111141045.14027-2-heikki.krogerus@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove one more platform_device_add_properties() call | expand |
Hi Heikki On 11/01/2021 14:10, Heikki Krogerus wrote: > This helper will register a software node and then assign > it to device at the same time. The function will also make > sure that the device can't have more than one software node. > > Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- I like this change. One comment below, but for what it's worth: Reviewed-by: Daniel Scally <djrscally@gmail.com> > +/** > + * 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)? > + > int software_node_notify(struct device *dev, unsigned long action) > { > - struct fwnode_handle *fwnode = dev_fwnode(dev); > struct swnode *swnode; > int ret; > > - if (!fwnode) > - return 0; > - > - if (!is_software_node(fwnode)) > - fwnode = fwnode->secondary; > - if (!is_software_node(fwnode)) > + swnode = dev_to_swnode(dev); > + if (!swnode) > return 0; > > - swnode = to_swnode(fwnode); > - > switch (action) { > case KOBJ_ADD: > ret = sysfs_create_link(&dev->kobj, &swnode->kobj, > diff --git a/include/linux/property.h b/include/linux/property.h > index 0a9001fe7aeab..b0e413dc59271 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -488,4 +488,7 @@ fwnode_create_software_node(const struct property_entry *properties, > const struct fwnode_handle *parent); > void fwnode_remove_software_node(struct fwnode_handle *fwnode); > > +int device_add_software_node(struct device *dev, const struct software_node *swnode); > +void device_remove_software_node(struct device *dev); > + > #endif /* _LINUX_PROPERTY_H_ */ >
Hi Daniel, On Wed, Jan 13, 2021 at 12:40:03AM +0000, Daniel Scally wrote: > Hi Heikki > > On 11/01/2021 14:10, Heikki Krogerus wrote: > > This helper will register a software node and then assign > > it to device at the same time. The function will also make > > sure that the device can't have more than one software node. > > > > Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > --- > > I like this change. One comment below, but for what it's worth: > > Reviewed-by: Daniel Scally <djrscally@gmail.com> Thanks! > > +/** > > + * 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)? 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.
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.
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)? Looking more into this code I think we need to call set_secondary_fwnode(dev, NULL); among these lines. The real problem is that set_primary_fwnode() and set_secondary_fwnode() have no reference counting. If we have a chain ->primary->secondary->-ENODEV is being used somewhere we can't tell from here. So, in practice it means that we lack of some kind of primary node to increment reference count of the secondary node when the latter is chained to the given primary. But it makes things too complicated. Any other options for shared primary-secondary chain? Standalone primary along with standalone (exclusive) secondary doesn't need this AFAICS. Perhaps a flag to primary like shared / exclusive that will prevent breaking the chain in set_secondary_fwnode()?
On Wed, Jan 13, 2021 at 05:55:04PM +0200, Andy Shevchenko 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)? > > Looking more into this code I think we need to call > > set_secondary_fwnode(dev, NULL); > > among these lines. > > The real problem is that set_primary_fwnode() and set_secondary_fwnode() have > no reference counting. If we have a chain ->primary->secondary->-ENODEV is > being used somewhere we can't tell from here. > > So, in practice it means that we lack of some kind of primary node to increment > reference count of the secondary node when the latter is chained to the given > primary. But it makes things too complicated. Any other options for shared > primary-secondary chain? Standalone primary along with standalone (exclusive) > secondary doesn't need this AFAICS. Perhaps a flag to primary like shared / > exclusive that will prevent breaking the chain in set_secondary_fwnode()? Or maybe I imagined only theoretical cases and we have no such issue? -- With Best Regards, Andy Shevchenko
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
On Wed, Jan 13, 2021 at 05:58:12PM +0200, Andy Shevchenko wrote: > On Wed, Jan 13, 2021 at 05:55:04PM +0200, Andy Shevchenko 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)? > > > > Looking more into this code I think we need to call > > > > set_secondary_fwnode(dev, NULL); > > > > among these lines. > > > > The real problem is that set_primary_fwnode() and set_secondary_fwnode() have > > no reference counting. If we have a chain ->primary->secondary->-ENODEV is > > being used somewhere we can't tell from here. > > > > So, in practice it means that we lack of some kind of primary node to increment > > reference count of the secondary node when the latter is chained to the given > > primary. But it makes things too complicated. Any other options for shared > > primary-secondary chain? Standalone primary along with standalone (exclusive) > > secondary doesn't need this AFAICS. Perhaps a flag to primary like shared / > > exclusive that will prevent breaking the chain in set_secondary_fwnode()? > > Or maybe I imagined only theoretical cases and we have no such issue? I think we should really start looking into the possibility of removing the whole secondary coupling, because that is the thing that is crippling us. Br,
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: > > 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). 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
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 4a4b2008fbc26..60a8501a4976c 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -48,6 +48,19 @@ EXPORT_SYMBOL_GPL(is_software_node); struct swnode, fwnode) : NULL; \ }) +static inline struct swnode *dev_to_swnode(struct device *dev) +{ + struct fwnode_handle *fwnode = dev_fwnode(dev); + + if (!fwnode) + return NULL; + + if (!is_software_node(fwnode)) + fwnode = fwnode->secondary; + + return to_swnode(fwnode); +} + static struct swnode * software_node_to_swnode(const struct software_node *node) { @@ -843,22 +856,60 @@ void fwnode_remove_software_node(struct fwnode_handle *fwnode) } EXPORT_SYMBOL_GPL(fwnode_remove_software_node); +/** + * device_add_software_node - Assign software node to a device + * @dev: The device the software node is meant for. + * @swnode: The software node. + * + * This function will register @swnode and make it the secondary firmware node + * pointer of @dev. If @dev has no primary node, then @swnode will become the primary + * node. + */ +int device_add_software_node(struct device *dev, const struct software_node *swnode) +{ + int ret; + + /* Only one software node per device. */ + if (dev_to_swnode(dev)) + return -EBUSY; + + ret = software_node_register(swnode); + if (ret) + return ret; + + set_secondary_fwnode(dev, software_node_fwnode(swnode)); + + return 0; +} +EXPORT_SYMBOL_GPL(device_add_software_node); + +/** + * 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); + int software_node_notify(struct device *dev, unsigned long action) { - struct fwnode_handle *fwnode = dev_fwnode(dev); struct swnode *swnode; int ret; - if (!fwnode) - return 0; - - if (!is_software_node(fwnode)) - fwnode = fwnode->secondary; - if (!is_software_node(fwnode)) + swnode = dev_to_swnode(dev); + if (!swnode) return 0; - swnode = to_swnode(fwnode); - switch (action) { case KOBJ_ADD: ret = sysfs_create_link(&dev->kobj, &swnode->kobj, diff --git a/include/linux/property.h b/include/linux/property.h index 0a9001fe7aeab..b0e413dc59271 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -488,4 +488,7 @@ fwnode_create_software_node(const struct property_entry *properties, const struct fwnode_handle *parent); void fwnode_remove_software_node(struct fwnode_handle *fwnode); +int device_add_software_node(struct device *dev, const struct software_node *swnode); +void device_remove_software_node(struct device *dev); + #endif /* _LINUX_PROPERTY_H_ */