Re: [PATCH v6 09/11] drm/xe/configfs: Check if device was preconfigured

[John Harrison <john.c.harrison@intel.com>]

On 8/5/2025 12:30 PM, Lucas De Marchi wrote:
> On Thu, Jul 31, 2025 at 09:33:37PM +0200, Michal Wajdeczko wrote:
>> Since device configuration using configfs could be prepared long
>> time prior the driver load, add a debug log whether the current
>> device driver probe is using custom or default settings.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>> v2: make function void, rename helpers (Lucas) and rebased
>> ---
>> drivers/gpu/drm/xe/xe_configfs.c | 25 +++++++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_configfs.h |  2 ++
>> drivers/gpu/drm/xe/xe_pci.c      |  3 +++
>> 3 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_configfs.c 
>> b/drivers/gpu/drm/xe/xe_configfs.c
>> index 150e7f2becc8..5f145ccdf535 100644
>> --- a/drivers/gpu/drm/xe/xe_configfs.c
>> +++ b/drivers/gpu/drm/xe/xe_configfs.c
>> @@ -107,6 +107,11 @@ static void set_device_defaults(struct 
>> xe_config_device *config)
>>     *config = device_defaults;
>> }
>>
>> +static bool config_device_is_default(const struct xe_config_device 
>> *config)
>
> this isn't exactly safe and as such shouldn't be a helper that can be
> misused.
>
> $ pahole -C xe_config_device 
> /p2/build-drm-xe-next/drivers/gpu/drm/xe/xe.o
> struct xe_config_device {
>         bool                       survivability_mode;   /* 0     1 */
>
>         /* XXX 7 bytes hole, try to pack */
>
>         u64                        engines_allowed;      /* 8     8 */
>
>         /* size: 16, cachelines: 1, members: 2 */
>         /* sum members: 9, holes: 1, sum holes: 7 */
>         /* last cacheline: 16 bytes */
> };
>
On i915 at least, the rule for the module parameter structure was to 
keep all bools at the end. Would that fix the issue here? If the 
structure is full size integers first, then bool/char values at the end 
then there would be no holes and no issues with uninitialised data?

John.

>> +{
>> +    return !memcmp(config, &device_defaults, sizeof(*config));
>> +}
>> +
>> struct engine_info {
>>     const char *cls;
>>     u64 mask;
>> @@ -339,6 +344,26 @@ static struct xe_config_group_device 
>> *find_xe_config_group_device(struct pci_dev
>>     return to_xe_config_group_device(item);
>> }
>>
>> +/**
>> + * xe_configfs_check_device() - Test if device was configured by 
>> configfs
>> + * @pdev: the &pci_dev device to test
>> + *
>> + * Try to find the configfs group that belongs to the specified pci 
>> device
>> + * and print a diagnostic message if found.
>> + */
>> +void xe_configfs_check_device(struct pci_dev *pdev)
>> +{
>> +    struct xe_config_group_device *dev = 
>> find_xe_config_group_device(pdev);
>> +
>> +    if (!dev)
>> +        return;
>> +
>> +    pci_dbg(pdev, "found %s settings in configfs\n",
>> +        config_device_is_default(&dev->config) ? "default" : "custom");
>
> I guess comparing dev->config and device_defaults is safe because both
> will zero-initialize the hole (or padding if we had one): the former is
> allocated via kzalloc() and the latter has static storage.
>
> I'd just remove the helper and do the memcmp explicitly in this
> function.
>
> Lucas De Marchi
>
>> +
>> +    config_group_put(&dev->group);
>> +}
>> +
>> /**
>>  * xe_configfs_get_survivability_mode - get configfs survivability 
>> mode attribute
>>  * @pdev: pci device
>> diff --git a/drivers/gpu/drm/xe/xe_configfs.h 
>> b/drivers/gpu/drm/xe/xe_configfs.h
>> index fb8764008089..fa4ea7f0c375 100644
>> --- a/drivers/gpu/drm/xe/xe_configfs.h
>> +++ b/drivers/gpu/drm/xe/xe_configfs.h
>> @@ -13,12 +13,14 @@ struct pci_dev;
>> #if IS_ENABLED(CONFIG_CONFIGFS_FS)
>> int xe_configfs_init(void);
>> void xe_configfs_exit(void);
>> +void xe_configfs_check_device(struct pci_dev *pdev);
>> bool xe_configfs_get_survivability_mode(struct pci_dev *pdev);
>> void xe_configfs_clear_survivability_mode(struct pci_dev *pdev);
>> u64 xe_configfs_get_engines_allowed(struct pci_dev *pdev);
>> #else
>> static inline int xe_configfs_init(void) { return 0; }
>> static inline void xe_configfs_exit(void) { }
>> +static inline void xe_configfs_check_device(struct pci_dev *pdev) { }
>> static inline bool xe_configfs_get_survivability_mode(struct pci_dev 
>> *pdev) { return false; }
>> static inline void xe_configfs_clear_survivability_mode(struct 
>> pci_dev *pdev) { }
>> static inline u64 xe_configfs_get_engines_allowed(struct pci_dev 
>> *pdev) { return U64_MAX; }
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 52d46c66ae1e..9ce6e6dca5bc 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -17,6 +17,7 @@
>>
>> #include "display/xe_display.h"
>> #include "regs/xe_gt_regs.h"
>> +#include "xe_configfs.h"
>> #include "xe_device.h"
>> #include "xe_drv.h"
>> #include "xe_gt.h"
>> @@ -771,6 +772,8 @@ static int xe_pci_probe(struct pci_dev *pdev, 
>> const struct pci_device_id *ent)
>>     struct xe_device *xe;
>>     int err;
>>
>> +    xe_configfs_check_device(pdev);
>> +
>>     if (desc->require_force_probe && !id_forced(pdev->device)) {
>>         dev_info(&pdev->dev,
>>              "Your graphics device %04x is not officially supported\n"
>> -- 
>> 2.47.1
>>