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.
Reviewed-by: Andy Shevchenko <[email protected]>
Acked-by: Zhangfei Gao <[email protected]>
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/pci/quirks.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b6b4c803bdc94..fe5eedba47908 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1850,7 +1850,7 @@ static void quirk_huawei_pcie_sva(struct pci_dev *pdev)
* can set it directly.
*/
if (!pdev->dev.of_node &&
- device_add_properties(&pdev->dev, properties))
+ device_create_managed_software_node(&pdev->dev, properties, NULL))
pci_warn(pdev, "could not add stall property");
}
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva);
--
2.33.0
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.
> Reviewed-by: Andy Shevchenko <[email protected]>
> Acked-by: Zhangfei Gao <[email protected]>
> Signed-off-by: Heikki Krogerus <[email protected]>
> ---
> drivers/pci/quirks.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index b6b4c803bdc94..fe5eedba47908 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -1850,7 +1850,7 @@ static void quirk_huawei_pcie_sva(struct pci_dev *pdev)
> * can set it directly.
> */
> if (!pdev->dev.of_node &&
> - device_add_properties(&pdev->dev, properties))
> + device_create_managed_software_node(&pdev->dev, properties, NULL))
> pci_warn(pdev, "could not add stall property");
> }
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva);
> --
> 2.33.0
>
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().
SoB...
thanks,
--
heikki
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 <[email protected]>
Bjorn