Message-ID: <CAMRc=MekMuV6ULeX_x8mgQiL=XoHuH3PrJLihqucWqowN-YRLQ@mail.gmail.com>
Date: Fri, 4 Oct 2024 19:59:41 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Konrad Dybcio <konradybcio@...nel.org>, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Johan Hovold <johan@...nel.org>, 
	Stephan Gerhold <stephan.gerhold@...aro.org>
Subject: Re: [PATCH] PCI/pwrctl: pwrseq: abandon probe on pre-pwrseq device-trees

On Fri, Oct 4, 2024 at 7:31 PM Bjorn Andersson <andersson@...nel.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

Powered by blists - more mailing lists