drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 55 +++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 5 deletions(-)
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Old device trees for some platforms already define wifi nodes for the WCN
family of chips since before power sequencing was added upstream.
These nodes don't consume the regulator outputs from the PMU and if we
allow this driver to bind to one of such "incomplete" nodes, we'll see
a kernel log error about the infinite probe deferral.
Let's extend the driver by adding a platform data struct matched against
the compatible. This struct will now contain the pwrseq target string as
well as a validation function called right after entering probe(). For
Qualcomm WCN models, we'll check the existence of the regulator supply
property that indicates the DT is already using power sequencing and
return -ENODEV if it's not there, indicating to the driver model that
the device should not be bound to the pwrctl driver.
Fixes: 6140d185a43d ("PCI/pwrctl: Add a PCI power control driver for power sequenced devices")
Reported-by: Johan Hovold <johan@kernel.org>
Closes: https://lore.kernel.org/all/Zv565olMDDGHyYVt@hovoldconsulting.com/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
v1 -> v2:
- make the fix more generic, add a platform data struct matched against
the compatible and use it to store a device-specific validation
callback
drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 55 +++++++++++++++++++++++---
1 file changed, 50 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
index a23a4312574b..e331a73a1b0c 100644
--- a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
+++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
@@ -6,9 +6,9 @@
#include <linux/device.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
-#include <linux/of.h>
#include <linux/pci-pwrctl.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/pwrseq/consumer.h>
#include <linux/slab.h>
#include <linux/types.h>
@@ -18,6 +18,40 @@ struct pci_pwrctl_pwrseq_data {
struct pwrseq_desc *pwrseq;
};
+struct pci_pwrctl_pwrseq_pdata {
+ const char *target;
+ /*
+ * Called before doing anything else to perform device-specific
+ * verification between requesting the power sequencing handle.
+ */
+ int (*validate_device)(struct device *dev);
+};
+
+static int pci_pwrctl_pwrseq_qcm_wcn_validate_device(struct device *dev)
+{
+ /*
+ * Old device trees for some platforms already define wifi nodes for
+ * the WCN family of chips since before power sequencing was added
+ * upstream.
+ *
+ * These nodes don't consume the regulator outputs from the PMU and
+ * if we allow this driver to bind to one of such "incomplete" nodes,
+ * we'll see a kernel log error about the indefinite probe deferral.
+ *
+ * Let's check the existence of the regulator supply that exists on all
+ * WCN models before moving forward.
+ */
+ if (!device_property_present(dev, "vddaon-supply"))
+ return -ENODEV;
+
+ return 0;
+}
+
+static const struct pci_pwrctl_pwrseq_pdata pci_pwrctl_pwrseq_qcom_wcn_pdata = {
+ .target = "wlan",
+ .validate_device = pci_pwrctl_pwrseq_qcm_wcn_validate_device,
+};
+
static void devm_pci_pwrctl_pwrseq_power_off(void *data)
{
struct pwrseq_desc *pwrseq = data;
@@ -27,15 +61,26 @@ static void devm_pci_pwrctl_pwrseq_power_off(void *data)
static int pci_pwrctl_pwrseq_probe(struct platform_device *pdev)
{
+ const struct pci_pwrctl_pwrseq_pdata *pdata;
struct pci_pwrctl_pwrseq_data *data;
struct device *dev = &pdev->dev;
int ret;
+ pdata = device_get_match_data(dev);
+ if (!pdata || !pdata->target)
+ return -EINVAL;
+
+ if (pdata->validate_device) {
+ ret = pdata->validate_device(dev);
+ if (ret)
+ return ret;
+ }
+
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
- data->pwrseq = devm_pwrseq_get(dev, of_device_get_match_data(dev));
+ data->pwrseq = devm_pwrseq_get(dev, pdata->target);
if (IS_ERR(data->pwrseq))
return dev_err_probe(dev, PTR_ERR(data->pwrseq),
"Failed to get the power sequencer\n");
@@ -64,17 +109,17 @@ static const struct of_device_id pci_pwrctl_pwrseq_of_match[] = {
{
/* ATH11K in QCA6390 package. */
.compatible = "pci17cb,1101",
- .data = "wlan",
+ .data = &pci_pwrctl_pwrseq_qcom_wcn_pdata,
},
{
/* ATH11K in WCN6855 package. */
.compatible = "pci17cb,1103",
- .data = "wlan",
+ .data = &pci_pwrctl_pwrseq_qcom_wcn_pdata,
},
{
/* ATH12K in WCN7850 package. */
.compatible = "pci17cb,1107",
- .data = "wlan",
+ .data = &pci_pwrctl_pwrseq_qcom_wcn_pdata,
},
{ }
};
--
2.30.2On Mon, Oct 07, 2024 at 11:24:46AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Old device trees for some platforms already define wifi nodes for the WCN
> family of chips since before power sequencing was added upstream.
>
> These nodes don't consume the regulator outputs from the PMU and if we
> allow this driver to bind to one of such "incomplete" nodes, we'll see
> a kernel log error about the infinite probe deferral.
>
> Let's extend the driver by adding a platform data struct matched against
> the compatible. This struct will now contain the pwrseq target string as
> well as a validation function called right after entering probe(). For
> Qualcomm WCN models, we'll check the existence of the regulator supply
> property that indicates the DT is already using power sequencing and
> return -ENODEV if it's not there, indicating to the driver model that
> the device should not be bound to the pwrctl driver.
>
> Fixes: 6140d185a43d ("PCI/pwrctl: Add a PCI power control driver for power sequenced devices")
> Reported-by: Johan Hovold <johan@kernel.org>
> Closes: https://lore.kernel.org/all/Zv565olMDDGHyYVt@hovoldconsulting.com/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Applied to pci/for-linus for v6.12, thanks.
It would help me out to have a hint in the posting that this is
intended for the current release, since by default everything is for
the "next" release.
> ---
> v1 -> v2:
> - make the fix more generic, add a platform data struct matched against
> the compatible and use it to store a device-specific validation
> callback
>
> drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 55 +++++++++++++++++++++++---
> 1 file changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> index a23a4312574b..e331a73a1b0c 100644
> --- a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> @@ -6,9 +6,9 @@
> #include <linux/device.h>
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> -#include <linux/of.h>
> #include <linux/pci-pwrctl.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/pwrseq/consumer.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> @@ -18,6 +18,40 @@ struct pci_pwrctl_pwrseq_data {
> struct pwrseq_desc *pwrseq;
> };
>
> +struct pci_pwrctl_pwrseq_pdata {
> + const char *target;
> + /*
> + * Called before doing anything else to perform device-specific
> + * verification between requesting the power sequencing handle.
> + */
> + int (*validate_device)(struct device *dev);
> +};
> +
> +static int pci_pwrctl_pwrseq_qcm_wcn_validate_device(struct device *dev)
> +{
> + /*
> + * Old device trees for some platforms already define wifi nodes for
> + * the WCN family of chips since before power sequencing was added
> + * upstream.
> + *
> + * These nodes don't consume the regulator outputs from the PMU and
> + * if we allow this driver to bind to one of such "incomplete" nodes,
> + * we'll see a kernel log error about the indefinite probe deferral.
> + *
> + * Let's check the existence of the regulator supply that exists on all
> + * WCN models before moving forward.
> + */
> + if (!device_property_present(dev, "vddaon-supply"))
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static const struct pci_pwrctl_pwrseq_pdata pci_pwrctl_pwrseq_qcom_wcn_pdata = {
> + .target = "wlan",
> + .validate_device = pci_pwrctl_pwrseq_qcm_wcn_validate_device,
> +};
> +
> static void devm_pci_pwrctl_pwrseq_power_off(void *data)
> {
> struct pwrseq_desc *pwrseq = data;
> @@ -27,15 +61,26 @@ static void devm_pci_pwrctl_pwrseq_power_off(void *data)
>
> static int pci_pwrctl_pwrseq_probe(struct platform_device *pdev)
> {
> + const struct pci_pwrctl_pwrseq_pdata *pdata;
> struct pci_pwrctl_pwrseq_data *data;
> struct device *dev = &pdev->dev;
> int ret;
>
> + pdata = device_get_match_data(dev);
> + if (!pdata || !pdata->target)
> + return -EINVAL;
> +
> + if (pdata->validate_device) {
> + ret = pdata->validate_device(dev);
> + if (ret)
> + return ret;
> + }
> +
> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> return -ENOMEM;
>
> - data->pwrseq = devm_pwrseq_get(dev, of_device_get_match_data(dev));
> + data->pwrseq = devm_pwrseq_get(dev, pdata->target);
> if (IS_ERR(data->pwrseq))
> return dev_err_probe(dev, PTR_ERR(data->pwrseq),
> "Failed to get the power sequencer\n");
> @@ -64,17 +109,17 @@ static const struct of_device_id pci_pwrctl_pwrseq_of_match[] = {
> {
> /* ATH11K in QCA6390 package. */
> .compatible = "pci17cb,1101",
> - .data = "wlan",
> + .data = &pci_pwrctl_pwrseq_qcom_wcn_pdata,
> },
> {
> /* ATH11K in WCN6855 package. */
> .compatible = "pci17cb,1103",
> - .data = "wlan",
> + .data = &pci_pwrctl_pwrseq_qcom_wcn_pdata,
> },
> {
> /* ATH12K in WCN7850 package. */
> .compatible = "pci17cb,1107",
> - .data = "wlan",
> + .data = &pci_pwrctl_pwrseq_qcom_wcn_pdata,
> },
> { }
> };
> --
> 2.30.2
>
On Wed, Oct 23, 2024 at 8:36 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, Oct 07, 2024 at 11:24:46AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Old device trees for some platforms already define wifi nodes for the WCN
> > family of chips since before power sequencing was added upstream.
> >
> > These nodes don't consume the regulator outputs from the PMU and if we
> > allow this driver to bind to one of such "incomplete" nodes, we'll see
> > a kernel log error about the infinite probe deferral.
> >
> > Let's extend the driver by adding a platform data struct matched against
> > the compatible. This struct will now contain the pwrseq target string as
> > well as a validation function called right after entering probe(). For
> > Qualcomm WCN models, we'll check the existence of the regulator supply
> > property that indicates the DT is already using power sequencing and
> > return -ENODEV if it's not there, indicating to the driver model that
> > the device should not be bound to the pwrctl driver.
> >
> > Fixes: 6140d185a43d ("PCI/pwrctl: Add a PCI power control driver for power sequenced devices")
> > Reported-by: Johan Hovold <johan@kernel.org>
> > Closes: https://lore.kernel.org/all/Zv565olMDDGHyYVt@hovoldconsulting.com/
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Applied to pci/for-linus for v6.12, thanks.
>
> It would help me out to have a hint in the posting that this is
> intended for the current release, since by default everything is for
> the "next" release.
>
Noted.
Thanks,
Bartosz
On Mon, Oct 7, 2024 at 11:24 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Old device trees for some platforms already define wifi nodes for the WCN
> family of chips since before power sequencing was added upstream.
>
> These nodes don't consume the regulator outputs from the PMU and if we
> allow this driver to bind to one of such "incomplete" nodes, we'll see
> a kernel log error about the infinite probe deferral.
>
> Let's extend the driver by adding a platform data struct matched against
> the compatible. This struct will now contain the pwrseq target string as
> well as a validation function called right after entering probe(). For
> Qualcomm WCN models, we'll check the existence of the regulator supply
> property that indicates the DT is already using power sequencing and
> return -ENODEV if it's not there, indicating to the driver model that
> the device should not be bound to the pwrctl driver.
>
> Fixes: 6140d185a43d ("PCI/pwrctl: Add a PCI power control driver for power sequenced devices")
> Reported-by: Johan Hovold <johan@kernel.org>
> Closes: https://lore.kernel.org/all/Zv565olMDDGHyYVt@hovoldconsulting.com/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
Hi Bjorn (Helgaas),
It's been over two weeks with no feedback but this patch fixes a
regression on at least two ARM64 platforms. Could you also pick it up
into v6.12?
Thanks!
Bartosz
On Mon, Oct 7, 2024 at 11:24 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Old device trees for some platforms already define wifi nodes for the WCN
> family of chips since before power sequencing was added upstream.
>
> These nodes don't consume the regulator outputs from the PMU and if we
> allow this driver to bind to one of such "incomplete" nodes, we'll see
> a kernel log error about the infinite probe deferral.
>
> Let's extend the driver by adding a platform data struct matched against
> the compatible. This struct will now contain the pwrseq target string as
> well as a validation function called right after entering probe(). For
> Qualcomm WCN models, we'll check the existence of the regulator supply
> property that indicates the DT is already using power sequencing and
> return -ENODEV if it's not there, indicating to the driver model that
> the device should not be bound to the pwrctl driver.
>
> Fixes: 6140d185a43d ("PCI/pwrctl: Add a PCI power control driver for power sequenced devices")
> Reported-by: Johan Hovold <johan@kernel.org>
> Closes: https://lore.kernel.org/all/Zv565olMDDGHyYVt@hovoldconsulting.com/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
Bjorn (Andersson), gentle ping. Does this work better for you?
Bartosz
© 2016 - 2024 Red Hat, Inc.