On Thu, Oct 07, 2021 at 02:03:50PM +0300, Heikki Krogerus wrote: > On Wed, Oct 06, 2021 at 01:47:54PM -0500, Bjorn Helgaas wrote: > > On Wed, Oct 06, 2021 at 02:26:41PM +0300, Heikki Krogerus wrote: > > > In quirk_huawei_pcie_sva(), use device_create_managed_software_node() > > > instead of device_add_properties() to set the "dma-can-stall" > > > property. > > > > > > This is the last user of device_add_properties() that relied on > > > device_del() to take care of also calling device_remove_properties(). > > > After this change we can finally get rid of that > > > device_remove_properties() call in device_del(). > > > > > > After that device_remove_properties() call has been removed from > > > device_del(), the software nodes that hold the additional device > > > properties become reusable and shareable as there is no longer a > > > default assumption that those nodes are lifetime bound the first > > > device they are associated with. > > > > This does not help me determine whether this patch is safe. > > device_create_managed_software_node() sets swnode->managed = true, > > but device_add_properties() did not. I still don't know what the > > effect of that is. > > OK. So how about this: > > PCI: Convert to device_create_managed_software_node() > > In quirk_huawei_pcie_sva(), device_add_properties() is used to > inject additional device properties, but there is no > device_remove_properties() call anywhere to remove those > properties. The assumption is most likely that the device is > never removed, and the properties therefore do not also never > need to be removed. > > Even though it is unlikely that the device is ever removed in > this case, it is safer to make sure that the properties are > also removed if the device ever does get unregistered. > > To achieve this, instead of adding a separate quirk for the > case of device removal where device_remove_properties() is > called, using device_create_managed_software_node() instead of > device_add_properties(). Both functions create a software node > (a type of fwnode) that holds the device properties, which is > then assigned to the device very much the same way. > > The difference between the two functions is, that > device_create_managed_software_node() guarantees that the > software node (together with the properties) is removed when > the device is removed. The function device_add_property() does > _not_ guarantee that, so the properties added with it should > always be removed with device_remove_properties(). That makes sense to me, thanks. And it sounds like it *does* resolve a lifetime issue, namely, a caller of device_add_properties(dev) is required to arrange for device_remove_properties(dev) to be called when "dev" is removed. The fact that in this particular case, "dev" is a non-removable AMBA device doesn't mean there was no issue; it only means we should have had a matching device_remove_properties() call somewhere or at the very least a comment about why it wasn't needed. Otherwise people copy the code to somewhere where it *does* matter. But removing device_add_properties() altogether will mean this is all moot anyway. You can add my: Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> Bjorn
