| Message ID | 20210929133729.9427-2-heikki.krogerus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | device property: Remove device_add_properties() | expand |
[+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@linux.intel.com>
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.
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.
> ---
> 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 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@linux.intel.com> > 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. > > 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. Acked-by: Zhangfei Gao <zhangfei.gao@linaro.org> > >> --- >> 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 >>
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@linux.intel.com> > > 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,
On Thu, Sep 30, 2021 at 1:06 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > On Thu, Sep 30, 2021 at 10:33:27AM +0800, Zhangfei Gao wrote: ... > 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: I'm not sure the user can't force removal of the device (via PCI rescan, for example,, or via unbind/bind cycle). I guess this way should be really taken carefully.
On Thu, Sep 30, 2021 at 12:20 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Sep 30, 2021 at 1:06 PM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > On Thu, Sep 30, 2021 at 10:33:27AM +0800, Zhangfei Gao wrote: > > ... > > > 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: > > I'm not sure the user can't force removal of the device (via PCI > rescan, for example,, or via unbind/bind cycle). The sysfs unbind doesn't remove the device, though, AFAICS. It just unbinds the driver from it, if any. > I guess this way should be really taken carefully. But I agree.
On Thu, Sep 30, 2021 at 12:37:27PM +0200, Rafael J. Wysocki wrote: > On Thu, Sep 30, 2021 at 12:20 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Thu, Sep 30, 2021 at 1:06 PM Heikki Krogerus > > <heikki.krogerus@linux.intel.com> wrote: > > > On Thu, Sep 30, 2021 at 10:33:27AM +0800, Zhangfei Gao wrote: > > > > ... > > > > > 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: > > > > I'm not sure the user can't force removal of the device (via PCI > > rescan, for example,, or via unbind/bind cycle). > > The sysfs unbind doesn't remove the device, though, AFAICS. It just > unbinds the driver from it, if any. > > > I guess this way should be really taken carefully. > > But I agree. OK. Makes sense. Thanks guys,
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);
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@linux.intel.com> --- drivers/pci/quirks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)