Message-ID: <a1b7c47c-9657-54bb-6b4e-1d98b3a65b91@quicinc.com>
Date:   Mon, 29 Aug 2022 23:01:58 +0530
From:   Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>
CC:     <helgaas@...nel.org>, <linux-pci@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <mka@...omium.org>, <quic_vbadigan@...cinc.com>,
        <quic_hemantk@...cinc.com>, <quic_nitegupt@...cinc.com>,
        <quic_skananth@...cinc.com>, <quic_ramkri@...cinc.com>,
        <dmitry.baryshkov@...aro.org>, Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Stanimir Varbanov <svarbanov@...sol.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>
Subject: Re: [PATCH v5 2/3] PCI: qcom: Restrict pci transactions after pci
 suspend


On 8/27/2022 10:56 PM, Manivannan Sadhasivam wrote:
> On Fri, Aug 26, 2022 at 03:23:00PM -0500, Stephen Boyd wrote:
>> 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 MSI'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, these
>>>>>>> transactions are resulting in un-clocked access and eventually to crashes.
>>>>>> Why are the platform driver pm suspend calls disabling clks that early?
>>>>>> 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 given
>>>>>> 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 that
>>>>> 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
>>>
>>> [   54.946268] Enabling non-boot CPUs ...
>>> [   54.951182] CPU: 1 PID: 21 Comm: cpuhp/1 Not tainted 5.15.41 #105
>>> 43491e4414b1db8a6f59d56b617b520d92a9498e
>>> [   54.961122] Hardware name: Qualcomm Technologies, Inc. sc7280 IDP
>>> SKU2 platform (DT)
>>> [   54.969088] Call trace:
>>> [   54.971612]  dump_backtrace+0x0/0x200
>>> [   54.975399]  show_stack+0x20/0x2c
>>> [   54.978826]  dump_stack_lvl+0x6c/0x90
>>> [   54.982614]  dump_stack+0x18/0x38
>>> [   54.986043]  dw_msi_unmask_irq+0x2c/0x58
>>> [   54.990096]  irq_enable+0x58/0x90
>>> [   54.993522]  __irq_startup+0x68/0x94
>>> [   54.997216]  irq_startup+0xf4/0x140
>>> [   55.000820]  irq_affinity_online_cpu+0xc8/0x154
>>> [   55.005491]  cpuhp_invoke_callback+0x19c/0x6e4
>>> [   55.010077]  cpuhp_thread_fun+0x11c/0x188
>>> [   55.014216]  smpboot_thread_fn+0x1ac/0x30c
>>> [   55.018445]  kthread+0x140/0x30c
>>> [   55.021788]  ret_from_fork+0x10/0x20
>>> [   55.028243] CPU1 is up
>>>
>>> So the same stack should be called at the suspend path while disabling CPU.
>> 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).
> The affinity setting should not happen since DWC MSI controller doesn't support
> setting IRQ affinity (hierarchial IRQ domain). In the migrate_one_irq()
> function, there is a check for the existence of the irq_set_affinity()
> callback, but the DWC MSI controller return -EINVAL in the callback. So this
> is the reason the migration was still atempted?
>
> A quick check would be to test this suspend/resume with GIC ITS for MSI since
> it supports settings IRQ affinity and resides in a separate domain.
> Chaitanya, can you try that?

Hi mani,

I tried with gic its there also I see same behavior.

The only which helps to comment out affinity in the following function.

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 21b3ac2a29d2..042afec1cf9d 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -487,8 +487,9 @@ static int alloc_descs(unsigned int start, unsigned 
int cnt, int node,



                if (affinity) {
                         if (affinity->is_managed) {
-                               flags = IRQD_AFFINITY_MANAGED |
-                                       IRQD_MANAGED_SHUTDOWN;
+//                             flags = IRQD_AFFINITY_MANAGED |
+//                                     IRQD_MANAGED_SHUTDOWN;
+                               flags = 0;//IRQD_AFFINITY_MANAGED |
                         }
                         mask = &affinity->mask;
                         node = cpu_to_node(cpumask_first(mask));

>>> 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.
> Wake up should be handled by a dedicated side-band GPIO or in-band PME message.
>
> But I still wonder how the link stays in L1/L1ss when the clocks are disabled
> and PHY is powered down. Maybe the link or phy is powered by a separate power
> domain like MX that keeps the link active?
We will come back to you on this.
>
> Thanks,
> Mani
>

Powered by blists - more mailing lists