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: 3777 bytes --]

On Tue, 3 Jun 2025, Ilpo Järvinen wrote:

> 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 :-/).

Or maybe DT, I'm not very familiar with DT things.

--
 i.

> 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
> > > 
> > 
> 
>