Re: [PATCH 1/2] PCI: Use software node API with additional device properties

[Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>]

Hi guys,

On Thu, Sep 30, 2021 at 10:33:27AM +0800, Zhangfei Gao wrote:
> On 2021/9/30 上午1:08, Bjorn Helgaas wrote:
> > [+cc Zhangfei, author of 8304a3a199ee ("PCI: Set dma-can-stall for
> > HiSilicon chips"), which added this]
> > 
> > On Wed, Sep 29, 2021 at 04:37:28PM +0300, Heikki Krogerus wrote:
> > > Using device_create_managed_software_node() to inject the
> > > properties in quirk_huawei_pcie_sva() instead of with the
> > > old device_add_properties() API.
> > > 
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > This is fine with me, but please update the subject line and commit
> > log something like this:
> > 
> >    PCI: Convert to device_create_managed_software_node()
> > 
> >    In quirk_huawei_pcie_sva(), use device_create_managed_software_node()
> >    instead of device_add_properties() to set the "dma-can-stall"
> >    property.
> > 
> >    This resolves a software node lifetime issue (see 151f6ff78cdf
> >    ("software node: Provide replacement for device_add_properties()"))
> >    and paves the way for removing device_add_properties() completely.
> > 
> > Actually, 8304a3a199ee was merged during the v5.15 merge window, so if
> > this does in fact fix a lifetime issue, I can merge this before
> > v5.15-final.

It does not fix lifetime issues. This is because device_del() called
device_remove_properties() unconditionally with every device.

There should be no functional impact.

> > I know *this* quirk applies to AMBA devices, and I assume they cannot
> > be removed, so there's no actual lifetime problem in this particular
> > case, but in general it looks like a problem for PCI devices.
> Thanks Bjorn
> This patch also works, though the quirk is for platform devices and not
> removed.

If the device is really never removed, then we could also constify the
node and the properties in it. Then the patch would look like this:

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b6b4c803bdc94..3dc7a1c62bf24 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1833,13 +1833,17 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_HUAWEI, 0x1610, PCI_CLASS_BRIDGE_PCI
  * even when a "PCI" device turns out to be a regular old SoC device
  * dressed up as a RCiEP and normal rules don't apply.
  */
+static const struct property_entry huawei_pcie_sva_props[] = {
+       PROPERTY_ENTRY_BOOL("dma-can-stall"),
+       { }
+};
+
+static const struct software_node huawei_pcie_sva_swnode = {
+       .properties = huawei_pcie_sva_props,
+};
+
 static void quirk_huawei_pcie_sva(struct pci_dev *pdev)
 {
-       struct property_entry properties[] = {
-               PROPERTY_ENTRY_BOOL("dma-can-stall"),
-               {},
-       };
-
        if (pdev->revision != 0x21 && pdev->revision != 0x30)
                return;
 
@@ -1850,7 +1854,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_add_software_node(&pdev->dev, &huawei_pcie_sva_swnode))
                pci_warn(pdev, "could not add stall property");
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_HUAWEI, 0xa250, quirk_huawei_pcie_sva);


Let me know if you prefer it that way.

thanks,

-- 
heikki