Re: [PATCH v2 1/1] PCI: brcmstb: Fix regression regarding missing PCIe linkup

[Stefan Wahren <stefan.wahren@i2se.com>]

Hi Cyril,

Am 29.05.22 um 03:15 schrieb Cyril Brulebois:
> Hi Jim,
>
> Jim Quinlan <jim2101024@gmail.com> (2022-05-28):
>> commit 93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators")
>>
>> introduced a regression on the PCIe RPi4 Compute Module.  If the PCIe
>> root port DT node described in [2] was missing, no linkup would be attempted,
>> and subsequent accesses would cause a panic because this particular PCIe HW
>> causes a CPU abort on illegal accesses (instead of returning 0xffffffff).
>>
>> We fix this by allowing the DT root port node to be missing, as it behaved
>> before the original patchset messed things up.
>>
>> In addition, two small changes are made:
>>
>>    1. Having pci_subdev_regulators_remove_bus() call
>>       regulator_bulk_free() in addtion to regulator_bulk_disable().
>>    2. Having brcm_pcie_add_bus() return 0 if there is an
>>       error in calling pci_subdev_regulators_add_bus().
>>       Instead, we dev_err() and turn on our refusal mode instead.
>>
>> It would be best if this commit were tested by someone with a Rpi CM4
>> platform, as that is how the regression was found.  I have only emulated
>> the problem and fix on different platform.
> Testing is less flawless than it was with the earlier version, but this
> might be related to the fact master has moved a lot since then (from
> v5.18-rcX to open merge window).
>
> Overall, it's still a net win over the status quo (broken boot).
>
>
> Applying your patch on 664a393a2663a0f62fc1b18157ccae33dcdbb8c8 and
> performing cold boots is mostly fine:
>   - without anything on the PCIe slot;
>   - with a PCIe→quad-USB extension board, a USB keyboard and a USB stick
>     (both work fine).
>
> However, with an empty PCIe slot, I'm no longer able to perform the
> following (which was rock solid, and has been used in all my testing up
> to now):
>   - boot the exact same Debian stable image as before (running v5.10.y if
>     that matters);
>   - deploy the patched kernel;
>   - enable serial console;
>   - reboot into the patched kernel.
>
> PCI-related messages, a call trace, and broken storage:
>
>      [    3.425331] brcm-pcie fd500000.pcie: host bridge /scb/pcie@7d500000 ranges:
>      [    3.425353] brcm-pcie fd500000.pcie:   No bus range found for /scb/pcie@7d500000, using [bus 00-ff]
>      [    3.425388] brcm-pcie fd500000.pcie:      MEM 0x0600000000..0x0603ffffff -> 0x00f8000000
>      [    3.425420] brcm-pcie fd500000.pcie:   IB MEM 0x0000000000..0x003fffffff -> 0x0400000000
>      [    3.426229] brcm-pcie fd500000.pcie: PCI host bridge to bus 0000:00
>      [    3.426243] pci_bus 0000:00: root bus resource [bus 00-ff]
>      [    3.426255] pci_bus 0000:00: root bus resource [mem 0x600000000-0x603ffffff] (bus address [0xf8000000-0xfbffffff])
>      [    3.426303] pci 0000:00:00.0: [14e4:2711] type 01 class 0x060400
>      [    3.426398] pci 0000:00:00.0: PME# supported from D0 D3hot
>      [    3.428797] pci 0000:00:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring
>      [    3.745909] brcm-pcie fd500000.pcie: link down
>      [    3.747915] pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
>      [    3.747944] pci 0000:00:00.0: PCI bridge to [bus 01]
>      [    3.748294] pcieport 0000:00:00.0: PME: Signaling with IRQ 23
>      [    3.748691] pcieport 0000:00:00.0: AER: enabled with IRQ 23
>      [    3.749201] pci_bus 0000:01: busn_res: [bus 01] is released
>      [    3.749462] pci_bus 0000:00: busn_res: [bus 00-ff] is released
>      …
>      [    5.617308] irq 35: nobody cared (try booting with the "irqpoll" option)
>      [    5.617335] CPU: 0 PID: 127 Comm: systemd-udevd Not tainted 5.18.0+ #1
>      [    5.617350] Hardware name: Raspberry Pi Compute Module 4 Rev 1.0 (DT)
>      [    5.617358] Call trace:
>      [    5.617362]  dump_backtrace+0xc0/0x130
>      [    5.617386]  show_stack+0x24/0x70
>      [    5.617396]  dump_stack_lvl+0x68/0x84
>      [    5.617415]  dump_stack+0x18/0x34
>      [    5.617426]  __report_bad_irq+0x54/0x16c
>      [    5.617436]  note_interrupt+0x324/0x41c
>      [    5.617445]  handle_irq_event+0xc0/0x180
>      [    5.617460]  handle_fasteoi_irq+0xc8/0x1fc
>      [    5.617468]  generic_handle_domain_irq+0x38/0x50
>      [    5.617481]  gic_handle_irq+0x68/0xa0
>      [    5.617489]  call_on_irq_stack+0x2c/0x60
>      [    5.617500]  do_interrupt_handler+0x88/0x90
>      [    5.617511]  el0_interrupt+0x58/0x124
>      [    5.617526]  __el0_irq_handler_common+0x18/0x2c
>      [    5.617538]  el0t_64_irq_handler+0x10/0x20
>      [    5.617549]  el0t_64_irq+0x18c/0x190
>      [    5.617558] handlers:
>      [    5.617563] [<(____ptrval____)>] sdhci_irq [sdhci] threaded [<(____ptrval____)>] sdhci_thread_irq [sdhci]
>      [    5.617613] Disabling IRQ #35
>      …
>      [   15.581894] mmc0: Timeout waiting for hardware cmd interrupt.
>      [   15.581914] mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
>      [   15.581920] mmc0: sdhci: Sys addr:  0x00000000 | Version:  0x00001002
>      [   15.581931] mmc0: sdhci: Blk size:  0x00000000 | Blk cnt:  0x00000000
>      [   15.581937] mmc0: sdhci: Argument:  0x00000c00 | Trn mode: 0x00000000
>      [   15.581944] mmc0: sdhci: Present:   0x1fff0000 | Host ctl: 0x00000001
>      [   15.581951] mmc0: sdhci: Power:     0x0000000f | Blk gap:  0x00000080
>      [   15.581957] mmc0: sdhci: Wake-up:   0x00000000 | Clock:    0x00007d07
>      [   15.581964] mmc0: sdhci: Timeout:   0x00000000 | Int stat: 0x00018000
>      [   15.581971] mmc0: sdhci: Int enab:  0x00ff1003 | Sig enab: 0x00ff1003
>      [   15.581976] mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000001
>      [   15.581982] mmc0: sdhci: Caps:      0x45ee6432 | Caps_1:   0x0000a525
>      [   15.581988] mmc0: sdhci: Cmd:       0x0000341a | Max curr: 0x00080008
>      [   15.581996] mmc0: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x00000000
>      [   15.582001] mmc0: sdhci: Resp[2]:   0x00000000 | Resp[3]:  0x00000000
>      [   15.582005] mmc0: sdhci: Host ctl2: 0x00000000
>      [   15.582011] mmc0: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0x00000000
>      [   15.582016] mmc0: sdhci: ============================================
>
> This last part gets repeated over and over, and storage (external SD
> card) never comes up. I can share fuller logs if that's desirable. I can
> also test booting with irqpoll if that's desirable. Or anything else that
> might help.
>
>
> I did check that applying the same patch on top of the v5.18 tag gives
> good results (cold boots and reboots are fine, with or without an empty
> PCIe slot, as that was the case during earlier test sessions), so I'd
> guess something changed since then, and makes reboots more brittle than
> they used to be.

i think we should better trust the results based on the v5.18 tag. 
During the merge window, regressions from other subsystems are possible.

Best regards

>
> I can also check applying the v1 patch on top of master and compare
> results, to give a different perspective.
>
> But I'd also be happy to get some directions as to which test(s) would
> be most beneficial, which would help me cut down on combinatorics.
>
>
> Cheers,
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel