| Message ID | 20210202125032.64982-2-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: Handle device properties with software node API | expand |
On Tue, Feb 2, 2021 at 1:50 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > Adding function device_create_managed_software_node() that > is designed to work as a drop-in replacement for > device_add_properties(). The function has one additional > feature compared to device_add_properties(). It takes also > an optional parent node as parameter, and that way allow the > nodes created with it to be part of a node hierarchy. > > The lifetime of the software nodes created with this > function will be tied to the device they are assigned to. > The function will therefore behave exactly the same way as > device_add_properties() is expected to behave, except that > it does not simply assume that the nodes attached to the > device are always destroyed in device_del() unconditionally. > > The nodes created with this function are guaranteed to be > removed when the device is removed even after device_del() > stops calling device_remove_properties() unconditionally. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/base/swnode.c | 43 ++++++++++++++++++++++++++++++++++++++++ > include/linux/property.h | 4 ++++ > 2 files changed, 47 insertions(+) > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > index 20faa9f4f5ed2..37179a8b1ceba 100644 > --- a/drivers/base/swnode.c > +++ b/drivers/base/swnode.c > @@ -24,6 +24,7 @@ struct swnode { > struct swnode *parent; > > unsigned int allocated:1; > + unsigned int managed:1; > }; > > static DEFINE_IDA(swnode_root_ids); > @@ -1045,6 +1046,43 @@ void device_remove_software_node(struct device *dev) > } > EXPORT_SYMBOL_GPL(device_remove_software_node); > > +/** > + * device_create_managed_software_node - Create a software node for a device > + * @dev: The device the software node is assigned to. > + * @properties: Device properties for the software node. > + * @parent: Parent of the software node. > + * > + * Creates a software node as a managed resource for @dev, which means the > + * lifetime of the newly created software node is tied to the lifetime of @dev. > + * Software nodes created with this function should not be reused or shared > + * because of that. The function takes a deep copy of @properties for the > + * software node. > + * > + * Since the new software node is assigned directly to @dev, and since it should > + * not be shared, it is not returned to the caller. The function returns 0 on > + * success, and errno in case of an error. > + */ > +int device_create_managed_software_node(struct device *dev, > + const struct property_entry *properties, > + const struct software_node *parent) > +{ > + struct fwnode_handle *p = software_node_fwnode(parent); > + struct fwnode_handle *fwnode; > + > + if (parent && !p) > + return -EINVAL; > + > + fwnode = fwnode_create_software_node(properties, p); > + if (IS_ERR(fwnode)) > + return PTR_ERR(fwnode); > + > + to_swnode(fwnode)->managed = true; > + set_secondary_fwnode(dev, fwnode); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(device_create_managed_software_node); > + > int software_node_notify(struct device *dev, unsigned long action) > { > struct swnode *swnode; > @@ -1073,6 +1111,11 @@ int software_node_notify(struct device *dev, unsigned long action) > sysfs_remove_link(&swnode->kobj, dev_name(dev)); > sysfs_remove_link(&dev->kobj, "software_node"); > kobject_put(&swnode->kobj); > + > + if (swnode->managed) { > + set_secondary_fwnode(dev, NULL); > + kobject_put(&swnode->kobj); Where does the corresponding kobject_get() get called? > + } > break; > default: > break; > diff --git a/include/linux/property.h b/include/linux/property.h > index b0e413dc59271..dafccfce02624 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -491,4 +491,8 @@ 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); > > +int device_create_managed_software_node(struct device *dev, > + const struct property_entry *properties, > + const struct software_node *parent); > + > #endif /* _LINUX_PROPERTY_H_ */ > -- > 2.30.0 >
Hi Rafael, On Tue, Feb 02, 2021 at 03:44:05PM +0100, Rafael J. Wysocki wrote: > > +/** > > + * device_create_managed_software_node - Create a software node for a device > > + * @dev: The device the software node is assigned to. > > + * @properties: Device properties for the software node. > > + * @parent: Parent of the software node. > > + * > > + * Creates a software node as a managed resource for @dev, which means the > > + * lifetime of the newly created software node is tied to the lifetime of @dev. > > + * Software nodes created with this function should not be reused or shared > > + * because of that. The function takes a deep copy of @properties for the > > + * software node. > > + * > > + * Since the new software node is assigned directly to @dev, and since it should > > + * not be shared, it is not returned to the caller. The function returns 0 on > > + * success, and errno in case of an error. > > + */ > > +int device_create_managed_software_node(struct device *dev, > > + const struct property_entry *properties, > > + const struct software_node *parent) > > +{ > > + struct fwnode_handle *p = software_node_fwnode(parent); > > + struct fwnode_handle *fwnode; > > + > > + if (parent && !p) > > + return -EINVAL; > > + > > + fwnode = fwnode_create_software_node(properties, p); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ To answer your question below: here. > > + if (IS_ERR(fwnode)) > > + return PTR_ERR(fwnode); > > + > > + to_swnode(fwnode)->managed = true; > > + set_secondary_fwnode(dev, fwnode); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(device_create_managed_software_node); > > + > > int software_node_notify(struct device *dev, unsigned long action) > > { > > struct swnode *swnode; > > @@ -1073,6 +1111,11 @@ int software_node_notify(struct device *dev, unsigned long action) > > sysfs_remove_link(&swnode->kobj, dev_name(dev)); > > sysfs_remove_link(&dev->kobj, "software_node"); > > kobject_put(&swnode->kobj); > > + > > + if (swnode->managed) { > > + set_secondary_fwnode(dev, NULL); > > + kobject_put(&swnode->kobj); > > Where does the corresponding kobject_get() get called? So in function fwnode_create_software_node() we use kobject_init_and_add(). thanks,
On Tue, Feb 2, 2021 at 4:01 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > Hi Rafael, > > On Tue, Feb 02, 2021 at 03:44:05PM +0100, Rafael J. Wysocki wrote: > > > +/** > > > + * device_create_managed_software_node - Create a software node for a device > > > + * @dev: The device the software node is assigned to. > > > + * @properties: Device properties for the software node. > > > + * @parent: Parent of the software node. > > > + * > > > + * Creates a software node as a managed resource for @dev, which means the > > > + * lifetime of the newly created software node is tied to the lifetime of @dev. > > > + * Software nodes created with this function should not be reused or shared > > > + * because of that. The function takes a deep copy of @properties for the > > > + * software node. > > > + * > > > + * Since the new software node is assigned directly to @dev, and since it should > > > + * not be shared, it is not returned to the caller. The function returns 0 on > > > + * success, and errno in case of an error. > > > + */ > > > +int device_create_managed_software_node(struct device *dev, > > > + const struct property_entry *properties, > > > + const struct software_node *parent) > > > +{ > > > + struct fwnode_handle *p = software_node_fwnode(parent); > > > + struct fwnode_handle *fwnode; > > > + > > > + if (parent && !p) > > > + return -EINVAL; > > > + > > > + fwnode = fwnode_create_software_node(properties, p); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > To answer your question below: here. > > > > + if (IS_ERR(fwnode)) > > > + return PTR_ERR(fwnode); > > > + > > > + to_swnode(fwnode)->managed = true; > > > + set_secondary_fwnode(dev, fwnode); > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(device_create_managed_software_node); > > > + > > > int software_node_notify(struct device *dev, unsigned long action) > > > { > > > struct swnode *swnode; > > > @@ -1073,6 +1111,11 @@ int software_node_notify(struct device *dev, unsigned long action) > > > sysfs_remove_link(&swnode->kobj, dev_name(dev)); > > > sysfs_remove_link(&dev->kobj, "software_node"); > > > kobject_put(&swnode->kobj); > > > + > > > + if (swnode->managed) { > > > + set_secondary_fwnode(dev, NULL); > > > + kobject_put(&swnode->kobj); > > > > Where does the corresponding kobject_get() get called? > > So in function fwnode_create_software_node() we use > kobject_init_and_add(). OK It looks like there is a use case that cannot be addressed by using device_add_properties() and that's why you need this new function. Can you describe that use case, please, and explain what the problem with using device_add_properties() in it is?
On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote: > It looks like there is a use case that cannot be addressed by using > device_add_properties() and that's why you need this new function. > > Can you describe that use case, please, and explain what the problem > with using device_add_properties() in it is? The problem with device_add_properties() is that it gives false impression that the device properties are somehow directly attached to the devices, which is not true. Now, that should not be a major issue, but it seems that it is. I think Lee Jones basically used that as an argument to refuse changes (and pretty minor changes) that would have allowed us to use software nodes with the MFD drivers. Nevertheless, I was not planning to provide a replacement for it originally. We do in any case have the real issue caused by that device_remove_properties() call in device_del() which has to be fixed. I started to fix that by moving device_add_properties() under drivers/base/swnode.c so I can implement it like I've implemented now that new function, but after that when I started to tackle the second issue by removing the subsystem wrappers like platform_device_add_device_properties() and replacing them with things like platform_device_add_software_node() in order to give the drivers a chance to actually use software nodes, I realised that there isn't much use for device_add_properties() after that. Also, even though I'm not super happy about adding more API to this thing, this function - device_create_managed_software_node() - I do like. Not only is it implemented so that we don't have to rely on anything else in drivers core in order to achieve the lifetime link with the device, it is also much more descriptive. The function name alone does not leave any questions about what is going to happen if that function is called. You'll end up with a software node that just happens to be attached to the device. On top of those two things, by adding the new function it also gives me a nice stepping stone to do these changes in the normal stages: 1. Add feature/modification. 2. Convert users. 3. Cleanup. And finally, even though we may not have any users left for device_add_properties() after I have updated all the subsystems and drivers, this new function will still be useful. That is because, even though it can be used as a drop-in replacement for device_add_properties(), it does add that one small but important change. It allows the nodes created with it to be part of node hierarchy, and that alone is useful to me, and I'm planning on using that feature later. I'm sorry about the long explanation. Br,
On Wed, Feb 3, 2021 at 10:45 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote: > > It looks like there is a use case that cannot be addressed by using > > device_add_properties() and that's why you need this new function. > > > > Can you describe that use case, please, and explain what the problem > > with using device_add_properties() in it is? > > The problem with device_add_properties() is that it gives false > impression that the device properties are somehow directly attached to > the devices, which is not true. Now, that should not be a major issue, > but it seems that it is. I think Lee Jones basically used that as an > argument to refuse changes (and pretty minor changes) that would have > allowed us to use software nodes with the MFD drivers. > > Nevertheless, I was not planning to provide a replacement for it > originally. We do in any case have the real issue caused by that > device_remove_properties() call in device_del() which has to be fixed. What's that issue, specifically? > I started to fix that by moving device_add_properties() under > drivers/base/swnode.c so I can implement it like I've implemented now > that new function, but after that when I started to tackle the second > issue by removing the subsystem wrappers like > platform_device_add_device_properties() and replacing them with things > like platform_device_add_software_node() in order to give the drivers > a chance to actually use software nodes, I realised that there isn't > much use for device_add_properties() after that. > > Also, even though I'm not super happy about adding more API to this > thing, this function - device_create_managed_software_node() - I do > like. Not only is it implemented so that we don't have to rely on > anything else in drivers core in order to achieve the lifetime link > with the device, it is also much more descriptive. The function name > alone does not leave any questions about what is going to happen if > that function is called. You'll end up with a software node that just > happens to be attached to the device. > > On top of those two things, by adding the new function it also gives > me a nice stepping stone to do these changes in the normal stages: > > 1. Add feature/modification. > 2. Convert users. > 3. Cleanup. > > And finally, even though we may not have any users left for > device_add_properties() after I have updated all the subsystems and > drivers, this new function will still be useful. That is because, even > though it can be used as a drop-in replacement for > device_add_properties(), it does add that one small but important > change. It allows the nodes created with it to be part of node > hierarchy, and that alone is useful to me, and I'm planning on using > that feature later. > > I'm sorry about the long explanation. No need to be sorry, now I know what this really is about. :-) I'm not against this patch, but I IMO the "motivation" part of the changelog needs to be improved. If the final goal is to get rid of device_add_properties(), please spell it out and say what problems there are with it and why the new function will be better.
On Wed, Feb 03, 2021 at 02:50:24PM +0100, Rafael J. Wysocki wrote: > On Wed, Feb 3, 2021 at 10:45 AM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote: > > > It looks like there is a use case that cannot be addressed by using > > > device_add_properties() and that's why you need this new function. > > > > > > Can you describe that use case, please, and explain what the problem > > > with using device_add_properties() in it is? > > > > The problem with device_add_properties() is that it gives false > > impression that the device properties are somehow directly attached to > > the devices, which is not true. Now, that should not be a major issue, > > but it seems that it is. I think Lee Jones basically used that as an > > argument to refuse changes (and pretty minor changes) that would have > > allowed us to use software nodes with the MFD drivers. > > > > Nevertheless, I was not planning to provide a replacement for it > > originally. We do in any case have the real issue caused by that > > device_remove_properties() call in device_del() which has to be fixed. > > What's that issue, specifically? The problem is that we can't now reuse or share if necessary, or just in general be in charge of the lifetime of the software nodes because that call is in device_del(). Now the lifetime of the software nodes is always tied to the devices they are attached, no questions asked. > > I started to fix that by moving device_add_properties() under > > drivers/base/swnode.c so I can implement it like I've implemented now > > that new function, but after that when I started to tackle the second > > issue by removing the subsystem wrappers like > > platform_device_add_device_properties() and replacing them with things > > like platform_device_add_software_node() in order to give the drivers > > a chance to actually use software nodes, I realised that there isn't > > much use for device_add_properties() after that. > > > > Also, even though I'm not super happy about adding more API to this > > thing, this function - device_create_managed_software_node() - I do > > like. Not only is it implemented so that we don't have to rely on > > anything else in drivers core in order to achieve the lifetime link > > with the device, it is also much more descriptive. The function name > > alone does not leave any questions about what is going to happen if > > that function is called. You'll end up with a software node that just > > happens to be attached to the device. > > > > On top of those two things, by adding the new function it also gives > > me a nice stepping stone to do these changes in the normal stages: > > > > 1. Add feature/modification. > > 2. Convert users. > > 3. Cleanup. > > > > And finally, even though we may not have any users left for > > device_add_properties() after I have updated all the subsystems and > > drivers, this new function will still be useful. That is because, even > > though it can be used as a drop-in replacement for > > device_add_properties(), it does add that one small but important > > change. It allows the nodes created with it to be part of node > > hierarchy, and that alone is useful to me, and I'm planning on using > > that feature later. > > > > I'm sorry about the long explanation. > > No need to be sorry, now I know what this really is about. :-) > > I'm not against this patch, but I IMO the "motivation" part of the > changelog needs to be improved. > > If the final goal is to get rid of device_add_properties(), please > spell it out and say what problems there are with it and why the new > function will be better. Sure thing. Thanks Rafael. Br,
On Wed, Feb 3, 2021 at 3:27 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > On Wed, Feb 03, 2021 at 02:50:24PM +0100, Rafael J. Wysocki wrote: > > On Wed, Feb 3, 2021 at 10:45 AM Heikki Krogerus > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote: > > > > It looks like there is a use case that cannot be addressed by using > > > > device_add_properties() and that's why you need this new function. > > > > > > > > Can you describe that use case, please, and explain what the problem > > > > with using device_add_properties() in it is? > > > > > > The problem with device_add_properties() is that it gives false > > > impression that the device properties are somehow directly attached to > > > the devices, which is not true. Now, that should not be a major issue, > > > but it seems that it is. I think Lee Jones basically used that as an > > > argument to refuse changes (and pretty minor changes) that would have > > > allowed us to use software nodes with the MFD drivers. > > > > > > Nevertheless, I was not planning to provide a replacement for it > > > originally. We do in any case have the real issue caused by that > > > device_remove_properties() call in device_del() which has to be fixed. > > > > What's that issue, specifically? > > The problem is that we can't now reuse or share if necessary, or just > in general be in charge of the lifetime of the software nodes because > that call is in device_del(). Now the lifetime of the software nodes > is always tied to the devices they are attached, no questions asked. I see and so instead you want the reference counting to trigger the cleanup when the count gets to 0. Sounds reasonable to me and please put this information into the patch changelog.
On Wed, Feb 03, 2021 at 03:39:02PM +0100, Rafael J. Wysocki wrote: > On Wed, Feb 3, 2021 at 3:27 PM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > On Wed, Feb 03, 2021 at 02:50:24PM +0100, Rafael J. Wysocki wrote: > > > On Wed, Feb 3, 2021 at 10:45 AM Heikki Krogerus > > > <heikki.krogerus@linux.intel.com> wrote: > > > > > > > > On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote: > > > > > It looks like there is a use case that cannot be addressed by using > > > > > device_add_properties() and that's why you need this new function. > > > > > > > > > > Can you describe that use case, please, and explain what the problem > > > > > with using device_add_properties() in it is? > > > > > > > > The problem with device_add_properties() is that it gives false > > > > impression that the device properties are somehow directly attached to > > > > the devices, which is not true. Now, that should not be a major issue, > > > > but it seems that it is. I think Lee Jones basically used that as an > > > > argument to refuse changes (and pretty minor changes) that would have > > > > allowed us to use software nodes with the MFD drivers. > > > > > > > > Nevertheless, I was not planning to provide a replacement for it > > > > originally. We do in any case have the real issue caused by that > > > > device_remove_properties() call in device_del() which has to be fixed. > > > > > > What's that issue, specifically? > > > > The problem is that we can't now reuse or share if necessary, or just > > in general be in charge of the lifetime of the software nodes because > > that call is in device_del(). Now the lifetime of the software nodes > > is always tied to the devices they are attached, no questions asked. > > I see and so instead you want the reference counting to trigger the > cleanup when the count gets to 0. > > Sounds reasonable to me and please put this information into the patch > changelog. Yes. I'll do that. thanks,
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 20faa9f4f5ed2..37179a8b1ceba 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -24,6 +24,7 @@ struct swnode { struct swnode *parent; unsigned int allocated:1; + unsigned int managed:1; }; static DEFINE_IDA(swnode_root_ids); @@ -1045,6 +1046,43 @@ void device_remove_software_node(struct device *dev) } EXPORT_SYMBOL_GPL(device_remove_software_node); +/** + * device_create_managed_software_node - Create a software node for a device + * @dev: The device the software node is assigned to. + * @properties: Device properties for the software node. + * @parent: Parent of the software node. + * + * Creates a software node as a managed resource for @dev, which means the + * lifetime of the newly created software node is tied to the lifetime of @dev. + * Software nodes created with this function should not be reused or shared + * because of that. The function takes a deep copy of @properties for the + * software node. + * + * Since the new software node is assigned directly to @dev, and since it should + * not be shared, it is not returned to the caller. The function returns 0 on + * success, and errno in case of an error. + */ +int device_create_managed_software_node(struct device *dev, + const struct property_entry *properties, + const struct software_node *parent) +{ + struct fwnode_handle *p = software_node_fwnode(parent); + struct fwnode_handle *fwnode; + + if (parent && !p) + return -EINVAL; + + fwnode = fwnode_create_software_node(properties, p); + if (IS_ERR(fwnode)) + return PTR_ERR(fwnode); + + to_swnode(fwnode)->managed = true; + set_secondary_fwnode(dev, fwnode); + + return 0; +} +EXPORT_SYMBOL_GPL(device_create_managed_software_node); + int software_node_notify(struct device *dev, unsigned long action) { struct swnode *swnode; @@ -1073,6 +1111,11 @@ int software_node_notify(struct device *dev, unsigned long action) sysfs_remove_link(&swnode->kobj, dev_name(dev)); sysfs_remove_link(&dev->kobj, "software_node"); kobject_put(&swnode->kobj); + + if (swnode->managed) { + set_secondary_fwnode(dev, NULL); + kobject_put(&swnode->kobj); + } break; default: break; diff --git a/include/linux/property.h b/include/linux/property.h index b0e413dc59271..dafccfce02624 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -491,4 +491,8 @@ 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); +int device_create_managed_software_node(struct device *dev, + const struct property_entry *properties, + const struct software_node *parent); + #endif /* _LINUX_PROPERTY_H_ */
Adding function device_create_managed_software_node() that is designed to work as a drop-in replacement for device_add_properties(). The function has one additional feature compared to device_add_properties(). It takes also an optional parent node as parameter, and that way allow the nodes created with it to be part of a node hierarchy. The lifetime of the software nodes created with this function will be tied to the device they are assigned to. The function will therefore behave exactly the same way as device_add_properties() is expected to behave, except that it does not simply assume that the nodes attached to the device are always destroyed in device_del() unconditionally. The nodes created with this function are guaranteed to be removed when the device is removed even after device_del() stops calling device_remove_properties() unconditionally. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/base/swnode.c | 43 ++++++++++++++++++++++++++++++++++++++++ include/linux/property.h | 4 ++++ 2 files changed, 47 insertions(+)