linux-kernel - Re: [PATCH 1/4] PCI: qcom: Reshuffle reset logic in 2_7

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZZU5jqJ14HscR1Ed@hovoldconsulting.com>
Date: Wed, 3 Jan 2024 11:40:14 +0100
From: Johan Hovold <johan@...nel.org>
To: Konrad Dybcio <konrad.dybcio@...aro.org>
Cc: Manivannan Sadhasivam <mani@...nel.org>,
	Bjorn Andersson <andersson@...nel.org>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Stanimir Varbanov <svarbanov@...sol.com>,
	Andrew Murray <amurray@...goodpenguin.co.uk>,
	Vinod Koul <vkoul@...nel.org>,
	Marijn Suijten <marijn.suijten@...ainline.org>,
	linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] PCI: qcom: Reshuffle reset logic in 2_7_0 .init

On Tue, Jan 02, 2024 at 06:03:36PM +0100, Konrad Dybcio wrote:
> On 2.01.2024 11:17, Johan Hovold wrote:
> > On Sat, Dec 30, 2023 at 02:16:18AM +0100, Konrad Dybcio wrote:
> >> On 29.12.2023 16:29, Johan Hovold wrote:
> >>> On Fri, Dec 29, 2023 at 04:01:27PM +0100, Konrad Dybcio wrote:
> >>>> On 29.12.2023 15:04, Johan Hovold wrote:
> >>>>> On Wed, Dec 27, 2023 at 11:17:19PM +0100, Konrad Dybcio wrote:
> >>>>>> At least on SC8280XP, if the PCIe reset is asserted, the corresponding
> >>>>>> AUX_CLK will be stuck at 'off'.
> >>>>>
> >>>>> No, this path is exercised on every boot without the aux clock ever
> >>>>> being stuck at off. So something is clearly missing in this description.
> >>>
> >>>> That's likely because the hardware has been initialized and not cleanly
> >>>> shut down by your bootloader. When you reset it, or your bootloader
> >>>> wasn't so kind, you need to start initialization from scratch.
> >>>
> >>> What does that even mean? I'm telling you that this reset is asserted on
> >>> each boot, on all sc8280xp platforms I have access to, and never have I
> >>> seen the aux clk stuck at off.
> >>>
> >>> So clearly your claim above is too broad and the commit message is
> >>> incorrect or incomplete.

> > We're clearly talking past each other. When I'm saying reset is asserted
> > on each boot, I'm referring to reset being asserted in
> > qcom_pcie_init_2_7_0(), whereas you appear to be referring to whether
> > the reset has been left asserted by the bootloader when the driver
> > probes.
> 
> OK, "boot" meant "booting the device" to me, not the PCIe controller.

Still not getting across to you apparently.

Again, the code in question is exercised on every boot and not once have
I seen a stuck clock due to reset being asserted *in*
qcom_pcie_init_2_7_0().

Now that's not what you were trying to describe as you were thinking of
reset having been left asserted *before* the driver probes (or before
qcom_pcie_init_2_7_0() is called).

See? Do you understand now what I was trying to say and why my
misinterpretation of your terse commit message lead me to claim that it
was clearly false?

> > I understand what you meant to say now, but I think you should rephrase:
> > 
> > 	At least on SC8280XP, if the PCIe reset is asserted, the
> > 	corresponding AUX_CLK will be stuck at 'off'.
> > 
> > because as it stands, it sounds like the problem happens when the driver
> > asserts reset.
> 
> Does this sound good?
> 
> "At least on SC8280XP, trying to enable the AUX_CLK associated with
> a PCIe host fails if the corresponding PCIe reset is asserted."

Yes, but you need to also say something about how this would happen, for
example, your hypothetical bootloader leaving it asserted and your actual
motivation for this change which is that it appears to be needed after
suspend. 

A commit message should be clear and self-contained and not force
reviewers to have to try to interpret what it means and guess what the
motivation for the change really is.

Johan