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]

Message-ID: <8c9d0a86-cbbb-be1b-41b1-61f27890b77d@redhat.com>
Date:   Tue, 21 Dec 2021 19:38:38 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     david.e.box@...ux.intel.com, Greg KH <gregkh@...uxfoundation.org>
Cc:     lee.jones@...aro.org, 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

Hi,

On 12/21/21 19:16, David E. Box wrote:
> On Tue, 2021-12-21 at 18:04 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 12/21/21 17:54, Greg KH wrote:
>>> On Tue, Dec 21, 2021 at 08:44:57AM -0800, David E. Box wrote:
>>>> 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.
>>>
>>> I can take the aux driver drvdata patch now, through my tree, if you
>>> want, no need to make it wait for this tiny driver.
>>>
>>> Feel free to send it independant of the existing patchset, and with the
>>> cleanup patches at the same time, should be quite easy to get merged.
>>
>> If you're going to take that one, can you perhaps take patches
>> 1-3 for 5.17 through your tree as well (patch 3 depends on 2/it) ?
>>
>> Note there is a v4 of this series, see please use that :)
>>
>> I assume the follow up patches are also going to need patch 3
>> (the actual conversion of the driver to aux-bus).
> 
> Yes.
> 
>>
>> Here is my Ack for the pdx86 bits in patch 3:
>>
>> Acked-by: Hans de Goede <hdegoede@...hat.com>
>>
>> And patch 1 and 3 also have acks from the PCI resp. MFD subsys maintainers,
>> so I guess taking this all upstream through your tree is fine.
> 
> Should I send 1-3 plus the drvdata cleanup patches I have to Grep? V5?

No there is no need for that v4 is fine, since no changes have been
requested there is no need to send out a new version.

Regards,

Hans