[PATCH] PCI/pwrctl: pwrseq: abandon probe on pre-pwrseq device-trees

[PATCH] PCI/pwrctl: pwrseq: abandon probe on pre-pwrseq device-trees

[Bartosz Golaszewski]

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 indefinite probe deferral.

Let's check the existence of the regulator supply that exists on all WCN
models before moving forward.

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/
Suggested-by: Stephan Gerhold <stephan.gerhold@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
index a23a4312574b..8ed613655d4a 100644
--- a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
+++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
@@ -9,6 +9,7 @@
 #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>
@@ -31,6 +32,25 @@ static int pci_pwrctl_pwrseq_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int ret;
 
+	/*
+	 * 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.
+	 *
+	 * NOTE: If this driver is ever used to support a device other than
+	 * a WCN chip, the following lines should become conditional and depend
+	 * on the compatible string.
+	 */
+	if (!device_property_present(dev, "vddaon-supply"))
+		return -ENODEV;
+
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
-- 
2.43.0

Re: [PATCH] PCI/pwrctl: pwrseq: abandon probe on pre-pwrseq device-trees

[Bjorn Andersson]

On Fri, Oct 04, 2024 at 02:52:27PM GMT, 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 indefinite probe deferral.
> 
> Let's check the existence of the regulator supply that exists on all WCN
> models before moving forward.
> 
> 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/

Opens-up: A whole bunch of new issues

> Suggested-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> index a23a4312574b..8ed613655d4a 100644
> --- a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> @@ -9,6 +9,7 @@
>  #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>
> @@ -31,6 +32,25 @@ static int pci_pwrctl_pwrseq_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	int ret;
>  
> +	/*
> +	 * 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.
> +	 *
> +	 * NOTE: If this driver is ever used to support a device other than
> +	 * a WCN chip, the following lines should become conditional and depend
> +	 * on the compatible string.

What do you mean "is ever used ... other than WCN chip"?

This driver and the power sequence framework was presented as a
completely generic solution to solve all kinds of PCI power sequence
problems - upon which the WCN case was built.

In fact, if I read this correctly, the second user of the power sequence
implementation (the QPS615, proposed in [1]), would break if this check
is added.

Add to this that your colleagues are pushing people to implement simple
power supplies for M.2-connected devices into this framework - which I
can only assume would trip on this as well (the one supply pin in a M.2.
connector isn't named "vddaon").

[1] https://lore.kernel.org/all/20240803-qps615-v2-3-9560b7c71369@quicinc.com/

Regards,
Bjorn

> +	 */
> +	if (!device_property_present(dev, "vddaon-supply"))
> +		return -ENODEV;
> +
>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
> -- 
> 2.43.0
> 

Re: [PATCH] PCI/pwrctl: pwrseq: abandon probe on pre-pwrseq device-trees

[Bartosz Golaszewski]

On Fri, Oct 4, 2024 at 7:31 PM Bjorn Andersson <andersson@kernel.org> wrote:
>
> >
> > +     /*
> > +      * 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.
> > +      *
> > +      * NOTE: If this driver is ever used to support a device other than
> > +      * a WCN chip, the following lines should become conditional and depend
> > +      * on the compatible string.
>
> What do you mean "is ever used ... other than WCN chip"?
>

This driver was released as part of v6.11 and so far (until v6.12) is
only used to support the WCN chips. That's not to say that it cannot
be extended to support more hardware. I don't know how to put it in
simpler words.

> This driver and the power sequence framework was presented as a
> completely generic solution to solve all kinds of PCI power sequence
> problems - upon which the WCN case was built.
>

I never presented anything as "completely generic". You demanded that
I make it into a miraculous catch-all solution. I argued that there's
no such thing and this kind of attitude is precisely why it's so hard
to get anything done in the kernel. I made it *generic enough* and we
can cross any bridge requiring new features when we get to it. This is
why we have no stable APIs in the kernel! And why every long-lived
user-space library is at major API version 2 or 3. You can never
possibly get *everything* right.

Also: there's a big difference between the framework and this driver.
A driver is just a consumer of the larger framework. We could possibly
make it WCN-specific and create a new one for QPS615 (even if it was
to use pwrseq as well) instead of cramming a solution for every
possible corner case into a single compilation unit.

> In fact, if I read this correctly, the second user of the power sequence
> implementation (the QPS615, proposed in [1]), would break if this check
> is added.
>

Is it queued for v6.13 yet? If not, then we make no guarantees. A
regression is when something upstream stops working, not when
yet-unmerged patches from the list do. Have you really never had to
modify existing code to accommodate new one?

This is a fix that needs to go into v6.12 and be backported to v6.11.
Hence a simple patch. For v6.13 we can easily extend the match data to
become a structure indicating whether we should do the check or not.
That's a really simple change too. But it would grow the fix
needlessly.

> Add to this that your colleagues are pushing people to implement simple
> power supplies for M.2-connected devices into this framework - which I
> can only assume would trip on this as well (the one supply pin in a M.2.
> connector isn't named "vddaon").
>
> [1] https://lore.kernel.org/all/20240803-qps615-v2-3-9560b7c71369@quicinc.com/
>

I'm not sure what exactly you're referring to here.

Bart

Re: [PATCH] PCI/pwrctl: pwrseq: abandon probe on pre-pwrseq device-trees

[Bjorn Andersson]

On Fri, Oct 04, 2024 at 07:59:41PM GMT, Bartosz Golaszewski wrote:
> On Fri, Oct 4, 2024 at 7:31 PM Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > >
> > > +     /*
> > > +      * 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.
> > > +      *
> > > +      * NOTE: If this driver is ever used to support a device other than
> > > +      * a WCN chip, the following lines should become conditional and depend
> > > +      * on the compatible string.
> >
> > What do you mean "is ever used ... other than WCN chip"?
> >
> 
> This driver was released as part of v6.11 and so far (until v6.12) is
> only used to support the WCN chips. That's not to say that it cannot
> be extended to support more hardware. I don't know how to put it in
> simpler words.
> 
> > This driver and the power sequence framework was presented as a
> > completely generic solution to solve all kinds of PCI power sequence
> > problems - upon which the WCN case was built.
> >
> 
> I never presented anything as "completely generic". You demanded that
> I make it into a miraculous catch-all solution.

That is correct. I strongly requested that you would come up with a
solution that worked for BOTH (all two!) use cases we had on the table
for PCI power sequencing.

> I argued that there's no such thing and this kind of attitude is
> precisely why it's so hard to get anything done in the kernel.

I'm sorry that you feel it's my attitude that's the problem here. I
don't think that is what make this hard, but rather the technical
challenges of the problem itself.

Regards,
Bjorn