Re: [PATCH 1/2] mei: me: Add exported function to check ME device availabiliy

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 7/16/2025 10:57 AM, Daniele Ceraolo Spurio wrote:
>
>
> On 7/16/2025 9:49 AM, Greg Kroah-Hartman wrote:
>> On Wed, Jul 16, 2025 at 09:38:09AM -0700, Daniele Ceraolo Spurio wrote:
>>>
>>> On 7/15/2025 10:10 PM, Greg Kroah-Hartman wrote:
>>>> On Tue, Jul 15, 2025 at 04:00:01PM -0700, Daniele Ceraolo Spurio 
>>>> wrote:
>>>>> The intel GFX drivers (i915/xe) interface with the ME device for 
>>>>> some of
>>>>> their features (e.g. PXP, HDCP) via the component interface. Given 
>>>>> that
>>>>> the MEI device can be hidden by BIOS/Coreboot, the GFX drivers need a
>>>>> way to check if the device is available before attempting to bind the
>>>>> component, otherwise they'll go ahead and initialize features that 
>>>>> will
>>>>> never work.
>>>>> The simplest way to check if the device is available is to check the
>>>>> available devices against the PCI ID list of the mei_me driver. To 
>>>>> avoid
>>>>> duplication of the list, the function to do such a check is added to
>>>>> the mei_me driver and exported so that the GFX driver can call it
>>>>> directly.
>>>>>
>>>>> Signed-off-by: Daniele Ceraolo Spurio 
>>>>> <daniele.ceraolospurio@intel.com>
>>>>> Cc: Alexander Usyskin <alexander.usyskin@intel.com>
>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>> ---
>>>>>    drivers/misc/mei/pci-me.c | 17 +++++++++++++++++
>>>>>    include/linux/mei_me.h    | 20 ++++++++++++++++++++
>>>>>    2 files changed, 37 insertions(+)
>>>>>    create mode 100644 include/linux/mei_me.h
>>>>>
>>>>> diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c
>>>>> index 3f9c60b579ae..16e9a11eb286 100644
>>>>> --- a/drivers/misc/mei/pci-me.c
>>>>> +++ b/drivers/misc/mei/pci-me.c
>>>>> @@ -18,6 +18,7 @@
>>>>>    #include <linux/pm_runtime.h>
>>>>>    #include <linux/mei.h>
>>>>> +#include <linux/mei_me.h>
>>>>>    #include "mei_dev.h"
>>>>>    #include "client.h"
>>>>> @@ -133,6 +134,22 @@ static const struct pci_device_id 
>>>>> mei_me_pci_tbl[] = {
>>>>>    MODULE_DEVICE_TABLE(pci, mei_me_pci_tbl);
>>>>> +/**
>>>>> + * mei_me_device_present - check if an ME device is present on 
>>>>> the system
>>>>> + *
>>>>> + * Other drivers (e.g., i915, xe) interface with the ME device 
>>>>> for some of their
>>>>> + * features (e.g., PXP, HDCP). However, the ME device can be 
>>>>> hidden by
>>>>> + * BIOS/coreboot, so this function offers a way for those drivers 
>>>>> to check if
>>>>> + * the device is available before attempting to interface with it.
>>>>> + *
>>>>> + * Return: true if an ME device is available, false otherwise
>>>>> + */
>>>>> +bool mei_me_device_present(void)
>>>>> +{
>>>>> +    return pci_dev_present(mei_me_pci_tbl);
>>>> And what happens if the device goes away right after you call this?
>>> Both intel graphics drivers do already handle the device being 
>>> missing or
>>> going away, it's just not very clean. Behavior changes based on when 
>>> this
>>> happens:
>>>
>>> - if the device is never there or disappears before the component 
>>> binds, we
>>> timeout waiting for the bind
>>> - if the device disappears after the bind, we handle the case as 
>>> part of the
>>> component unbind call
>>>
>>> The timeout is quite long and can therefore impact/delay userspace, 
>>> so the
>>> aim here is to mitigate that impact in the case where the device is 
>>> just
>>> never there, which is easy to check and more likely to happen 
>>> compared to
>>> the device going away after initially being there.
>>>
>>> Should I add a note about the device going away to the function doc?
>>> Something like "Callers are still expected to handle the case where the
>>> device goes away after this check is performed".
>> My point is that calls like this are pointless without a lock, as the
>> state that you think the device is in (which this function returns), is
>> just a lie as it could have already changed.
>>
>> So look into what you are really wanting to do here, as what this
>> function does is not what you think it is doing :)
>
> What we want to do from the GFX driver POV can be summarized as 
> follows: if at the time we try to initialize PXP the CSME PCI device 
> is not present, we don't initialize PXP and continue without it.
>
> We don't want to support hot (re-)plug of CSME, so we don't care if 
> the state changes after we've checked (or it has already changed by 
> the time we act on it):
> - if CSME was not there at check-time and re-appears later, we'll 
> still continue without PXP support.
> - if CSME was there at check-time and then disappears, we'll handle it 
> as described above.
>
> IMO this function covers this use-case, but maybe it's too narrow a 
> use-case for it to be a generic export in the mei driver?
> Would it be cleaner if I replaced this with a function to export the 
> PCI ID list, and then ran the pci_dev_present check inside i915, so it 
> doesn't matter if it only covers our narrow use-case?

Any more feedback on this? Should I just go ahead and switch to 
returning the PCI ID list?

Thanks,
Daniele

>
>>
>>>>> +}
>>>>> +EXPORT_SYMBOL(mei_me_device_present);
>>>> EXPORT_SYMBOL_GPL()?  I have to ask, sorry.
>>> The non-GPL version seemed more appropriate to me because I didn't 
>>> think
>>> calling this function qualified as "derivative work", but I don't 
>>> really
>>> care either way because both i915 and Xe are part of the kernel and
>>> GPL-compatible, so I can switch to the GPL version.
>> All other mei_* exports are EXPORT_SYMBOL_GPL(), please don't break that
>> pattern without a whole lot of documentation and reasons to back it up.
>
> Sure, will switch.
>
> Thanks,
> Daniele
>
>>
>> thanks,
>>
>> greg k-h
>