From: Bjorn Helgaas <helgaas@kernel.org>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: "Krzysztof Wilczyński" <kw@linux.com>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Oliver O'Halloran" <oohall@gmail.com>,
linux-pci@vger.kernel.org
Subject: Re: [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")
Date: Fri, 21 Jan 2022 18:24:45 -0600 [thread overview]
Message-ID: <20220122002445.GA1162144@bhelgaas> (raw)
In-Reply-To: <YbzMyDm+5PCer8Fj@intel.com>
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[] = {
next prev parent reply other threads:[~2022-01-22 0:24 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-17 10:44 [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute") Ville Syrjälä
2021-12-17 17:29 ` Bjorn Helgaas
2021-12-17 17:45 ` Ville Syrjälä
2021-12-17 22:49 ` Krzysztof Wilczyński
2022-01-20 13:19 ` Thorsten Leemhuis
2022-01-20 15:00 ` Bjorn Helgaas
2022-01-20 15:05 ` Thorsten Leemhuis
2022-01-22 0:24 ` Bjorn Helgaas [this message]
2022-01-25 18:56 ` Ville Syrjälä
2021-12-18 6:10 ` Thorsten Leemhuis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220122002445.GA1162144@bhelgaas \
--to=helgaas@kernel.org \
--cc=bhelgaas@google.com \
--cc=kw@linux.com \
--cc=linux-pci@vger.kernel.org \
--cc=oohall@gmail.com \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).