Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3d734a1a20ed2e895a0a7177f3789c72e26e7fd5.camel@sipsolutions.net>
Subject: Re: [PATCH v2] PCI/PM: enable runtime PM later during device
 scanning
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Lukas Wunner <lukas@wunner.de>
Cc:     linux-wireless@vger.kernel.org, linux-pci@vger.kernel.org,
        linux-pm@vger.kernel.org, rafael@kernel.org,
        Bjorn Helgaas <bhelgaas@google.com>
Date:   Tue, 06 Jun 2023 09:22:19 +0200
In-Reply-To: <20230605205017.GA23596@wunner.de>
References: <20230605121621.4259f1be6cd2.Idbaa55b93f780838af44ebccb84c36f60716df04@changeid>
         <20230605203519.bc4232207449.Idbaa55b93f780838af44ebccb84c36f60716df04@changeid>
         <20230605205017.GA23596@wunner.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.2 (3.48.2-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2023-06-05 at 22:50 +0200, Lukas Wunner wrote:
> On Mon, Jun 05, 2023 at 08:35:45PM +0200, Johannes Berg wrote:
> > v2: use pm_runtime_get_noresume()/pm_runtime_put_noidle()
> >     instead as advised by Rafael
>=20
> You've changed the code but seemingly did not update the commit
> message and code comment.
>=20

Yeah. I actually _considered_ that, but didn't feel it was really any
different (or let's say wrong) now.

That said, I probably don't understand the lingo around runtime PM well
enough, and am more or less conflating "runtime PM" and "runtime
suspend" in my head, which is still not allowed, and indeed that's the
whole point of the patch.

>   Technically you're not "allowing"
> runtime PM, you just stop keeping the device runtime active.
>=20
> A more fitting subject might thus be:
>=20
> PCI/PM: Keep devices runtime active during enumeration

*shrug*

Like I said, terminology I'm not familiar with. I guess I can change it,
or if anyone ends up committing it as is (rather than treating it as an
extended bug report) they can :-)

> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -1278,6 +1278,9 @@ static int pci_pm_runtime_suspend(struct device *=
dev)
> >  	pci_power_t prev =3D pci_dev->current_state;
> >  	int error;
> > =20
> > +	if (WARN_ON(!pci_dev_is_added(pci_dev)))
> > +		return -EBUSY;
> > +
>=20
> If this can't happen (as the commit message says), why warn?

The code here causes quite some trouble if it _does_ happen and it was
incredibly tricky to debug.

johannes