Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 4 Aug 2023 18:27:09 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Igor Mammedov <imammedo@redhat.com>
Cc:     linux-kernel@vger.kernel.org, terraluna977@gmail.com,
        bhelgaas@google.com, linux-pci@vger.kernel.org, mst@redhat.com,
        rafael@kernel.org, linux-acpi@vger.kernel.org
Subject: Re: [PATCH 1/1] PCI: acpiphp:: use
 pci_assign_unassigned_bridge_resources() only if bus->self not NULL
Message-ID: <20230804232709.GA174043@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230726123518.2361181-2-imammedo@redhat.com>
Precedence: bulk

On Wed, Jul 26, 2023 at 02:35:18PM +0200, Igor Mammedov wrote:
> Commit [1] switched acpiphp hotplug to use
>    pci_assign_unassigned_bridge_resources()
> which depends on bridge being available, however in some cases
> when acpiphp is in use, enable_slot() can get a slot without
> bridge associated.

acpiphp is *always* in use if we get to enable_slot(), so that doesn't
really add information here.

>   1. legitimate case of hotplug on root bus
>       (likely not exiting on real hw, but widely used in virt world)
>   2. broken firmware, that sends 'Bus check' events to non
>      existing root ports (Dell Inspiron 7352/0W6WV0), which somehow
>      endup at acpiphp:enable_slot(..., bridge = 0) and with bus
>      without bridge assigned to it.

IIUC, the Inspiron problem happens when:

  - acpiphp_context->bridge is NULL, so hotplug_event() calls
    enable_slot() instead of acpiphp_check_bridge(), AND

  - acpiphp_slot->bus->self is also NULL, because enable_slot() calls
    pci_assign_unassigned_bridge_resources() with that NULL pointer,
    which dereferences "bridge->subordinate"

But I can't figure out why acpiphp_context->bridge is NULL for RP07
and RP08 (which don't exist), but not for RP03 (which does).

I guess all the acpiphp_contexts (RP03, RP07, RP08) must be allocated in
acpiphp_add_context() by acpiphp_init_context().

Woody's lspci from [1] shows only one Root Port:

  00:1c.0 Wildcat Point-LP PCI Express Root Port #3

The DSDT.DSL includes:

  Device (RP01) _ADR 0x001C0000		# 1c.0
  Device (RP02) _ADR 0x001C0001		# 1c.1
  Device (RP03) _ADR 0x001C0002		# 1c.2
  Device (RP04) _ADR 0x001C0003		# 1c.3
  Device (RP05) _ADR 0x001C0004		# 1c.4
  Device (RP06) _ADR 0x001C0005		# 1c.5
  Device (RP07) _ADR 0x001C0006		# 1c.6
  Device (RP08) _ADR 0x001C0007		# 1c.7

I can see why we might need a Bus Check after resume to see if
something got added while we were suspended.  But I don't see why we
handle RP03 differently from RP07 and RP08.

Can you help me out?  I'm lost in a maze of twisty passages, all
alike.

Bjorn

[1] https://lore.kernel.org/r/92150d8d-8a3a-d600-a996-f60a8e4c876c@gmail.com