ACPI: PCI: Fix device reference counting in acpi_get_pci_dev()

  From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commit 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()") failed
to reference count the device returned by acpi_get_pci_dev() as
expected by its callers which in some cases may cause device objects
to be dropped prematurely.

Add the missing get_device() to acpi_get_pci_dev().

Fixes: 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/pci_root.c |    1 +
 1 file changed, 1 insertion(+)
  

On Tue, Oct 18, 2022 at 07:34:03PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()") failed
> to reference count the device returned by acpi_get_pci_dev() as
> expected by its callers which in some cases may cause device objects
> to be dropped prematurely.
> 
> Add the missing get_device() to acpi_get_pci_dev().
> 
> Fixes: 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()")

FYI this (and the rtc-cmos regression discussed in
https://lore.kernel.org/linux-acpi/5887691.lOV4Wx5bFT@kreacher/)
took down the entire Intel gfx CI. I've applied both fixes
into our fixup branch and things are looking much healthier
now.

This one caused i915 selftests to eat a lot of POISON_FREE
in the CI. While bisecting it locally I didn't have
poisoning enabled so I got refcount_t undeflows instead.

https://intel-gfx-ci.01.org/tree/drm-tip/index.html has a lot
of colorful boxes to click if you're interested in any of the
logs. The fixes are included in the CI_DRM_12259 build. Earlier
builds were broken.

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/pci_root.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: linux-pm/drivers/acpi/pci_root.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/pci_root.c
> +++ linux-pm/drivers/acpi/pci_root.c
> @@ -323,6 +323,7 @@ struct pci_dev *acpi_get_pci_dev(acpi_ha
>  
>  	list_for_each_entry(pn, &adev->physical_node_list, node) {
>  		if (dev_is_pci(pn->dev)) {
> +			get_device(pn->dev);
>  			pci_dev = to_pci_dev(pn->dev);
>  			break;
>  		}
> 
> 
>
  

On Wed, Oct 19, 2022 at 11:02 AM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Tue, Oct 18, 2022 at 07:34:03PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Commit 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()") failed
> > to reference count the device returned by acpi_get_pci_dev() as
> > expected by its callers which in some cases may cause device objects
> > to be dropped prematurely.
> >
> > Add the missing get_device() to acpi_get_pci_dev().
> >
> > Fixes: 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()")
>
> FYI this (and the rtc-cmos regression discussed in
> https://lore.kernel.org/linux-acpi/5887691.lOV4Wx5bFT@kreacher/)
> took down the entire Intel gfx CI.

Sorry for the disturbance.

> I've applied both fixes into our fixup branch and things are looking much
> healthier now.

Thanks for letting me know.

I've just added the $subject patch to my linux-next branch as an
urgent fix and the other one has been applied to the RTC tree.

> This one caused i915 selftests to eat a lot of POISON_FREE
> in the CI. While bisecting it locally I didn't have
> poisoning enabled so I got refcount_t undeflows instead.

Unfortunately, making no mistakes is generally hard to offer.

If catching things like this early is better, what about pulling my
bleeding-edge branch, where all of my changes are staged before going
into linux-next, into the CI?

> https://intel-gfx-ci.01.org/tree/drm-tip/index.html has a lot
> of colorful boxes to click if you're interested in any of the
> logs. The fixes are included in the CI_DRM_12259 build. Earlier
> builds were broken.

Thanks!
  

On Wed, Oct 19, 2022 at 01:35:26PM +0200, Rafael J. Wysocki wrote:
> On Wed, Oct 19, 2022 at 11:02 AM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Tue, Oct 18, 2022 at 07:34:03PM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > Commit 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()") failed
> > > to reference count the device returned by acpi_get_pci_dev() as
> > > expected by its callers which in some cases may cause device objects
> > > to be dropped prematurely.
> > >
> > > Add the missing get_device() to acpi_get_pci_dev().
> > >
> > > Fixes: 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()")
> >
> > FYI this (and the rtc-cmos regression discussed in
> > https://lore.kernel.org/linux-acpi/5887691.lOV4Wx5bFT@kreacher/)
> > took down the entire Intel gfx CI.
> 
> Sorry for the disturbance.
> 
> > I've applied both fixes into our fixup branch and things are looking much
> > healthier now.
> 
> Thanks for letting me know.
> 
> I've just added the $subject patch to my linux-next branch as an
> urgent fix and the other one has been applied to the RTC tree.
> 
> > This one caused i915 selftests to eat a lot of POISON_FREE
> > in the CI. While bisecting it locally I didn't have
> > poisoning enabled so I got refcount_t undeflows instead.
> 
> Unfortunately, making no mistakes is generally hard to offer.
> 
> If catching things like this early is better, what about pulling my
> bleeding-edge branch, where all of my changes are staged before going
> into linux-next, into the CI?

Pretty sure we don't have the resources to become the CI for
everyone. So testing random trees is not really possible. And 
the alternative of pulling random trees into drm-tip is probably
a not a popular idea either. We used to pull in the sound tree
since it's pretty closely tied to graphics, but I think we
stopped even that because it eneded up pulling the whole of
-rc1 in at random points in time when we were't expecting it.

Ideally each subsystem would have its own CI, or there should
be some kernel wide thing. But I suppose the progress towards
something like that is glacial.

That said, we do test linux-next to some degree. And looks like
at least one of these could have been caught a bit earlier through
that. Unfortunately no one is really keeping an eye on that so
things tend to slip through. Probably need to figure out something
to make better use of that.

> 
> > https://intel-gfx-ci.01.org/tree/drm-tip/index.html has a lot
> > of colorful boxes to click if you're interested in any of the
> > logs. The fixes are included in the CI_DRM_12259 build. Earlier
> > builds were broken.
> 
> Thanks!
  

On Wed, 19 Oct 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Oct 19, 2022 at 01:35:26PM +0200, Rafael J. Wysocki wrote:
>> If catching things like this early is better, what about pulling my
>> bleeding-edge branch, where all of my changes are staged before going
>> into linux-next, into the CI?
>
> Pretty sure we don't have the resources to become the CI for
> everyone. So testing random trees is not really possible. And 
> the alternative of pulling random trees into drm-tip is probably
> a not a popular idea either. We used to pull in the sound tree
> since it's pretty closely tied to graphics, but I think we
> stopped even that because it eneded up pulling the whole of
> -rc1 in at random points in time when we were't expecting it.

Basically, we only pull branches to drm-tip that are managed using our
tools and our development model and under our control. It was too much
trouble dealing with conflicts, Linus' master being pulled in at random
points (like in the middle of the merge window), and stuff like that,
with the external trees.

> Ideally each subsystem would have its own CI, or there should
> be some kernel wide thing. But I suppose the progress towards
> something like that is glacial.
>
> That said, we do test linux-next to some degree. And looks like
> at least one of these could have been caught a bit earlier through
> that. Unfortunately no one is really keeping an eye on that so
> things tend to slip through. Probably need to figure out something
> to make better use of that.

Yeah, we need to pay more attention to linux-next test results, as well
as Linus' master during the merge window. It's not necessarily easy with
the volatility of linux-next, you could easily have very broken runs
followed by good ones, but the low hanging fruit is raising more flags
and being louder about it earlier when everything's busted for several
days in linux-next or Linus' master.


BR,
Jani.
  

On Wed, Oct 19, 2022 at 2:22 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Wed, Oct 19, 2022 at 01:35:26PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Oct 19, 2022 at 11:02 AM Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Tue, Oct 18, 2022 at 07:34:03PM +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > Commit 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()") failed
> > > > to reference count the device returned by acpi_get_pci_dev() as
> > > > expected by its callers which in some cases may cause device objects
> > > > to be dropped prematurely.
> > > >
> > > > Add the missing get_device() to acpi_get_pci_dev().
> > > >
> > > > Fixes: 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()")
> > >
> > > FYI this (and the rtc-cmos regression discussed in
> > > https://lore.kernel.org/linux-acpi/5887691.lOV4Wx5bFT@kreacher/)
> > > took down the entire Intel gfx CI.
> >
> > Sorry for the disturbance.
> >
> > > I've applied both fixes into our fixup branch and things are looking much
> > > healthier now.
> >
> > Thanks for letting me know.
> >
> > I've just added the $subject patch to my linux-next branch as an
> > urgent fix and the other one has been applied to the RTC tree.
> >
> > > This one caused i915 selftests to eat a lot of POISON_FREE
> > > in the CI. While bisecting it locally I didn't have
> > > poisoning enabled so I got refcount_t undeflows instead.
> >
> > Unfortunately, making no mistakes is generally hard to offer.
> >
> > If catching things like this early is better, what about pulling my
> > bleeding-edge branch, where all of my changes are staged before going
> > into linux-next, into the CI?
>
> Pretty sure we don't have the resources to become the CI for
> everyone. So testing random trees is not really possible. And
> the alternative of pulling random trees into drm-tip is probably
> a not a popular idea either. We used to pull in the sound tree
> since it's pretty closely tied to graphics, but I think we
> stopped even that because it eneded up pulling the whole of
> -rc1 in at random points in time when we were't expecting it.

I see.

> Ideally each subsystem would have its own CI, or there should
> be some kernel wide thing. But I suppose the progress towards
> something like that is glacial.

Well, I definitely cannot afford a dedicated CI just for my tree and I
haven't got any useful information from KernlCI yet (even though hey
pull and test my linux-next branch on a regular basis).

KernelCI seems to be focusing on different set of hardware, so to speak.

> That said, we do test linux-next to some degree. And looks like
> at least one of these could have been caught a bit earlier through
> that. Unfortunately no one is really keeping an eye on that so
> things tend to slip through. Probably need to figure out something
> to make better use of that.

I think it could also be possible to contribute to KernelCI to get
more useful x86 coverage from it.
  

On Wed, Oct 19, 2022 at 11:54:42AM +0300, Ville Syrjälä wrote:
> On Tue, Oct 18, 2022 at 07:34:03PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Commit 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()") failed
> > to reference count the device returned by acpi_get_pci_dev() as
> > expected by its callers which in some cases may cause device objects
> > to be dropped prematurely.
> > 
> > Add the missing get_device() to acpi_get_pci_dev().
> > 
> > Fixes: 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()")
> 
> FYI this (and the rtc-cmos regression discussed in
> https://lore.kernel.org/linux-acpi/5887691.lOV4Wx5bFT@kreacher/)
> took down the entire Intel gfx CI.

From 1000 miles away and zero background with the gfx CI, this sounds
like "our CI system, whose purpose is to find bugs, found one", which
is a good thing.

Bjorn
  

On Wed, Oct 19, 2022 at 11:53:26AM -0500, Bjorn Helgaas wrote:
> On Wed, Oct 19, 2022 at 11:54:42AM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 18, 2022 at 07:34:03PM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > Commit 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()") failed
> > > to reference count the device returned by acpi_get_pci_dev() as
> > > expected by its callers which in some cases may cause device objects
> > > to be dropped prematurely.
> > > 
> > > Add the missing get_device() to acpi_get_pci_dev().
> > > 
> > > Fixes: 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()")
> > 
> > FYI this (and the rtc-cmos regression discussed in
> > https://lore.kernel.org/linux-acpi/5887691.lOV4Wx5bFT@kreacher/)
> > took down the entire Intel gfx CI.
> 
> >From 1000 miles away and zero background with the gfx CI, this sounds
> like "our CI system, whose purpose is to find bugs, found one", which
> is a good thing.

Mostly. It's certainly better than it going entirely undetected.

Sadly we found it after rc1 because no one was really looking at
linux-next results. Something we need to improve.

But ideally it would have been found by some other CI system
whose primary job is to prevent bugs in those subsystems, rather
than the one whose primary job is to prevent bugs in gfx drivers.
Also ideally it wouldn't have been me bisecting this :P

The biggest downside of bugs reaching our CI via rc1/etc. is that
it pretty much stops everyone from getting premerge results for
their graphics driver patches since the CI keeps tripping over
the already existing bugs. But I guess you can call this one a
somewhat self inflicted wound and we should just try harder to
keep new code out of our tree until it's known to be healthy.