| Message ID | 20241007092447.18616-1-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] PCI/pwrctl: pwrseq: abandon QCom WCN probe on pre-pwrseq device-trees | expand |
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
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 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
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, }, { } };