Re: [PATCH 24/25] PCI: Perform reset_resource() and build fail list in sync

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3512 bytes --]

On Tue, 3 Jun 2025, Tudor Ambarus wrote:
> On 6/3/25 3:13 PM, Ilpo Järvinen wrote:
> > On Tue, 3 Jun 2025, Tudor Ambarus wrote:
> >> On 6/3/25 9:13 AM, Ilpo Järvinen wrote:
> >>> So please test if this patch solves your problem:
> >>
> >> It fails in a different way, the bridge window resource never gets
> >> assigned with the proposed patch.
> > 
> > Is that a failure? I was expecting that to occur. It didn't assign 
> > any resources into that bridge window.
> 
> It leads to a watchdog interrupt on my pixel6. Last print I see on my
> console is related to the modem booting status. My wild guess is that
> that modem accesses something from the unassigned bridge window.

The bridge window is not for the bridge device itself. It's as the name 
says, a window into where subordinate busses can assign their resources.
The bridge knows it must forward that window address range to the 
subordinate bus.

> In the working case I see the bridge window printed:
> [   15.457310][ T1083] pcieport 0000:00:00.0: [s51xx_pcie_probe] BAR 14:
> tmp rsc : [mem 0x40000000-0x401fffff]
> 
> [   15.457683][ T1083] cpif: s51xx_pcie_probe: Set Doorbell register
> address.
> 
> In the failing case I see:
> [   15.623270][ T1113] pcieport 0000:00:00.0: [s51xx_pcie_probe] BAR 14:
> tmp rsc : [??? 0x00000000 flags 0x0]
> 
> [   15.623638][ T1113] cpif: s51xx_pcie_probe: Set Doorbell register
> address.

Oh, is it this one?

https://github.com/oberdfr/google-modules_radio_samsung_s5300/blob/11a10f955a267a45a1997f65671d7054adf1a33a/s51xx_pcie.c#L366

There are number of crazy things going on there... Probe shouldn't be 
messing resources like that. If it wants to change resources, a quirk 
would be more appropriate place I guess but I'm very unsure what that 
even tries to achieve with all that craziness ("Disable BAR resources" by 
assigning them :-/).

But yes, it seems to take the bridge window's address and assumes 
something is there (which isn't there as we know). So this driver code is 
plain wrong.

Perhaps it would want to use the address of some endpoint device resource 
instead of the bridge window address (e.g., that device with class 0?).

> > If there's nothing to be assigned into the bridge window, the bridge 
> > window itself is not created, that is the expected behavior (working as 
> > designed). So you're comparing to the bridge window that was made too 
> > large due to the disparity (and left unused, AFAICT).
> > 
> > It would be possible to put the condition inside the block which adds 
> > the resource to the realloc_head, I initially put it there but then 
> > decided to remove the disparity completely because why keep it if no 
> > resource is going to be placed into the bridge window.
> > 
> Thanks for the educative answers.
> 
> > What's that class 0 device anyway? Why it has class 0?
> >
> I don't know yet, it's the first time I'm dealing with a PCI driver. Any
> idea where is the class typically assigned?

https://pcisig.com/sites/default/files/files/PCI_Code-ID_r_1_12__v9_Jan_2020.pdf

Perhaps try a quirk which changes the class of the device underneath the 
bridge to something else than 0, it should make the resource fitting and 
allocation to assign its resources.

But honestly, that s51xx_pcie_probe() has more than one thing wrong.

> >> With the patch applied: https://termbin.com/h3w0
> >> With the blamed commit reverted: https://termbin.com/3rh6
> > 
> 

-- 
 i.