Message-ID: <e9648546c3fb751954e411dfa392f0e0f90f0c85.camel@linux.intel.com>
Subject: Re: [PATCH RESEND V2 3/6] platform/x86/intel: Move intel_pmt from
 MFD to Auxiliary Bus
From:   "David E. Box" <david.e.box@linux.intel.com>
Reply-To: david.e.box@linux.intel.com
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     lee.jones@linaro.org, hdegoede@redhat.com, bhelgaas@google.com,
        andriy.shevchenko@linux.intel.com, srinivas.pandruvada@intel.com,
        mgross@linux.intel.com, linux-kernel@vger.kernel.org,
        platform-driver-x86@vger.kernel.org, linux-pci@vger.kernel.org,
        Mark Gross <markgross@kernel.org>
Date:   Tue, 21 Dec 2021 08:44:57 -0800
In-Reply-To: <YcGEaH0oAAocziU2@kroah.com>
References: <20211208015015.891275-1-david.e.box@linux.intel.com>
         <20211208015015.891275-4-david.e.box@linux.intel.com>
         <YbDbql39x7Kw6iAC@kroah.com>
         <7e78e6311cb0d261892f7361a1ef10130436f358.camel@linux.intel.com>
         <YbD1NsYHbU8FvtTN@kroah.com>
         <a70956e1c4da10603e29087e893cbae62ce82631.camel@linux.intel.com>
         <YbEFuN7fwdiNI8vW@kroah.com>
         <622887d53eaf6e6ae36354bfa0ed483df1cd9214.camel@linux.intel.com>
         <YcGEaH0oAAocziU2@kroah.com>
Organization: David E. Box
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2021-12-21 at 08:38 +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 01:30:06PM -0800, David E. Box wrote:
> > On Wed, 2021-12-08 at 20:21 +0100, Greg KH wrote:
> > > On Wed, Dec 08, 2021 at 11:09:48AM -0800, David E. Box wrote:
> > > > On Wed, 2021-12-08 at 19:11 +0100, Greg KH wrote:
> > > > > On Wed, Dec 08, 2021 at 09:47:26AM -0800, David E. Box wrote:
> > > > > > On Wed, 2021-12-08 at 17:22 +0100, Greg KH wrote:
> > > > > > > On Tue, Dec 07, 2021 at 05:50:12PM -0800, David E. Box wrote:
> > > > > > > > +static struct pci_driver intel_vsec_pci_driver = {
> > > > > > > > +       .name = "intel_vsec",
> > > > > > > > +       .id_table = intel_vsec_pci_ids,
> > > > > > > > +       .probe = intel_vsec_pci_probe,
> > > > > > > > +};
> > > > > > > 
> > > > > > > So when the PCI device is removed from the system you leak
> > > > > > > resources and
> > > > > > > have dangling devices?
> > > > > > 
> > > > > > No.
> > > > > > 
> > > > > > > Why no PCI remove driver callback?
> > > > > > 
> > > > > > After probe all resources are device managed. There's nothing to
> > > > > > explicitly clean up. When
> > > > > > the
> > > > > > PCI
> > > > > > device is removed, all aux devices are automatically removed. This
> > > > > > is the case for the SDSi
> > > > > > driver
> > > > > > as well.
> > > > > 
> > > > > Where is the "automatic cleanup" happening?  As this pci driver is
> > > > > bound
> > > > > to the PCI device, when the device is removed, what is called in this
> > > > > driver to remove the resources allocated in the probe callback?
> > > > > 
> > > > > confused,
> > > > 
> > > > devm_add_action_or_reset(&pdev->dev, intel_vsec_remove_aux, auxdev)
> > > 
> > > Wow that is opaque.  Why not do it on remove instead?
> > 
> > This code is common for auxdev cleanup. AFAICT most auxiliary bus code is
> > done by drivers that have
> > some other primary function. They clean up their primary function resources
> > in remove, but they
> > clean up the auxdev using the method above. In this case the sole purpose of
> > this driver is to
> > create the auxdev. There are no other resources beyond what the auxdev is
> > using.
> > 
> > Adding runtime pm to the pci driver will change this. Remove will be needed
> > then.
> 
> And who will notice that being required when that happens?
> 
> Why is there no runtime PM for this driver?  Do you not care about power
> consumption?  :)

Of course. :)

There's a backlog of patches waiting for this series. One adds support for the
telemetry device (an auxdev) on the DG2 GPU. This device requires runtime pm in
order for the slot to go D3. But this also requires changes to the telemetry
driver in order for runtime pm to be handled correctly. These and other patches,
including a series to have all current aux drivers use the new drvdata helpers,
are waiting for this.

David

> 
> thanks,
> 
> greg k-h