| Message ID | 20210111141045.14027-1-heikki.krogerus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Remove one more platform_device_add_properties() call | expand |
Heikki Krogerus <heikki.krogerus@linux.intel.com> writes: > Hi Felipe, Rafael, > > This is the second version of this series. There are no real changes, > but I added the Tiger Lake ID patch to this series in hope that it > will make your life a bit easier, assuming that Rafael will still pick > these. > > > The original over letter: > > I originally introduced these as part of my series where I was > proposing PM ops for software nodes [1], but since that still needs > work, I'm sending these two separately. > > So basically I'm only modifying dwc3-pci.c so it registers a software > node directly at this point. That will remove one more user of > platform_device_add_properties(). > > [1] https://lore.kernel.org/lkml/20201029105941.63410-1-heikki.krogerus@linux.intel.com/ > > thanks, > > Heikki Krogerus (3): > software node: Introduce device_add_software_node() > usb: dwc3: pci: Register a software node for the dwc3 platform device > usb: dwc3: pci: ID for Tiger Lake CPU Looks good to me. Acked-by: Felipe Balbi <balbi@kernel.org>
On Mon, Jan 11, 2021 at 05:10:42PM +0300, Heikki Krogerus wrote: > Hi Felipe, Rafael, > > This is the second version of this series. There are no real changes, > but I added the Tiger Lake ID patch to this series in hope that it > will make your life a bit easier, assuming that Rafael will still pick > these. I can take all 3 of these if that makes it easier. Rafael, let me know what you want to do, either is fine with me. thanks, greg k-h
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. -- heikki
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. -- With Best Regards, Andy Shevchenko
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()? -- 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,
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, -- heikki
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,
On Tue, Jan 12, 2021 at 12:49:14PM +0100, Greg KH wrote: > On Mon, Jan 11, 2021 at 05:10:42PM +0300, Heikki Krogerus wrote: > > Hi Felipe, Rafael, > > > > This is the second version of this series. There are no real changes, > > but I added the Tiger Lake ID patch to this series in hope that it > > will make your life a bit easier, assuming that Rafael will still pick > > these. > > I can take all 3 of these if that makes it easier. Rafael, let me know > what you want to do, either is fine with me. I've added it to my usb-next branch now. thanks, greg k-h