On Thu, Feb 01, 2024 at 04:55:31PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> Some PCI devices must be powered-on before they can be detected on the
> bus. Introduce a simple framework reusing the existing PCI OF
> infrastructure.
>
> The way this works is: a DT node representing a PCI device connected to
> the port can be matched against its power control platform driver. If
> the match succeeds, the driver is responsible for powering-up the device
> and calling pcie_pwrctl_device_enable() which will trigger a PCI bus
> rescan as well as subscribe to PCI bus notifications.
>
> When the device is detected and created, we'll make it consume the same
> DT node that the platform device did. When the device is bound, we'll
> create a device link between it and the parent power control device.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> ---
> drivers/pci/Kconfig | 1 +
> drivers/pci/Makefile | 1 +
> drivers/pci/pwrctl/Kconfig | 8 ++++
> drivers/pci/pwrctl/Makefile | 3 ++
> drivers/pci/pwrctl/core.c | 82 +++++++++++++++++++++++++++++++++++++
> include/linux/pci-pwrctl.h | 24 +++++++++++
> 6 files changed, 119 insertions(+)
> create mode 100644 drivers/pci/pwrctl/Kconfig
> create mode 100644 drivers/pci/pwrctl/Makefile
> create mode 100644 drivers/pci/pwrctl/core.c
> create mode 100644 include/linux/pci-pwrctl.h
>
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 74147262625b..5b9b84f8774f 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -291,5 +291,6 @@ source "drivers/pci/hotplug/Kconfig"
> source "drivers/pci/controller/Kconfig"
> source "drivers/pci/endpoint/Kconfig"
> source "drivers/pci/switch/Kconfig"
> +source "drivers/pci/pwrctl/Kconfig"
>
> endif
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index cc8b4e01e29d..6ae202d950f8 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_PCI) += access.o bus.o probe.o host-bridge.o \
>
> obj-$(CONFIG_PCI) += msi/
> obj-$(CONFIG_PCI) += pcie/
> +obj-$(CONFIG_PCI) += pwrctl/
>
> ifdef CONFIG_PCI
> obj-$(CONFIG_PROC_FS) += proc.o
> diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
> new file mode 100644
> index 000000000000..e2dc5e5d2af1
> --- /dev/null
> +++ b/drivers/pci/pwrctl/Kconfig
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +menu "PCI Power control drivers"
> +
> +config PCI_PWRCTL
> + bool
> +
> +endmenu
> diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile
> new file mode 100644
> index 000000000000..4381cfbf3f21
> --- /dev/null
> +++ b/drivers/pci/pwrctl/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_PCI_PWRCTL) += core.o
> diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
> new file mode 100644
> index 000000000000..312e6fe95c31
> --- /dev/null
> +++ b/drivers/pci/pwrctl/core.c
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024 Linaro Ltd.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/pci-pwrctl.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +
> +static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
> + void *data)
> +{
> + struct pci_pwrctl *pwrctl = container_of(nb, struct pci_pwrctl, nb);
> + struct device *dev = data;
> +
> + if (dev_fwnode(dev) != dev_fwnode(pwrctl->dev))
> + return NOTIFY_DONE;
> +
> + switch (action) {
> + case BUS_NOTIFY_ADD_DEVICE:
> + device_set_of_node_from_dev(dev, pwrctl->dev);
What happens if the bootloader left the power on, and the
of_platform_populate() got probe deferred because the pwrseq wasn't
ready, so this happens after pci_set_of_node() has been called?
(I think dev->of_node will be put, then get and then node_reused
assigned...but I'm not entirely sure)
> + break;
> + case BUS_NOTIFY_BOUND_DRIVER:
> + pwrctl->link = device_link_add(dev, pwrctl->dev,
> + DL_FLAG_AUTOREMOVE_CONSUMER);
> + if (!pwrctl->link)
> + dev_err(pwrctl->dev, "Failed to add device link\n");
> + break;
> + case BUS_NOTIFY_UNBOUND_DRIVER:
> + device_link_del(pwrctl->link);
This might however become a NULL-pointer dereference, if dev was bound
to its driver before the pci_pwrctl_notify() was registered for the
pwrctl and then the PCI device is unbound.
This would also happen if device_link_add() failed when the PCI device
was bound...
> + break;
> + }
> +
> + return NOTIFY_DONE;
> +}
> +
> +int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl)
This function doesn't really "enable the device", looking at the example
driver it's rather "device_enabled" than "device_enable"...
> +{
> + if (!pwrctl->dev)
> + return -ENODEV;
> +
> + pwrctl->nb.notifier_call = pci_pwrctl_notify;
> + bus_register_notifier(&pci_bus_type, &pwrctl->nb);
> +
> + pci_lock_rescan_remove();
> + pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus);
> + pci_unlock_rescan_remove();
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_pwrctl_device_enable);
> +
> +void pci_pwrctl_device_disable(struct pci_pwrctl *pwrctl)
Similarly this doesn't disable the device, the code calling this is
doing so...
Regards,
Bjorn
> +{
> + bus_unregister_notifier(&pci_bus_type, &pwrctl->nb);
> +}
> +EXPORT_SYMBOL_GPL(pci_pwrctl_device_disable);
> +
> +static void devm_pci_pwrctl_device_disable(void *data)
> +{
> + struct pci_pwrctl *pwrctl = data;
> +
> + pci_pwrctl_device_disable(pwrctl);
> +}
> +
> +int devm_pci_pwrctl_device_enable(struct device *dev,
> + struct pci_pwrctl *pwrctl)
> +{
> + int ret;
> +
> + ret = pci_pwrctl_device_enable(pwrctl);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(dev, devm_pci_pwrctl_device_disable,
> + pwrctl);
> +}
> +EXPORT_SYMBOL_GPL(devm_pci_pwrctl_device_enable);
> diff --git a/include/linux/pci-pwrctl.h b/include/linux/pci-pwrctl.h
> new file mode 100644
> index 000000000000..8d16d27cbfeb
> --- /dev/null
> +++ b/include/linux/pci-pwrctl.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024 Linaro Ltd.
> + */
> +
> +#ifndef __PCI_PWRCTL_H__
> +#define __PCI_PWRCTL_H__
> +
> +#include <linux/notifier.h>
> +
> +struct device;
> +
> +struct pci_pwrctl {
> + struct notifier_block nb;
> + struct device *dev;
> + struct device_link *link;
> +};
> +
> +int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl);
> +void pci_pwrctl_device_disable(struct pci_pwrctl *pwrctl);
> +int devm_pci_pwrctl_device_enable(struct device *dev,
> + struct pci_pwrctl *pwrctl);
> +
> +#endif /* __PCI_PWRCTL_H__ */
> --
> 2.40.1
>
