[REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")

[REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")

[Ville Syrjälä]

Hi,

The pci sysfs "rom" file has disappeared for VGA devices.
Looks to be a regression from commit 527139d738d7 ("PCI/sysfs:
Convert "rom" to static attribute").

Some kind of ordering issue between the sysfs file creation 
vs. pci_fixup_video() perhaps?

-- 
Ville Syrjälä
Intel

Re: [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")

[Bjorn Helgaas]

Hi Ville,

Thanks for the report!

On Fri, Dec 17, 2021 at 12:44:51PM +0200, Ville Syrjälä wrote:
> Hi,
> 
> The pci sysfs "rom" file has disappeared for VGA devices.
> Looks to be a regression from commit 527139d738d7 ("PCI/sysfs:
> Convert "rom" to static attribute").
> 
> Some kind of ordering issue between the sysfs file creation 
> vs. pci_fixup_video() perhaps?

Can you attach your complete "lspci -vv" output?  Also, which is the
default device?  I think there's a "boot_vga" sysfs file that shows
this.  "find /sys -name boot_vga | xargs grep ."

I think the relevant path is something like this:

  acpi_pci_root_add
    pci_acpi_scan_root
      ...
        pci_scan_single_device
          pci_device_add
            device_add
              ...
                sysfs_create_groups
                  ...
                    if (grp->is_visible())
                      pci_dev_rom_attr_is_visible  # after 527139d738d7
                        if (pci_resource_len(pdev, PCI_ROM_RESOURCE))
                          ...
    pci_bus_add_devices
      pci_bus_add_device
        pci_fixup_device(pci_fixup_final)
          pci_fixup_video
            if (vga_default_device() ...)
              # update PCI_ROM_RESOURCE
        pci_create_sysfs_dev_files
          if (pci_resource_len(pdev, PCI_ROM_RESOURCE))
            sysfs_create_bin_file("rom")           # before 527139d738d7

Prior to 527139d738d7, we ran pci_fixup_video() in
pci_bus_add_devices().  The vga_default_device() there might depend on
the fact that we've discovered all the PCI devices.  

After 527139d738d7, we create the "rom" file in pci_device_add(),
which happens as we discover each device, so maybe we don't yet know
which device is the default VGA device.

Bjorn

Re: [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")

[Ville Syrjälä]

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

On Fri, Dec 17, 2021 at 11:29:28AM -0600, Bjorn Helgaas wrote:
> Hi Ville,
> 
> Thanks for the report!
> 
> On Fri, Dec 17, 2021 at 12:44:51PM +0200, Ville Syrjälä wrote:
> > Hi,
> > 
> > The pci sysfs "rom" file has disappeared for VGA devices.
> > Looks to be a regression from commit 527139d738d7 ("PCI/sysfs:
> > Convert "rom" to static attribute").
> > 
> > Some kind of ordering issue between the sysfs file creation 
> > vs. pci_fixup_video() perhaps?
> 
> Can you attach your complete "lspci -vv" output?  Also, which is the
> default device?  I think there's a "boot_vga" sysfs file that shows
> this.  "find /sys -name boot_vga | xargs grep ."

All I have is Intel iGPUs so it's always 00:02.0. 

$ cat /sys/bus/pci/devices/0000\:00\:02.0/boot_vga 
1
$ cat /sys/bus/pci/devices/0000\:00\:02.0/rom
cat: '/sys/bus/pci/devices/0000:00:02.0/rom': No such file or directory

I've attached the full lspci from my IVB laptop, but the problem
happens on every machine (with an iGPU at least).

I presume with a discrete GPU it might not happen since they
actually have a real ROM.

> 
> I think the relevant path is something like this:
> 
>   acpi_pci_root_add
>     pci_acpi_scan_root
>       ...
>         pci_scan_single_device
>           pci_device_add
>             device_add
>               ...
>                 sysfs_create_groups
>                   ...
>                     if (grp->is_visible())
>                       pci_dev_rom_attr_is_visible  # after 527139d738d7
>                         if (pci_resource_len(pdev, PCI_ROM_RESOURCE))
>                           ...
>     pci_bus_add_devices
>       pci_bus_add_device
>         pci_fixup_device(pci_fixup_final)
>           pci_fixup_video
>             if (vga_default_device() ...)
>               # update PCI_ROM_RESOURCE
>         pci_create_sysfs_dev_files
>           if (pci_resource_len(pdev, PCI_ROM_RESOURCE))
>             sysfs_create_bin_file("rom")           # before 527139d738d7
> 
> Prior to 527139d738d7, we ran pci_fixup_video() in
> pci_bus_add_devices().  The vga_default_device() there might depend on
> the fact that we've discovered all the PCI devices.  
> 
> After 527139d738d7, we create the "rom" file in pci_device_add(),
> which happens as we discover each device, so maybe we don't yet know
> which device is the default VGA device.
> 
> Bjorn

-- 
Ville Syrjälä
Intel

[-- Attachment #2: lspci.txt --]
[-- Type: text/plain, Size: 20793 bytes --]

00:00.0 Host bridge: Intel Corporation 3rd Gen Core processor DRAM Controller (rev 09)
	Subsystem: Lenovo 3rd Gen Core processor DRAM Controller
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ >SERR- <PERR- INTx-
	Latency: 0
	Capabilities: [e0] Vendor Specific Information: Len=0c <?>
	Kernel driver in use: ivb_uncore

00:02.0 VGA compatible controller: Intel Corporation 3rd Gen Core processor Graphics Controller (rev 09) (prog-if 00 [VGA controller])
	Subsystem: Lenovo 3rd Gen Core processor Graphics Controller
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 27
	Region 0: Memory at f0000000 (64-bit, non-prefetchable) [size=4M]
	Region 2: Memory at e0000000 (64-bit, prefetchable) [size=256M]
	Region 4: I/O ports at 4000 [size=64]
	Expansion ROM at 000c0000 [virtual] [disabled] [size=128K]
	Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit-
		Address: fee01004  Data: 002b
	Capabilities: [d0] Power Management version 2
		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [a4] PCI Advanced Features
		AFCap: TP+ FLR+
		AFCtrl: FLR-
		AFStatus: TP-
	Kernel driver in use: i915
	Kernel modules: i915

00:14.0 USB controller: Intel Corporation 7 Series/C210 Series Chipset Family USB xHCI Host Controller (rev 04) (prog-if 30 [XHCI])
	Subsystem: Lenovo 7 Series/C210 Series Chipset Family USB xHCI Host Controller
	Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Interrupt: pin A routed to IRQ 25
	Region 0: Memory at f1500000 (64-bit, non-prefetchable) [size=64K]
	Capabilities: [70] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA PME(D0-,D1-,D2-,D3hot+,D3cold+)
		Status: D3 NoSoftRst+ PME-Enable+ DSel=0 DScale=0 PME-
	Capabilities: [80] MSI: Enable+ Count=1/8 Maskable- 64bit+
		Address: 00000000fee01004  Data: 0029
	Kernel driver in use: xhci_hcd
	Kernel modules: xhci_pci

00:16.0 Communication controller: Intel Corporation 7 Series/C216 Chipset Family MEI Controller #1 (rev 04)
	Subsystem: Lenovo 7 Series/C216 Chipset Family MEI Controller
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx+
	Latency: 0
	Interrupt: pin A routed to IRQ 255
	Region 0: Memory at f1515000 (64-bit, non-prefetchable) [size=16]
	Capabilities: [50] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst+ PME-Enable+ DSel=0 DScale=0 PME-
	Capabilities: [8c] MSI: Enable- Count=1/1 Maskable- 64bit+
		Address: 0000000000000000  Data: 0000

00:1a.0 USB controller: Intel Corporation 7 Series/C216 Chipset Family USB Enhanced Host Controller #2 (rev 04) (prog-if 20 [EHCI])
	Subsystem: Lenovo 7 Series/C216 Chipset Family USB Enhanced Host Controller
	Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Interrupt: pin A routed to IRQ 16
	Region 0: Memory at f151a000 (32-bit, non-prefetchable) [size=1K]
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
	Capabilities: [58] Debug port: BAR=1 offset=00a0
	Capabilities: [98] PCI Advanced Features
		AFCap: TP+ FLR+
		AFCtrl: FLR-
		AFStatus: TP-
	Kernel driver in use: ehci-pci
	Kernel modules: ehci_pci

00:1b.0 Audio device: Intel Corporation 7 Series/C216 Chipset Family High Definition Audio Controller (rev 04)
	Subsystem: Lenovo 7 Series/C216 Chipset Family High Definition Audio Controller
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 28
	Region 0: Memory at f1510000 (64-bit, non-prefetchable) [size=16K]
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [60] MSI: Enable+ Count=1/1 Maskable- 64bit+
		Address: 00000000fee02004  Data: 0021
	Capabilities: [70] Express (v1) Root Complex Integrated Endpoint, MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0
			ExtTag- RBE- FLReset+
		DevCtl:	CorrErr- NonFatalErr- FatalErr- UnsupReq-
			RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop- FLReset-
			MaxPayload 128 bytes, MaxReadReq 128 bytes
		DevSta:	CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend-
	Capabilities: [100 v1] Virtual Channel
		Caps:	LPEVC=0 RefClk=100ns PATEntryBits=1
		Arb:	Fixed- WRR32- WRR64- WRR128-
		Ctrl:	ArbSelect=Fixed
		Status:	InProgress-
		VC0:	Caps:	PATOffset=00 MaxTimeSlots=1 RejSnoopTrans-
			Arb:	Fixed- WRR32- WRR64- WRR128- TWRR128- WRR256-
			Ctrl:	Enable+ ID=0 ArbSelect=Fixed TC/VC=01
			Status:	NegoPending- InProgress-
		VC1:	Caps:	PATOffset=00 MaxTimeSlots=1 RejSnoopTrans-
			Arb:	Fixed- WRR32- WRR64- WRR128- TWRR128- WRR256-
			Ctrl:	Enable+ ID=1 ArbSelect=Fixed TC/VC=22
			Status:	NegoPending- InProgress-
	Capabilities: [130 v1] Root Complex Link
		Desc:	PortNumber=0f ComponentID=00 EltType=Config
		Link0:	Desc:	TargetPort=00 TargetComponent=00 AssocRCRB- LinkType=MemMapped LinkValid+
			Addr:	00000000fed1c000
	Kernel driver in use: snd_hda_intel
	Kernel modules: snd_hda_intel

00:1c.0 PCI bridge: Intel Corporation 7 Series/C216 Chipset Family PCI Express Root Port 1 (rev c4) (prog-if 00 [Normal decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 16
	Bus: primary=00, secondary=02, subordinate=02, sec-latency=0
	I/O behind bridge: 00003000-00003fff [size=4K]
	Memory behind bridge: f0d00000-f14fffff [size=8M]
	Prefetchable memory behind bridge: 00000000f0400000-00000000f0bfffff [size=8M]
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
	BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [40] Express (v2) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0
			ExtTag- RBE+
		DevCtl:	CorrErr- NonFatalErr- FatalErr- UnsupReq-
			RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
			MaxPayload 128 bytes, MaxReadReq 128 bytes
		DevSta:	CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend-
		LnkCap:	Port #1, Speed 5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <512ns, L1 <16us
			ClockPM- Surprise- LLActRep+ BwNot- ASPMOptComp-
		LnkCtl:	ASPM L1 Enabled; RCB 64 bytes, Disabled- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s (downgraded), Width x1 (ok)
			TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt-
		SltCap:	AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise+
			Slot #0, PowerLimit 10.000W; Interlock- NoCompl+
		SltCtl:	Enable: AttnBtn- PwrFlt- MRL- PresDet+ CmdCplt- HPIrq- LinkChg-
			Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
		SltSta:	Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock-
			Changed: MRL- PresDet- LinkState-
		RootCap: CRSVisible-
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible-
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
		DevCap2: Completion Timeout: Range BC, TimeoutDis+ NROPrPrP- LTR-
			 10BitTagComp- 10BitTagReq- OBFF Not Supported, ExtFmt- EETLPPrefix-
			 EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit-
			 FRS- LN System CLS Not Supported, TPHComp- ExtTPHComp- ARIFwd-
			 AtomicOpsCap: Routing- 32bit- 64bit- 128bitCAS-
		DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR- OBFF Disabled, ARIFwd-
			 AtomicOpsCtl: ReqEn- EgressBlck-
		LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis-
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
			 Compliance De-emphasis: -6dB
		LnkSta2: Current De-emphasis Level: -3.5dB, EqualizationComplete- EqualizationPhase1-
			 EqualizationPhase2- EqualizationPhase3- LinkEqualizationRequest-
			 Retimer- 2Retimers- CrosslinkRes: unsupported
	Capabilities: [80] MSI: Enable- Count=1/1 Maskable- 64bit-
		Address: 00000000  Data: 0000
	Capabilities: [90] Subsystem: Lenovo 7 Series/C216 Chipset Family PCI Express Root Port 1
	Capabilities: [a0] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: pcieport

00:1c.1 PCI bridge: Intel Corporation 7 Series/C210 Series Chipset Family PCI Express Root Port 2 (rev c4) (prog-if 00 [Normal decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Interrupt: pin B routed to IRQ 17
	Bus: primary=00, secondary=03, subordinate=03, sec-latency=0
	I/O behind bridge: [disabled]
	Memory behind bridge: f0c00000-f0cfffff [size=1M]
	Prefetchable memory behind bridge: [disabled]
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
	BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [40] Express (v2) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0
			ExtTag- RBE+
		DevCtl:	CorrErr- NonFatalErr- FatalErr- UnsupReq-
			RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
			MaxPayload 128 bytes, MaxReadReq 128 bytes
		DevSta:	CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend-
		LnkCap:	Port #2, Speed 5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <512ns, L1 <16us
			ClockPM- Surprise- LLActRep+ BwNot- ASPMOptComp-
		LnkCtl:	ASPM L1 Enabled; RCB 64 bytes, Disabled- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s (downgraded), Width x1 (ok)
			TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt-
		SltCap:	AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug- Surprise-
			Slot #1, PowerLimit 10.000W; Interlock- NoCompl+
		SltCtl:	Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg-
			Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
		SltSta:	Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock-
			Changed: MRL- PresDet- LinkState+
		RootCap: CRSVisible-
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible-
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
		DevCap2: Completion Timeout: Range BC, TimeoutDis+ NROPrPrP- LTR-
			 10BitTagComp- 10BitTagReq- OBFF Not Supported, ExtFmt- EETLPPrefix-
			 EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit-
			 FRS- LN System CLS Not Supported, TPHComp- ExtTPHComp- ARIFwd-
			 AtomicOpsCap: Routing- 32bit- 64bit- 128bitCAS-
		DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR- OBFF Disabled, ARIFwd-
			 AtomicOpsCtl: ReqEn- EgressBlck-
		LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis-
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
			 Compliance De-emphasis: -6dB
		LnkSta2: Current De-emphasis Level: -3.5dB, EqualizationComplete- EqualizationPhase1-
			 EqualizationPhase2- EqualizationPhase3- LinkEqualizationRequest-
			 Retimer- 2Retimers- CrosslinkRes: unsupported
	Capabilities: [80] MSI: Enable- Count=1/1 Maskable- 64bit-
		Address: 00000000  Data: 0000
	Capabilities: [90] Subsystem: Lenovo 7 Series/C210 Series Chipset Family PCI Express Root Port 2
	Capabilities: [a0] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: pcieport

00:1d.0 USB controller: Intel Corporation 7 Series/C216 Chipset Family USB Enhanced Host Controller #1 (rev 04) (prog-if 20 [EHCI])
	Subsystem: Lenovo 7 Series/C216 Chipset Family USB Enhanced Host Controller
	Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Interrupt: pin A routed to IRQ 23
	Region 0: Memory at f1519000 (32-bit, non-prefetchable) [size=1K]
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D3 NoSoftRst- PME-Enable+ DSel=0 DScale=0 PME-
	Capabilities: [58] Debug port: BAR=1 offset=00a0
	Capabilities: [98] PCI Advanced Features
		AFCap: TP+ FLR+
		AFCtrl: FLR-
		AFStatus: TP-
	Kernel driver in use: ehci-pci
	Kernel modules: ehci_pci

00:1f.0 ISA bridge: Intel Corporation QS77 Express Chipset LPC Controller (rev 04)
	Subsystem: Lenovo QS77 Express Chipset LPC Controller
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Capabilities: [e0] Vendor Specific Information: Len=0c <?>
	Kernel driver in use: lpc_ich
	Kernel modules: lpc_ich

00:1f.2 SATA controller: Intel Corporation 7 Series Chipset Family 6-port SATA Controller [AHCI mode] (rev 04) (prog-if 01 [AHCI 1.0])
	Subsystem: Lenovo 7 Series Chipset Family 6-port SATA Controller [AHCI mode]
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin B routed to IRQ 24
	Region 0: I/O ports at 4088 [size=8]
	Region 1: I/O ports at 4094 [size=4]
	Region 2: I/O ports at 4080 [size=8]
	Region 3: I/O ports at 4090 [size=4]
	Region 4: I/O ports at 4060 [size=32]
	Region 5: Memory at f1518000 (32-bit, non-prefetchable) [size=2K]
	Capabilities: [80] MSI: Enable+ Count=1/1 Maskable- 64bit-
		Address: fee01004  Data: 0025
	Capabilities: [70] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot+,D3cold-)
		Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [a8] SATA HBA v1.0 BAR4 Offset=00000004
	Capabilities: [b0] PCI Advanced Features
		AFCap: TP+ FLR+
		AFCtrl: FLR-
		AFStatus: TP-
	Kernel driver in use: ahci

00:1f.3 SMBus: Intel Corporation 7 Series/C216 Chipset Family SMBus Controller (rev 04)
	Subsystem: Lenovo 7 Series/C216 Chipset Family SMBus Controller
	Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Interrupt: pin C routed to IRQ 18
	Region 0: Memory at f1514000 (64-bit, non-prefetchable) [size=256]
	Region 4: I/O ports at efa0 [size=32]
	Kernel driver in use: i801_smbus
	Kernel modules: i2c_i801

02:00.0 System peripheral: Ricoh Co Ltd MMC/SD Host Controller (rev 07) (prog-if 01)
	Subsystem: Lenovo MMC/SD Host Controller
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 16
	Region 0: Memory at f0d00000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+
		Address: 0000000000000000  Data: 0000
	Capabilities: [78] Power Management version 3
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=2 PME-
	Capabilities: [80] Express (v1) Endpoint, MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0, Latency L0s unlimited, L1 unlimited
			ExtTag- AttnBtn+ AttnInd+ PwrInd+ RBE+ FLReset- SlotPowerLimit 10.000W
		DevCtl:	CorrErr- NonFatalErr- FatalErr- UnsupReq-
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr+ NonFatalErr- FatalErr- UnsupReq+ AuxPwr- TransPend-
		LnkCap:	Port #1, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <4us, L1 unlimited
			ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp-
		LnkCtl:	ASPM L1 Enabled; RCB 64 bytes, Disabled- CommClk+
			ExtSynch- ClockPM+ AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s (ok), Width x1 (ok)
			TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
	Capabilities: [100 v1] Virtual Channel
		Caps:	LPEVC=0 RefClk=100ns PATEntryBits=1
		Arb:	Fixed- WRR32- WRR64- WRR128-
		Ctrl:	ArbSelect=Fixed
		Status:	InProgress-
		VC0:	Caps:	PATOffset=00 MaxTimeSlots=1 RejSnoopTrans-
			Arb:	Fixed- WRR32- WRR64- WRR128- TWRR128- WRR256-
			Ctrl:	Enable+ ID=0 ArbSelect=Fixed TC/VC=ff
			Status:	NegoPending- InProgress-
	Capabilities: [800 v1] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr+ BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		AERCap:	First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn-
			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
		HeaderLog: 00000000 00000000 00000000 00000000
	Kernel driver in use: sdhci-pci
	Kernel modules: sdhci_pci

03:00.0 Network controller: Intel Corporation Centrino Advanced-N 6205 [Taylor Peak] (rev 96)
	Subsystem: Intel Corporation Centrino Advanced-N 6205 [Taylor Peak]
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 26
	Region 0: Memory at f0c00000 (64-bit, non-prefetchable) [size=8K]
	Capabilities: [c8] Power Management version 3
		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [d0] MSI: Enable+ Count=1/1 Maskable- 64bit+
		Address: 00000000fee01004  Data: 002a
	Capabilities: [e0] Express (v1) Endpoint, MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0, Latency L0s <512ns, L1 unlimited
			ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset+ SlotPowerLimit 0.000W
		DevCtl:	CorrErr- NonFatalErr- FatalErr- UnsupReq-
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+ FLReset-
			MaxPayload 128 bytes, MaxReadReq 128 bytes
		DevSta:	CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend-
		LnkCap:	Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <4us, L1 <32us
			ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp-
		LnkCtl:	ASPM L1 Enabled; RCB 64 bytes, Disabled- CommClk+
			ExtSynch- ClockPM+ AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s (ok), Width x1 (ok)
			TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
	Capabilities: [100 v1] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES- TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr-
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		AERCap:	First Error Pointer: 00, ECRCGenCap- ECRCGenEn- ECRCChkCap- ECRCChkEn-
			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
		HeaderLog: 00000000 00000000 00000000 00000000
	Capabilities: [140 v1] Device Serial Number 84-3a-4b-ff-ff-46-a9-10
	Kernel driver in use: iwlwifi
	Kernel modules: iwlwifi

Re: [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")

[Krzysztof Wilczyński]

Hi Ville,

Thank you for letting us know, and sincere apologies for troubles!

[...]
> > > The pci sysfs "rom" file has disappeared for VGA devices.
> > > Looks to be a regression from commit 527139d738d7 ("PCI/sysfs:
> > > Convert "rom" to static attribute").
> > > 
> > > Some kind of ordering issue between the sysfs file creation 
> > > vs. pci_fixup_video() perhaps?
> > 
> > Can you attach your complete "lspci -vv" output?  Also, which is the
> > default device?  I think there's a "boot_vga" sysfs file that shows
> > this.  "find /sys -name boot_vga | xargs grep ."
> 
> All I have is Intel iGPUs so it's always 00:02.0. 
> 
> $ cat /sys/bus/pci/devices/0000\:00\:02.0/boot_vga 
> 1
> $ cat /sys/bus/pci/devices/0000\:00\:02.0/rom
> cat: '/sys/bus/pci/devices/0000:00:02.0/rom': No such file or directory
> 
> I've attached the full lspci from my IVB laptop, but the problem
> happens on every machine (with an iGPU at least).
> 
> I presume with a discrete GPU it might not happen since they
> actually have a real ROM.

Admittedly, the automated testing I was running before the patch was released
didn't catch this.  I primarily focused on trying to catch the race condition
related to the ROM attribute creation.

I need to look into how to properly address this problem as if we were to
revert the ROM attribute changes, then we would introduce the race condition
we've had back.

Again, apologies for troubles this caused!

	Krzysztof

Re: [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")

[Thorsten Leemhuis]

On 17.12.21 23:49, Krzysztof Wilczyński wrote:
> Hi Ville,
> 
> Thank you for letting us know, and sincere apologies for troubles!
> 
> [...]
>>>> The pci sysfs "rom" file has disappeared for VGA devices.
>>>> Looks to be a regression from commit 527139d738d7 ("PCI/sysfs:
>>>> Convert "rom" to static attribute").
>>>>
>>>> Some kind of ordering issue between the sysfs file creation 
>>>> vs. pci_fixup_video() perhaps?
>>>
>>> Can you attach your complete "lspci -vv" output?  Also, which is the
>>> default device?  I think there's a "boot_vga" sysfs file that shows
>>> this.  "find /sys -name boot_vga | xargs grep ."
>>
>> All I have is Intel iGPUs so it's always 00:02.0. 
>>
>> $ cat /sys/bus/pci/devices/0000\:00\:02.0/boot_vga 
>> 1
>> $ cat /sys/bus/pci/devices/0000\:00\:02.0/rom
>> cat: '/sys/bus/pci/devices/0000:00:02.0/rom': No such file or directory
>>
>> I've attached the full lspci from my IVB laptop, but the problem
>> happens on every machine (with an iGPU at least).
>>
>> I presume with a discrete GPU it might not happen since they
>> actually have a real ROM.
> 
> Admittedly, the automated testing I was running before the patch was released
> didn't catch this.  I primarily focused on trying to catch the race condition
> related to the ROM attribute creation.
> 
> I need to look into how to properly address this problem as if we were to
> revert the ROM attribute changes, then we would introduce the race condition
> we've had back.
> 
> Again, apologies for troubles this caused!

What's the status of this regression and getting it fixed? It looks like
there was no progress for quite a while. Could anyone please provide a
status update?

Ciao, Thorsten (wearing his 'Linux kernel regression tracker' hat)

P.S.: As a Linux kernel regression tracker I'm getting a lot of reports
on my table. I can only look briefly into most of them. Unfortunately
therefore I sometimes will get things wrong or miss something important.
I hope that's not the case here; if you think it is, don't hesitate to
tell me about it in a public reply, that's in everyone's interest.

BTW, I have no personal interest in this issue, which is tracked using
regzbot, my Linux kernel regression tracking bot
(https://linux-regtracking.leemhuis.info/regzbot/). I'm only posting
this mail to get things rolling again and hence don't need to be CC on
all further activities wrt to this regression.

#regzbot poke

Re: [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")

[Bjorn Helgaas]

On Thu, Jan 20, 2022 at 02:19:05PM +0100, Thorsten Leemhuis wrote:
> On 17.12.21 23:49, Krzysztof Wilczyński wrote:
> > Hi Ville,
> > 
> > Thank you for letting us know, and sincere apologies for troubles!
> > 
> > [...]
> >>>> The pci sysfs "rom" file has disappeared for VGA devices.
> >>>> Looks to be a regression from commit 527139d738d7 ("PCI/sysfs:
> >>>> Convert "rom" to static attribute").
> >>>>
> >>>> Some kind of ordering issue between the sysfs file creation 
> >>>> vs. pci_fixup_video() perhaps?
> >>>
> >>> Can you attach your complete "lspci -vv" output?  Also, which is the
> >>> default device?  I think there's a "boot_vga" sysfs file that shows
> >>> this.  "find /sys -name boot_vga | xargs grep ."
> >>
> >> All I have is Intel iGPUs so it's always 00:02.0. 
> >>
> >> $ cat /sys/bus/pci/devices/0000\:00\:02.0/boot_vga 
> >> 1
> >> $ cat /sys/bus/pci/devices/0000\:00\:02.0/rom
> >> cat: '/sys/bus/pci/devices/0000:00:02.0/rom': No such file or directory
> >>
> >> I've attached the full lspci from my IVB laptop, but the problem
> >> happens on every machine (with an iGPU at least).
> >>
> >> I presume with a discrete GPU it might not happen since they
> >> actually have a real ROM.
> > 
> > Admittedly, the automated testing I was running before the patch was released
> > didn't catch this.  I primarily focused on trying to catch the race condition
> > related to the ROM attribute creation.
> > 
> > I need to look into how to properly address this problem as if we were to
> > revert the ROM attribute changes, then we would introduce the race condition
> > we've had back.
> > 
> > Again, apologies for troubles this caused!
> 
> What's the status of this regression and getting it fixed? It looks like
> there was no progress for quite a while. Could anyone please provide a
> status update?

What a coincidence.  Krzysztof and I chatted about this yesterday.  No
progress to report yet, but we are working on it.

Bjorn

Re: [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")

[Thorsten Leemhuis]

On 20.01.22 16:00, Bjorn Helgaas wrote:
> On Thu, Jan 20, 2022 at 02:19:05PM +0100, Thorsten Leemhuis wrote:
>> On 17.12.21 23:49, Krzysztof Wilczyński wrote:
>>> Hi Ville,
>>>
>>> Thank you for letting us know, and sincere apologies for troubles!
>>>
>>> [...]
>>>>>> The pci sysfs "rom" file has disappeared for VGA devices.
>>>>>> Looks to be a regression from commit 527139d738d7 ("PCI/sysfs:
>>>>>> Convert "rom" to static attribute").
>>>>>>
>>>>>> Some kind of ordering issue between the sysfs file creation 
>>>>>> vs. pci_fixup_video() perhaps?
>>>>>
>>>>> Can you attach your complete "lspci -vv" output?  Also, which is the
>>>>> default device?  I think there's a "boot_vga" sysfs file that shows
>>>>> this.  "find /sys -name boot_vga | xargs grep ."
>>>>
>>>> All I have is Intel iGPUs so it's always 00:02.0. 
>>>>
>>>> $ cat /sys/bus/pci/devices/0000\:00\:02.0/boot_vga 
>>>> 1
>>>> $ cat /sys/bus/pci/devices/0000\:00\:02.0/rom
>>>> cat: '/sys/bus/pci/devices/0000:00:02.0/rom': No such file or directory
>>>>
>>>> I've attached the full lspci from my IVB laptop, but the problem
>>>> happens on every machine (with an iGPU at least).
>>>>
>>>> I presume with a discrete GPU it might not happen since they
>>>> actually have a real ROM.
>>>
>>> Admittedly, the automated testing I was running before the patch was released
>>> didn't catch this.  I primarily focused on trying to catch the race condition
>>> related to the ROM attribute creation.
>>>
>>> I need to look into how to properly address this problem as if we were to
>>> revert the ROM attribute changes, then we would introduce the race condition
>>> we've had back.
>>>
>>> Again, apologies for troubles this caused!
>>
>> What's the status of this regression and getting it fixed? It looks like
>> there was no progress for quite a while. Could anyone please provide a
>> status update?
> 
> What a coincidence.  Krzysztof and I chatted about this yesterday.  No
> progress to report yet, but we are working on it.

Many thx for the update! Ciao, Thorsten

Re: [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")

[Bjorn Helgaas]

On Fri, Dec 17, 2021 at 07:45:44PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 17, 2021 at 11:29:28AM -0600, Bjorn Helgaas wrote:
> > On Fri, Dec 17, 2021 at 12:44:51PM +0200, Ville Syrjälä wrote:
> > > The pci sysfs "rom" file has disappeared for VGA devices.
> > > Looks to be a regression from commit 527139d738d7 ("PCI/sysfs:
> > > Convert "rom" to static attribute").
> > > 
> > > Some kind of ordering issue between the sysfs file creation 
> > > vs. pci_fixup_video() perhaps?
> > 
> > Can you attach your complete "lspci -vv" output?  Also, which is the
> > default device?  I think there's a "boot_vga" sysfs file that shows
> > this.  "find /sys -name boot_vga | xargs grep ."
> 
> All I have is Intel iGPUs so it's always 00:02.0. 
> 
> $ cat /sys/bus/pci/devices/0000\:00\:02.0/boot_vga 
> 1
> $ cat /sys/bus/pci/devices/0000\:00\:02.0/rom
> cat: '/sys/bus/pci/devices/0000:00:02.0/rom': No such file or directory
> 
> I've attached the full lspci from my IVB laptop, but the problem
> happens on every machine (with an iGPU at least).
> 
> I presume with a discrete GPU it might not happen since they
> actually have a real ROM.
> 
> > I think the relevant path is something like this:
> > 
> >   acpi_pci_root_add
> >     pci_acpi_scan_root
> >       ...
> >         pci_scan_single_device
> >           pci_device_add
> >             device_add
> >               ...
> >                 sysfs_create_groups
> >                   ...
> >                     if (grp->is_visible())
> >                       pci_dev_rom_attr_is_visible  # after 527139d738d7
> >                         if (pci_resource_len(pdev, PCI_ROM_RESOURCE))
> >                           ...
> >     pci_bus_add_devices
> >       pci_bus_add_device
> >         pci_fixup_device(pci_fixup_final)
> >           pci_fixup_video
> >             if (vga_default_device() ...)
> >               # update PCI_ROM_RESOURCE
> >         pci_create_sysfs_dev_files
> >           if (pci_resource_len(pdev, PCI_ROM_RESOURCE))
> >             sysfs_create_bin_file("rom")           # before 527139d738d7
> > 
> > Prior to 527139d738d7, we ran pci_fixup_video() in
> > pci_bus_add_devices().  The vga_default_device() there might depend on
> > the fact that we've discovered all the PCI devices.  
> > 
> > After 527139d738d7, we create the "rom" file in pci_device_add(),
> > which happens as we discover each device, so maybe we don't yet know
> > which device is the default VGA device.

Thanks, Ville!

I think this is happening because the iGPU has no ROM BAR,
so pci_resource_len(PCI_ROM_RESOURCE) is 0 when we run
pci_dev_rom_attr_is_visible().  Prior to 527139d738d7, we also
used pci_resource_len(PCI_ROM_RESOURCE), but pci_fixup_video() had
been run, and it filled in the ROM resource with the shadow ROM
start and size.

Can you collect the dmesg output with "pci=earlydump"?  I think
that will show zeros at PCI_ROM_ADDRESS for your 00:02.0 device.

And can you try the test patch below?  I reproduced the problem in
qemu by instrumenting to clear the VGA ROM BAR, and moving the
pci_fixup_video() quirk earlier makes it run before
pci_dev_rom_attr_is_visible(), which fixes the problem for me.

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 2edd86649468..615a76d70019 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -353,8 +353,8 @@ static void pci_fixup_video(struct pci_dev *pdev)
 		}
 	}
 }
-DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
-				PCI_CLASS_DISPLAY_VGA, 8, pci_fixup_video);
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID,
+			       PCI_CLASS_DISPLAY_VGA, 8, pci_fixup_video);
 
 
 static const struct dmi_system_id msi_k8t_dmi_table[] = {

Re: [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")

[Ville Syrjälä]

On Fri, Jan 21, 2022 at 06:24:45PM -0600, Bjorn Helgaas wrote:
> On Fri, Dec 17, 2021 at 07:45:44PM +0200, Ville Syrjälä wrote:
> > On Fri, Dec 17, 2021 at 11:29:28AM -0600, Bjorn Helgaas wrote:
> > > On Fri, Dec 17, 2021 at 12:44:51PM +0200, Ville Syrjälä wrote:
> > > > The pci sysfs "rom" file has disappeared for VGA devices.
> > > > Looks to be a regression from commit 527139d738d7 ("PCI/sysfs:
> > > > Convert "rom" to static attribute").
> > > > 
> > > > Some kind of ordering issue between the sysfs file creation 
> > > > vs. pci_fixup_video() perhaps?
> > > 
> > > Can you attach your complete "lspci -vv" output?  Also, which is the
> > > default device?  I think there's a "boot_vga" sysfs file that shows
> > > this.  "find /sys -name boot_vga | xargs grep ."
> > 
> > All I have is Intel iGPUs so it's always 00:02.0. 
> > 
> > $ cat /sys/bus/pci/devices/0000\:00\:02.0/boot_vga 
> > 1
> > $ cat /sys/bus/pci/devices/0000\:00\:02.0/rom
> > cat: '/sys/bus/pci/devices/0000:00:02.0/rom': No such file or directory
> > 
> > I've attached the full lspci from my IVB laptop, but the problem
> > happens on every machine (with an iGPU at least).
> > 
> > I presume with a discrete GPU it might not happen since they
> > actually have a real ROM.
> > 
> > > I think the relevant path is something like this:
> > > 
> > >   acpi_pci_root_add
> > >     pci_acpi_scan_root
> > >       ...
> > >         pci_scan_single_device
> > >           pci_device_add
> > >             device_add
> > >               ...
> > >                 sysfs_create_groups
> > >                   ...
> > >                     if (grp->is_visible())
> > >                       pci_dev_rom_attr_is_visible  # after 527139d738d7
> > >                         if (pci_resource_len(pdev, PCI_ROM_RESOURCE))
> > >                           ...
> > >     pci_bus_add_devices
> > >       pci_bus_add_device
> > >         pci_fixup_device(pci_fixup_final)
> > >           pci_fixup_video
> > >             if (vga_default_device() ...)
> > >               # update PCI_ROM_RESOURCE
> > >         pci_create_sysfs_dev_files
> > >           if (pci_resource_len(pdev, PCI_ROM_RESOURCE))
> > >             sysfs_create_bin_file("rom")           # before 527139d738d7
> > > 
> > > Prior to 527139d738d7, we ran pci_fixup_video() in
> > > pci_bus_add_devices().  The vga_default_device() there might depend on
> > > the fact that we've discovered all the PCI devices.  
> > > 
> > > After 527139d738d7, we create the "rom" file in pci_device_add(),
> > > which happens as we discover each device, so maybe we don't yet know
> > > which device is the default VGA device.
> 
> Thanks, Ville!
> 
> I think this is happening because the iGPU has no ROM BAR,
> so pci_resource_len(PCI_ROM_RESOURCE) is 0 when we run
> pci_dev_rom_attr_is_visible().  Prior to 527139d738d7, we also
> used pci_resource_len(PCI_ROM_RESOURCE), but pci_fixup_video() had
> been run, and it filled in the ROM resource with the shadow ROM
> start and size.
> 
> Can you collect the dmesg output with "pci=earlydump"?  I think
> that will show zeros at PCI_ROM_ADDRESS for your 00:02.0 device.

Yeah no real ROM on these:
 pci 0000:00:02.0: config space:
 00000000: 86 80 26 01 07 00 90 00 09 00 00 03 00 00 00 00
 00000010: 04 00 00 f0 00 00 00 00 0c 00 00 e0 00 00 00 00
 00000020: 01 50 00 00 00 00 00 00 00 00 00 00 aa 17 da 21
 00000030: 00 00 00 00 90 00 00 00 00 00 00 00 0b 01 00 00
 00000040: 09 00 0c 01 9e 61 00 e2 90 00 08 14 00 00 00 00
 00000050: 11 02 00 00 11 00 00 00 00 00 00 00 01 00 a0 db
 00000060: 00 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00
 00000070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 00000080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 00000090: 05 d0 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 000000a0: 00 00 00 00 13 00 06 03 00 00 00 00 00 00 00 00
 000000b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 000000c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 000000d0: 01 a4 22 00 00 00 00 00 00 00 00 00 00 00 00 00
 000000e0: 00 00 00 00 00 00 00 00 00 80 00 00 00 00 00 00
 000000f0: 00 00 00 00 00 00 00 00 00 00 06 00 18 60 ef da
 pci 0000:00:02.0: [8086:0126] type 00 class 0x030000
 pci 0000:00:02.0: reg 0x10: [mem 0xf0000000-0xf03fffff 64bit]
 pci 0000:00:02.0: reg 0x18: [mem 0xe0000000-0xefffffff 64bit pref]
 pci 0000:00:02.0: reg 0x20: [io  0x5000-0x503f]

> 
> And can you try the test patch below?  I reproduced the problem in
> qemu by instrumenting to clear the VGA ROM BAR, and moving the
> pci_fixup_video() quirk earlier makes it run before
> pci_dev_rom_attr_is_visible(), which fixes the problem for me.
> 
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 2edd86649468..615a76d70019 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -353,8 +353,8 @@ static void pci_fixup_video(struct pci_dev *pdev)
>  		}
>  	}
>  }
> -DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> -				PCI_CLASS_DISPLAY_VGA, 8, pci_fixup_video);
> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_ANY_ID, PCI_ANY_ID,
> +			       PCI_CLASS_DISPLAY_VGA, 8, pci_fixup_video);
>  
>  
>  static const struct dmi_system_id msi_k8t_dmi_table[] = {

pci=earlydump now reports:
 pci 0000:00:02.0: Video device with shadowed ROM at [mem 0x000c0000-0x000dffff]
and poking it via sysfs works fine.

Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

-- 
Ville Syrjälä
Intel

Re: [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")

[Thorsten Leemhuis]

On 17.12.21 11:44, Ville Syrjälä wrote:
> Hi,
> 
> The pci sysfs "rom" file has disappeared for VGA devices.
> Looks to be a regression from commit 527139d738d7 ("PCI/sysfs:
> Convert "rom" to static attribute").
> 
> Some kind of ordering issue between the sysfs file creation 
> vs. pci_fixup_video() perhaps?
[TLDR: adding this regression to regzbot; most text you find below is
compiled from a few templates paragraphs some of you might have seen
already.]

Hi, this is your Linux kernel regression tracker speaking.

Thanks for the report.

Adding the regression mailing list to the list of recipients, as it
should be in the loop for all regressions, as explained here:
https://www.kernel.org/doc/html/latest/admin-guide/reporting-issues.html

To be sure this issue doesn't fall through the cracks unnoticed, I'm
adding it to regzbot, my Linux kernel regression tracking bot:

#regzbot ^introduced 527139d738d7
#regzbot title pci: the pci sysfs "rom" file has disappeared for VGA devices
#regzbot ignore-activity

Reminder: when fixing the issue, please add a 'Link:' tag with the URL
to the report (the parent of this mail), as explained in
'Documentaiton/process/submitting-patches.rst'. Regzbot then will
automatically mark the regression as resolved once the fix lands in the
appropriate tree. For more details about regzbot see footer.

Sending this to everyone that got the initial report, to make all aware
of the tracking. I also hope that messages like this motivate people to
directly get at least the regression mailing list and ideally even
regzbot involved when dealing with regressions, as messages like this
wouldn't be needed then.

Don't worry, I'll send further messages wrt to this regression just to
the lists (with a tag in the subject so people can filter them away), as
long as they are intended just for regzbot. With a bit of luck no such
messages will be needed anyway.

Ciao, Thorsten (wearing his 'Linux kernel regression tracker' hat).

P.S.: As a Linux kernel regression tracker I'm getting a lot of reports
on my table. I can only look briefly into most of them. Unfortunately
therefore I sometimes will get things wrong or miss something important.
I hope that's not the case here; if you think it is, don't hesitate to
tell me about it in a public reply. That's in everyone's interest, as
what I wrote above might be misleading to everyone reading this; any
suggestion I gave thus might sent someone reading this down the wrong
rabbit hole, which none of us wants.

BTW, I have no personal interest in this issue, which is tracked using
regzbot, my Linux kernel regression tracking bot
(https://linux-regtracking.leemhuis.info/regzbot/). I'm only posting
this mail to get things rolling again and hence don't need to be CC on
all further activities wrt to this regression.

---
Additional information about regzbot:

If you want to know more about regzbot, check out its web-interface, the
getting start guide, and/or the references documentation:

https://linux-regtracking.leemhuis.info/regzbot/
https://gitlab.com/knurd42/regzbot/-/blob/main/docs/getting_started.md
https://gitlab.com/knurd42/regzbot/-/blob/main/docs/reference.md

The last two documents will explain how you can interact with regzbot
yourself if your want to.

Hint for reporters: when reporting a regression it's in your interest to
tell #regzbot about it in the report, as that will ensure the regression
gets on the radar of regzbot and the regression tracker. That's in your
interest, as they will make sure the report won't fall through the
cracks unnoticed.

Hint for developers: you normally don't need to care about regzbot once
it's involved. Fix the issue as you normally would, just remember to
include a 'Link:' tag to the report in the commit message, as explained
in Documentation/process/submitting-patches.rst
That aspect was recently was made more explicit in commit 1f57bd42b77c:
https://git.kernel.org/linus/1f57bd42b77c