Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <81dcbf72-92bb-093a-da48-89a73ead820e@quicinc.com>
References: <1659526134-22978-1-git-send-email-quic_krichai@quicinc.com>
 <1659526134-22978-3-git-send-email-quic_krichai@quicinc.com>
 <CAE-0n500y-n+ZjasYQRAa3JgamQG1c+Aqn0YiX-i0L-w6C4dbQ@mail.gmail.com>
 <3d052733-3600-b6eb-baf3-d8806a150af3@quicinc.com> <CAE-0n53oMnnn7rOPEiibc=XM52z9THDc9jYhe3x3C_AsLtmARQ@mail.gmail.com>
 <81dcbf72-92bb-093a-da48-89a73ead820e@quicinc.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Fri, 26 Aug 2022 15:23:00 -0500
Message-ID: <CAE-0n50NRiBNDjK2UrA_wOoRz3+3cKb4uiUiCw4t1F19Kw9EhA@mail.gmail.com>
Subject: Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci suspend
To:     Krishna Chaitanya Chundru <quic_krichai@quicinc.com>,
        helgaas@kernel.org
Cc:     linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, mka@chromium.org,
        quic_vbadigan@quicinc.com, quic_hemantk@quicinc.com,
        quic_nitegupt@quicinc.com, quic_skananth@quicinc.com,
        quic_ramkri@quicinc.com, manivannan.sadhasivam@linaro.org,
        dmitry.baryshkov@linaro.org, Jingoo Han <jingoohan1@gmail.com>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Rob Herring <robh@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Stanimir Varbanov <svarbanov@mm-sol.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Quoting Krishna Chaitanya Chundru (2022-08-25 06:52:43)
>
> On 8/24/2022 10:50 PM, Stephen Boyd wrote:
> > Quoting Krishna Chaitanya Chundru (2022-08-23 20:37:59)
> >> On 8/9/2022 12:42 AM, Stephen Boyd wrote:
> >>> Quoting Krishna chaitanya chundru (2022-08-03 04:28:53)
> >>>> If the endpoint device state is D0 and irq's are not freed, then
> >>>> kernel try to mask interrupts in system suspend path by writing
> >>>> in to the vector table (for MSIX interrupts) and config space (for M=
SI's).
> >>>>
> >>>> These transactions are initiated in the pm suspend after pcie clocks=
 got
> >>>> disabled as part of platform driver pm  suspend call. Due to it, the=
se
> >>>> transactions are resulting in un-clocked access and eventually to cr=
ashes.
> >>> Why are the platform driver pm suspend calls disabling clks that earl=
y?
> >>> Can they disable clks in noirq phase, or even later, so that we don't
> >>> have to check if the device is clocking in the irq poking functions?
> >>> It's best to keep irq operations fast, so that irq control is fast gi=
ven
> >>> that these functions are called from irq flow handlers.
> >> We are registering the pcie pm suspend ops as noirq ops only. And this
> >> msix and config
> >>
> >> access is coming at the later point of time that is reason we added th=
at
> >> check.
> >>
> > What is accessing msix and config? Can you dump_stack() after noirq ops
> > are called and figure out what is trying to access the bus when it is
> > powered down?
>
> The msix and config space is being accessed to mask interrupts. The
> access is coming at the end of the suspend
>
> and near CPU disable. We tried to dump the stack there but the call
> stack is not coming as it is near cpu disable.

That is odd that you can't get a stacktrace.

>
> But we got dump at resume please have look at it
>
> [=C2=A0=C2=A0 54.946268] Enabling non-boot CPUs ...
> [=C2=A0=C2=A0 54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41=
 #105
> 43491e4414b1db8a6f59d56b617b520d92a9498e
> [=C2=A0=C2=A0 54.961122] Hardware name: Qualcomm Technologies, Inc. sc728=
0 IDP
> SKU2 platform (DT)
> [=C2=A0=C2=A0 54.969088] Call trace:
> [=C2=A0=C2=A0 54.971612]=C2=A0 dump_backtrace+0x0/0x200
> [=C2=A0=C2=A0 54.975399]=C2=A0 show_stack+0x20/0x2c
> [=C2=A0=C2=A0 54.978826]=C2=A0 dump_stack_lvl+0x6c/0x90
> [=C2=A0=C2=A0 54.982614]=C2=A0 dump_stack+0x18/0x38
> [=C2=A0=C2=A0 54.986043]=C2=A0 dw_msi_unmask_irq+0x2c/0x58
> [=C2=A0=C2=A0 54.990096]=C2=A0 irq_enable+0x58/0x90
> [=C2=A0=C2=A0 54.993522]=C2=A0 __irq_startup+0x68/0x94
> [=C2=A0=C2=A0 54.997216]=C2=A0 irq_startup+0xf4/0x140
> [=C2=A0=C2=A0 55.000820]=C2=A0 irq_affinity_online_cpu+0xc8/0x154
> [=C2=A0=C2=A0 55.005491]=C2=A0 cpuhp_invoke_callback+0x19c/0x6e4
> [=C2=A0=C2=A0 55.010077]=C2=A0 cpuhp_thread_fun+0x11c/0x188
> [=C2=A0=C2=A0 55.014216]=C2=A0 smpboot_thread_fn+0x1ac/0x30c
> [=C2=A0=C2=A0 55.018445]=C2=A0 kthread+0x140/0x30c
> [=C2=A0=C2=A0 55.021788]=C2=A0 ret_from_fork+0x10/0x20
> [=C2=A0=C2=A0 55.028243] CPU1 is up
>
> So the same stack should be called at the suspend path while disabling CP=
U.

Sounds like you're getting hit by affinity changes while offlining CPUs
during suspend (see irq_migrate_all_off_this_cpu()). That will happen
after devices are suspended (all phases of suspend ops).

>
> If there is any other way to remove these calls can you please help us
> point that way.

I'm not sure. I believe genirq assumes the irqchips are always
accessible. There is some support to suspend irqchips. See how the
struct irq_chip::irq_suspend() function is called by syscore ops in the
generic irqchip 'irq_gc_syscore_ops' hooks. Maybe you could add a
syscore suspend/resume hook to disable/enable the clks and power to the
PCI controller. syscore ops run after secondary CPUs are hotplugged out
during suspend.

Or maybe setting the IRQCHIP_MASK_ON_SUSPEND flag can be used so that on
irq migration nothing writes the irq hardware because it is already
masked in the hardware earlier. I think the problem is that on resume
we'll restart the irq from the first CPU online event, when you don't
want to do that because it is too early.

I have another question though, which is do MSIs support wakeup? I don't
see how it works if the whole bus is effectively off during suspend. If
wakeup needs to be supported then I suspect the bus can't be powered
down during suspend.