linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Bjorn Helgaas <helgaas@kernel.org>
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: Tue, 25 Jan 2022 20:56:14 +0200	[thread overview]
Message-ID: <YfBHzo4kNGgd2jTd@intel.com> (raw)
In-Reply-To: <20220122002445.GA1162144@bhelgaas>

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

  reply	other threads:[~2022-01-25 18:56 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-17 10:44 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
2022-01-25 18:56       ` Ville Syrjälä [this message]
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=YfBHzo4kNGgd2jTd@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=bhelgaas@google.com \
    --cc=helgaas@kernel.org \
    --cc=kw@linux.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=oohall@gmail.com \
    --subject='Re: [REGRESSION] 527139d738d7 ("PCI/sysfs: Convert "rom" to static attribute")' \
    /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

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