From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "David E. Box" <david.e.box@linux.intel.com>
Cc: me@adhityamohan.in, kw@linux.com, lorenzo.pieralisi@arm.com,
robh@kernel.org, linux-pci@vger.kernel.org,
intel-gfx@lists.freedesktop.org, rafael@kernel.org,
linux-kernel@vger.kernel.org, hch@infradead.org,
jonathan.derrick@linux.dev, bhelgaas@google.com,
nirmal.patel@linux.intel.com, michael.a.bottini@intel.com
Subject: Re: [Intel-gfx] [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR
Date: Tue, 21 Mar 2023 14:38:20 +0200 [thread overview]
Message-ID: <ZBmlPIU4FIBU7HU1@intel.com> (raw)
In-Reply-To: <8675a80b311443d3c3ed99e09832bd07355bfcc2.camel@linux.intel.com>
On Mon, Mar 20, 2023 at 07:24:16PM -0700, David E. Box wrote:
> Hi,
>
> On Tue, 2023-03-21 at 00:56 +0200, Ville Syrjälä wrote:
> > On Thu, Jan 19, 2023 at 07:15:22PM -0800, David E. Box wrote:
> > > +/*
> > > + * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> > > + */
> > > +static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > > +{
> > > + unsigned long features = *(unsigned long *)userdata;
> > > + u16 ltr = VMD_BIOS_PM_QUIRK_LTR;
> > > + u32 ltr_reg;
> > > + int pos;
> > > +
> > > + if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
> > > + return 0;
> > > +
> > > + pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
>
> We call pci_enable_link_state from a callback that's run during pci_walk_bus()
> which I see already acquires the semaphore. We've had this patch for well over a
> year and I haven't seen this issue before. Is there a particular config needed
> to reproduce it?
Not sure what would affect it, beyond the normal PROVE_LOCKING=y.
This is the .config our CI uses:
https://gitlab.freedesktop.org/gfx-ci/i915-infra/-/blob/master/kconfig/debug
>
> As far as a solution I think we can copy what __pci_disable_link_state() does
> and add a bool argument so that we only do down/up on the semaphore when set to
> true. Since we know we will in be the lock during the bus walk we can set it to
> false.
>
> David
>
> >
> > Hi,
> >
> > This is tripping lockdep on one our CI ADL machines.
> >
> > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12814/bat-adlp-6/boot0.txt
> >
> > <4>[ 13.815380] ============================================
> > <4>[ 13.815382] WARNING: possible recursive locking detected
> > <4>[ 13.815384] 6.3.0-rc1-CI_DRM_12814-g4753bbc2a817+ #1 Not tainted
> > <4>[ 13.815386] --------------------------------------------
> > <4>[ 13.815387] swapper/0/1 is trying to acquire lock:
> > <4>[ 13.815389] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > pci_enable_link_state+0x69/0x1d0
> > <4>[ 13.815396]
> > but task is already holding lock:
> > <4>[ 13.815398] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > pci_walk_bus+0x24/0x90
> > <4>[ 13.815403]
> > other info that might help us debug this:
> > <4>[ 13.815404] Possible unsafe locking scenario:
> >
> > <4>[ 13.815406] CPU0
> > <4>[ 13.815407] ----
> > <4>[ 13.815408] lock(pci_bus_sem);
> > <4>[ 13.815410] lock(pci_bus_sem);
> > <4>[ 13.815411]
> > *** DEADLOCK ***
> >
> > <4>[ 13.815413] May be due to missing lock nesting notation
> >
> > <4>[ 13.815414] 2 locks held by swapper/0/1:
> > <4>[ 13.815416] #0: ffff8881029511b8 (&dev->mutex){....}-{3:3}, at:
> > __driver_attach+0xab/0x180
> > <4>[ 13.815422] #1: ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > pci_walk_bus+0x24/0x90
> > <4>[ 13.815426]
> > stack backtrace:
> > <4>[ 13.815428] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.3.0-rc1-
> > CI_DRM_12814-g4753bbc2a817+ #1
> > <4>[ 13.815431] Hardware name: Intel Corporation Alder Lake Client
> > Platform/AlderLake-P DDR4 RVP, BIOS ADLPFWI1.R00.3135.A00.2203251419
> > 03/25/2022
> > <4>[ 13.815434] Call Trace:
> > <4>[ 13.815436] <TASK>
> > <4>[ 13.815437] dump_stack_lvl+0x64/0xb0
> > <4>[ 13.815443] __lock_acquire+0x9b5/0x2550
> > <4>[ 13.815461] lock_acquire+0xd7/0x330
> > <4>[ 13.815463] ? pci_enable_link_state+0x69/0x1d0
> > <4>[ 13.815466] down_read+0x3d/0x180
> > <4>[ 13.815480] ? pci_enable_link_state+0x69/0x1d0
> > <4>[ 13.815482] pci_enable_link_state+0x69/0x1d0
> > <4>[ 13.815485] ? __pfx_vmd_pm_enable_quirk+0x10/0x10
> > <4>[ 13.815488] vmd_pm_enable_quirk+0x49/0xb0
> > <4>[ 13.815490] pci_walk_bus+0x6d/0x90
> > <4>[ 13.815492] vmd_probe+0x75f/0x9d0
> > <4>[ 13.815495] pci_device_probe+0x95/0x120
> > <4>[ 13.815498] really_probe+0x164/0x3c0
> > <4>[ 13.815500] ? __pfx___driver_attach+0x10/0x10
> > <4>[ 13.815503] __driver_probe_device+0x73/0x170
> > <4>[ 13.815506] driver_probe_device+0x19/0xa0
> > <4>[ 13.815508] __driver_attach+0xb6/0x180
> > <4>[ 13.815511] ? __pfx___driver_attach+0x10/0x10
> > <4>[ 13.815513] bus_for_each_dev+0x77/0xd0
> > <4>[ 13.815516] bus_add_driver+0x114/0x210
> > <4>[ 13.815518] driver_register+0x5b/0x110
> > <4>[ 13.815520] ? __pfx_vmd_drv_init+0x10/0x10
> > <4>[ 13.815523] do_one_initcall+0x57/0x330
> > <4>[ 13.815527] kernel_init_freeable+0x181/0x3a0
> > <4>[ 13.815529] ? __pfx_kernel_init+0x10/0x10
> > <4>[ 13.815532] kernel_init+0x15/0x120
> > <4>[ 13.815534] ret_from_fork+0x29/0x50
> > <4>[ 13.815537] </TASK>
> >
--
Ville Syrjälä
Intel
WARNING: multiple messages have this Message-ID (diff)
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "David E. Box" <david.e.box@linux.intel.com>
Cc: nirmal.patel@linux.intel.com, jonathan.derrick@linux.dev,
lorenzo.pieralisi@arm.com, hch@infradead.org, kw@linux.com,
robh@kernel.org, bhelgaas@google.com,
michael.a.bottini@intel.com, rafael@kernel.org,
me@adhityamohan.in, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR
Date: Tue, 21 Mar 2023 14:38:20 +0200 [thread overview]
Message-ID: <ZBmlPIU4FIBU7HU1@intel.com> (raw)
In-Reply-To: <8675a80b311443d3c3ed99e09832bd07355bfcc2.camel@linux.intel.com>
On Mon, Mar 20, 2023 at 07:24:16PM -0700, David E. Box wrote:
> Hi,
>
> On Tue, 2023-03-21 at 00:56 +0200, Ville Syrjälä wrote:
> > On Thu, Jan 19, 2023 at 07:15:22PM -0800, David E. Box wrote:
> > > +/*
> > > + * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> > > + */
> > > +static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > > +{
> > > + unsigned long features = *(unsigned long *)userdata;
> > > + u16 ltr = VMD_BIOS_PM_QUIRK_LTR;
> > > + u32 ltr_reg;
> > > + int pos;
> > > +
> > > + if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
> > > + return 0;
> > > +
> > > + pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
>
> We call pci_enable_link_state from a callback that's run during pci_walk_bus()
> which I see already acquires the semaphore. We've had this patch for well over a
> year and I haven't seen this issue before. Is there a particular config needed
> to reproduce it?
Not sure what would affect it, beyond the normal PROVE_LOCKING=y.
This is the .config our CI uses:
https://gitlab.freedesktop.org/gfx-ci/i915-infra/-/blob/master/kconfig/debug
>
> As far as a solution I think we can copy what __pci_disable_link_state() does
> and add a bool argument so that we only do down/up on the semaphore when set to
> true. Since we know we will in be the lock during the bus walk we can set it to
> false.
>
> David
>
> >
> > Hi,
> >
> > This is tripping lockdep on one our CI ADL machines.
> >
> > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12814/bat-adlp-6/boot0.txt
> >
> > <4>[ 13.815380] ============================================
> > <4>[ 13.815382] WARNING: possible recursive locking detected
> > <4>[ 13.815384] 6.3.0-rc1-CI_DRM_12814-g4753bbc2a817+ #1 Not tainted
> > <4>[ 13.815386] --------------------------------------------
> > <4>[ 13.815387] swapper/0/1 is trying to acquire lock:
> > <4>[ 13.815389] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > pci_enable_link_state+0x69/0x1d0
> > <4>[ 13.815396]
> > but task is already holding lock:
> > <4>[ 13.815398] ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > pci_walk_bus+0x24/0x90
> > <4>[ 13.815403]
> > other info that might help us debug this:
> > <4>[ 13.815404] Possible unsafe locking scenario:
> >
> > <4>[ 13.815406] CPU0
> > <4>[ 13.815407] ----
> > <4>[ 13.815408] lock(pci_bus_sem);
> > <4>[ 13.815410] lock(pci_bus_sem);
> > <4>[ 13.815411]
> > *** DEADLOCK ***
> >
> > <4>[ 13.815413] May be due to missing lock nesting notation
> >
> > <4>[ 13.815414] 2 locks held by swapper/0/1:
> > <4>[ 13.815416] #0: ffff8881029511b8 (&dev->mutex){....}-{3:3}, at:
> > __driver_attach+0xab/0x180
> > <4>[ 13.815422] #1: ffffffff827ab0b0 (pci_bus_sem){++++}-{3:3}, at:
> > pci_walk_bus+0x24/0x90
> > <4>[ 13.815426]
> > stack backtrace:
> > <4>[ 13.815428] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.3.0-rc1-
> > CI_DRM_12814-g4753bbc2a817+ #1
> > <4>[ 13.815431] Hardware name: Intel Corporation Alder Lake Client
> > Platform/AlderLake-P DDR4 RVP, BIOS ADLPFWI1.R00.3135.A00.2203251419
> > 03/25/2022
> > <4>[ 13.815434] Call Trace:
> > <4>[ 13.815436] <TASK>
> > <4>[ 13.815437] dump_stack_lvl+0x64/0xb0
> > <4>[ 13.815443] __lock_acquire+0x9b5/0x2550
> > <4>[ 13.815461] lock_acquire+0xd7/0x330
> > <4>[ 13.815463] ? pci_enable_link_state+0x69/0x1d0
> > <4>[ 13.815466] down_read+0x3d/0x180
> > <4>[ 13.815480] ? pci_enable_link_state+0x69/0x1d0
> > <4>[ 13.815482] pci_enable_link_state+0x69/0x1d0
> > <4>[ 13.815485] ? __pfx_vmd_pm_enable_quirk+0x10/0x10
> > <4>[ 13.815488] vmd_pm_enable_quirk+0x49/0xb0
> > <4>[ 13.815490] pci_walk_bus+0x6d/0x90
> > <4>[ 13.815492] vmd_probe+0x75f/0x9d0
> > <4>[ 13.815495] pci_device_probe+0x95/0x120
> > <4>[ 13.815498] really_probe+0x164/0x3c0
> > <4>[ 13.815500] ? __pfx___driver_attach+0x10/0x10
> > <4>[ 13.815503] __driver_probe_device+0x73/0x170
> > <4>[ 13.815506] driver_probe_device+0x19/0xa0
> > <4>[ 13.815508] __driver_attach+0xb6/0x180
> > <4>[ 13.815511] ? __pfx___driver_attach+0x10/0x10
> > <4>[ 13.815513] bus_for_each_dev+0x77/0xd0
> > <4>[ 13.815516] bus_add_driver+0x114/0x210
> > <4>[ 13.815518] driver_register+0x5b/0x110
> > <4>[ 13.815520] ? __pfx_vmd_drv_init+0x10/0x10
> > <4>[ 13.815523] do_one_initcall+0x57/0x330
> > <4>[ 13.815527] kernel_init_freeable+0x181/0x3a0
> > <4>[ 13.815529] ? __pfx_kernel_init+0x10/0x10
> > <4>[ 13.815532] kernel_init+0x15/0x120
> > <4>[ 13.815534] ret_from_fork+0x29/0x50
> > <4>[ 13.815537] </TASK>
> >
--
Ville Syrjälä
Intel
next prev parent reply other threads:[~2023-03-21 12:38 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-20 3:15 [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware David E. Box
2023-01-20 3:15 ` [PATCH V10 1/4] PCI/ASPM: Add pci_enable_link_state() David E. Box
2023-01-20 3:15 ` [PATCH V10 2/4] PCI: vmd: Use PCI_VDEVICE in device list David E. Box
2023-01-20 3:15 ` [PATCH V10 3/4] PCI: vmd: Create feature grouping for client products David E. Box
2023-01-20 3:15 ` [PATCH V10 4/4] PCI: vmd: Add quirk to configure PCIe ASPM and LTR David E. Box
2023-03-20 22:56 ` [Intel-gfx] " Ville Syrjälä
2023-03-20 22:56 ` Ville Syrjälä
2023-03-21 2:24 ` [Intel-gfx] " David E. Box
2023-03-21 2:24 ` David E. Box
2023-03-21 12:38 ` Ville Syrjälä [this message]
2023-03-21 12:38 ` Ville Syrjälä
2023-03-21 23:40 ` [Intel-gfx] " David E. Box
2023-03-21 23:40 ` David E. Box
2023-01-20 5:28 ` [PATCH V10 0/4] Enable PCIe ASPM and LTR on select hardware Sathyanarayanan Kuppuswamy
2023-02-02 15:07 ` Lorenzo Pieralisi
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=ZBmlPIU4FIBU7HU1@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=bhelgaas@google.com \
--cc=david.e.box@linux.intel.com \
--cc=hch@infradead.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jonathan.derrick@linux.dev \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=me@adhityamohan.in \
--cc=michael.a.bottini@intel.com \
--cc=nirmal.patel@linux.intel.com \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.