linux-kernel - Re: [PATCH RESEND V2 3/6] platform/x86/intel: Move intel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 08 Dec 2021 13:30:06 -0800
From:   "David E. Box" <david.e.box@...ux.intel.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     lee.jones@...aro.org, hdegoede@...hat.com, bhelgaas@...gle.com,
        andriy.shevchenko@...ux.intel.com, srinivas.pandruvada@...el.com,
        mgross@...ux.intel.com, linux-kernel@...r.kernel.org,
        platform-driver-x86@...r.kernel.org, linux-pci@...r.kernel.org,
        Mark Gross <markgross@...nel.org>
Subject: Re: [PATCH RESEND V2 3/6] platform/x86/intel: Move intel_pmt from
 MFD to Auxiliary Bus

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.

> 
> > intel_vsec_remove_aux() gets called when the PCI device is removed. It calls
> > auxiliary_device_unit()
> > which in turn calls the auxdev release() function that cleans up resources.
> 
> Does this happen when the device is removed, or when the binding of
> driver <-> device is removed?

It happens when the device is removed as tested by unbinding it.

> 
> > When the auxdev is removed, all resources that were dev_m added by the SDSi driver are released
> > too
> > which is why it has no remove() either. I'll add the tests that check this.
> 
> Please do so and document it well, as that is an odd "pattern".

Sure, but I don't think it's that odd in practice given what I already mentioned.

David

> 
> thanks,
> 
> greg k-h