Re: [PATCH 3/9] drm/xe: Split out configfs data structures

[Matthew Brost <matthew.brost@intel.com>]

On Mon, May 04, 2026 at 08:24:14AM -0600, Summers, Stuart wrote:
> On Mon, 2026-05-04 at 11:47 +0300, Jani Nikula wrote:
> > On Mon, 04 May 2026, Stuart Summers <stuart.summers@intel.com> wrote:
> > > Split the configfs data structures into their own _types.h file.
> > 
> > Why? The commit message must always answer the question, "why".
> > 

This is a good point. In general, we should be moving public structs
that have no business being in public headers (we do quite a bit of this
in Xe) into private structs in C files. This patch does the opposite, so
unless there is a very good reason to make this public (e.g, a struct
is intended to embedded), my initial reaction is not to do this.

Private structures enforce good layering and component isolation, and
prevent vectors for abuse (i.e., another layer messing with a struct
owned by a different layer).

Matt

> > The obvious downside here is that you expose the types that were
> > previously hidden in xe_configfs.c to anyone who includes
> > xe_configfs_types.h. And that header depends on a ton of other
> > headers,
> > making the header interdepencies worse overall.
> > 
> > Maybe you need that header later for something, but please spell that
> > out here.
> 
> Yeah this is exactly true.. I'll do a better job with the commit
> messages here going forward. Really appreciate the feedback Jani!
> 
> -Stuart
> 
> > 
> > 
> > BR,
> > Jani.
> > 
> > > 
> > > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > > Assisted-by: Copilot:claude-opus-4.7
> > > ---
> > >  drivers/gpu/drm/xe/xe_configfs.c       | 85 +++++++---------------
> > > ----
> > >  drivers/gpu/drm/xe/xe_configfs_types.h | 59 ++++++++++++++++++
> > >  2 files changed, 80 insertions(+), 64 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/xe/xe_configfs_types.h
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_configfs.c
> > > b/drivers/gpu/drm/xe/xe_configfs.c
> > > index 1e134057fae8..12b7fe65446d 100644
> > > --- a/drivers/gpu/drm/xe/xe_configfs.c
> > > +++ b/drivers/gpu/drm/xe/xe_configfs.c
> > > @@ -4,7 +4,6 @@
> > >   */
> > >  
> > >  #include <linux/bitops.h>
> > > -#include <linux/ctype.h>
> > >  #include <linux/configfs.h>
> > >  #include <linux/cleanup.h>
> > >  #include <linux/find.h>
> > > @@ -15,12 +14,10 @@
> > >  
> > >  #include "instructions/xe_mi_commands.h"
> > >  #include "xe_configfs.h"
> > > +#include "xe_configfs_types.h"
> > >  #include "xe_defaults.h"
> > >  #include "xe_gt_types.h"
> > > -#include "xe_hw_engine_types.h"
> > >  #include "xe_module.h"
> > > -#include "xe_pci_types.h"
> > > -#include "xe_sriov_types.h"
> > >  
> > >  /**
> > >   * DOC: Xe Configfs
> > > @@ -245,36 +242,6 @@
> > >   *     # rmdir /sys/kernel/config/xe/0000:03:00.0/
> > >   */
> > >  
> > > -/* Similar to struct xe_bb, but not tied to HW (yet) */
> > > -struct wa_bb {
> > > -       u32 *cs;
> > > -       u32 len; /* in dwords */
> > > -};
> > > -
> > > -struct xe_config_group_device {
> > > -       struct config_group group;
> > > -       struct config_group sriov;
> > > -
> > > -       struct xe_config_device {
> > > -               struct wa_bb
> > > ctx_restore_mid_bb[XE_ENGINE_CLASS_MAX];
> > > -               struct wa_bb
> > > ctx_restore_post_bb[XE_ENGINE_CLASS_MAX];
> > > -               bool enable_psmi;
> > > -               bool enable_survivability_mode;
> > > -               u64 engines_allowed;
> > > -               u64 gt_types_allowed;
> > > -               struct {
> > > -                       bool admin_only_pf;
> > > -                       unsigned int max_vfs;
> > > -               } sriov;
> > > -       } config;
> > > -
> > > -       /* protects attributes */
> > > -       struct mutex lock;
> > > -       /* matching descriptor */
> > > -       const struct xe_device_desc *desc;
> > > -       /* tentative SR-IOV mode */
> > > -       enum xe_sriov_mode mode;
> > > -};
> > >  
> > >  static const struct xe_config_device device_defaults = {
> > >         .enable_psmi = false,
> > > @@ -322,16 +289,6 @@ static const struct {
> > >         { .name = "media", .type = XE_GT_TYPE_MEDIA },
> > >  };
> > >  
> > > -static struct xe_config_group_device
> > > *to_xe_config_group_device(struct config_item *item)
> > > -{
> > > -       return container_of(to_config_group(item), struct
> > > xe_config_group_device, group);
> > > -}
> > > -
> > > -static struct xe_config_device *to_xe_config_device(struct
> > > config_item *item)
> > > -{
> > > -       return &to_xe_config_group_device(item)->config;
> > > -}
> > > -
> > >  static bool is_bound(struct xe_config_group_device *dev)
> > >  {
> > >         unsigned int domain, bus, slot, function;
> > > @@ -359,7 +316,7 @@ static bool is_bound(struct
> > > xe_config_group_device *dev)
> > >  
> > >  static ssize_t enable_survivability_mode_show(struct config_item
> > > *item, char *page)
> > >  {
> > > -       struct xe_config_device *dev = to_xe_config_device(item);
> > > +       struct xe_config_device *dev = xe_configfs_to_device(item);
> > >  
> > >         return sprintf(page, "%d\n", dev-
> > > >enable_survivability_mode);
> > >  }
> > > @@ -367,7 +324,7 @@ static ssize_t
> > > enable_survivability_mode_show(struct config_item *item, char *pa
> > >  static ssize_t enable_survivability_mode_store(struct config_item
> > > *item, const char *page,
> > >                                                size_t len)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item);
> > >         bool enable_survivability_mode;
> > >         int ret;
> > >  
> > > @@ -386,7 +343,7 @@ static ssize_t
> > > enable_survivability_mode_store(struct config_item *item, const c
> > >  
> > >  static ssize_t gt_types_allowed_show(struct config_item *item,
> > > char *page)
> > >  {
> > > -       struct xe_config_device *dev = to_xe_config_device(item);
> > > +       struct xe_config_device *dev = xe_configfs_to_device(item);
> > >         char *p = page;
> > >  
> > >         for (size_t i = 0; i < ARRAY_SIZE(gt_types); i++)
> > > @@ -399,7 +356,7 @@ static ssize_t gt_types_allowed_show(struct
> > > config_item *item, char *page)
> > >  static ssize_t gt_types_allowed_store(struct config_item *item,
> > > const char *page,
> > >                                       size_t len)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item);
> > >         char *buf __free(kfree) = kstrdup(page, GFP_KERNEL);
> > >         char *p = buf;
> > >         u64 typemask = 0;
> > > @@ -437,7 +394,7 @@ static ssize_t gt_types_allowed_store(struct
> > > config_item *item, const char *page
> > >  
> > >  static ssize_t engines_allowed_show(struct config_item *item, char
> > > *page)
> > >  {
> > > -       struct xe_config_device *dev = to_xe_config_device(item);
> > > +       struct xe_config_device *dev = xe_configfs_to_device(item);
> > >         char *p = page;
> > >  
> > >         for (size_t i = 0; i < ARRAY_SIZE(engine_info); i++) {
> > > @@ -529,7 +486,7 @@ static int parse_engine(const char *s, const
> > > char *end_chars, u64 *mask,
> > >  static ssize_t engines_allowed_store(struct config_item *item,
> > > const char *page,
> > >                                      size_t len)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item);
> > >         ssize_t patternlen, p;
> > >         u64 mask, val = 0;
> > >  
> > > @@ -552,14 +509,14 @@ static ssize_t engines_allowed_store(struct
> > > config_item *item, const char *page,
> > >  
> > >  static ssize_t enable_psmi_show(struct config_item *item, char
> > > *page)
> > >  {
> > > -       struct xe_config_device *dev = to_xe_config_device(item);
> > > +       struct xe_config_device *dev = xe_configfs_to_device(item);
> > >  
> > >         return sprintf(page, "%d\n", dev->enable_psmi);
> > >  }
> > >  
> > >  static ssize_t enable_psmi_store(struct config_item *item, const
> > > char *page, size_t len)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item);
> > >         bool val;
> > >         int ret;
> > >  
> > > @@ -634,14 +591,14 @@ static ssize_t wa_bb_show(struct
> > > xe_config_group_device *dev,
> > >  
> > >  static ssize_t ctx_restore_mid_bb_show(struct config_item *item,
> > > char *page)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item);
> > >  
> > >         return wa_bb_show(dev, dev->config.ctx_restore_mid_bb,
> > > page, SZ_4K);
> > >  }
> > >  
> > >  static ssize_t ctx_restore_post_bb_show(struct config_item *item,
> > > char *page)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item);
> > >  
> > >         return wa_bb_show(dev, dev->config.ctx_restore_post_bb,
> > > page, SZ_4K);
> > >  }
> > > @@ -798,7 +755,7 @@ static ssize_t wa_bb_store(struct wa_bb
> > > wa_bb[static XE_ENGINE_CLASS_MAX],
> > >  static ssize_t ctx_restore_mid_bb_store(struct config_item *item,
> > >                                         const char *data, size_t
> > > sz)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item);
> > >  
> > >         return wa_bb_store(dev->config.ctx_restore_mid_bb, dev,
> > > data, sz);
> > >  }
> > > @@ -806,7 +763,7 @@ static ssize_t ctx_restore_mid_bb_store(struct
> > > config_item *item,
> > >  static ssize_t ctx_restore_post_bb_store(struct config_item *item,
> > >                                          const char *data, size_t
> > > sz)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item);
> > >  
> > >         return wa_bb_store(dev->config.ctx_restore_post_bb, dev,
> > > data, sz);
> > >  }
> > > @@ -830,7 +787,7 @@ static struct configfs_attribute
> > > *xe_config_device_attrs[] = {
> > >  
> > >  static void xe_config_device_release(struct config_item *item)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item);
> > >  
> > >         mutex_destroy(&dev->lock);
> > >  
> > > @@ -846,7 +803,7 @@ static struct configfs_item_operations
> > > xe_config_device_ops = {
> > >  static bool xe_config_device_is_visible(struct config_item *item,
> > >                                         struct configfs_attribute
> > > *attr, int n)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item);
> > >  
> > >         if (attr == &attr_enable_survivability_mode) {
> > >                 if (!dev->desc->is_dgfx || dev->desc->platform <
> > > XE_BATTLEMAGE)
> > > @@ -869,7 +826,7 @@ static const struct config_item_type
> > > xe_config_device_type = {
> > >  
> > >  static ssize_t sriov_max_vfs_show(struct config_item *item, char
> > > *page)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item->ci_parent);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item->ci_parent);
> > >  
> > >         guard(mutex)(&dev->lock);
> > >  
> > > @@ -881,7 +838,7 @@ static ssize_t sriov_max_vfs_show(struct
> > > config_item *item, char *page)
> > >  
> > >  static ssize_t sriov_max_vfs_store(struct config_item *item, const
> > > char *page, size_t len)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item->ci_parent);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item->ci_parent);
> > >         unsigned int max_vfs;
> > >         int ret;
> > >  
> > > @@ -903,7 +860,7 @@ static ssize_t sriov_max_vfs_store(struct
> > > config_item *item, const char *page, s
> > >  
> > >  static ssize_t sriov_admin_only_pf_show(struct config_item *item,
> > > char *page)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item->ci_parent);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item->ci_parent);
> > >  
> > >         guard(mutex)(&dev->lock);
> > >  
> > > @@ -912,7 +869,7 @@ static ssize_t sriov_admin_only_pf_show(struct
> > > config_item *item, char *page)
> > >  
> > >  static ssize_t sriov_admin_only_pf_store(struct config_item *item,
> > > const char *page, size_t len)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item->ci_parent);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item->ci_parent);
> > >         bool admin_only_pf;
> > >         int ret;
> > >  
> > > @@ -941,7 +898,7 @@ static struct configfs_attribute
> > > *xe_config_sriov_attrs[] = {
> > >  static bool xe_config_sriov_is_visible(struct config_item *item,
> > >                                        struct configfs_attribute
> > > *attr, int n)
> > >  {
> > > -       struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item->ci_parent);
> > > +       struct xe_config_group_device *dev =
> > > xe_configfs_to_group_device(item->ci_parent);
> > >  
> > >         if (attr == &sriov_attr_max_vfs && dev->mode !=
> > > XE_SRIOV_MODE_PF)
> > >                 return false;
> > > @@ -1084,7 +1041,7 @@ static struct xe_config_group_device
> > > *find_xe_config_group_device(struct pci_dev
> > >         if (!item)
> > >                 return NULL;
> > >  
> > > -       return to_xe_config_group_device(item);
> > > +       return xe_configfs_to_group_device(item);
> > >  }
> > >  
> > >  static void dump_custom_dev_config(struct pci_dev *pdev,
> > > diff --git a/drivers/gpu/drm/xe/xe_configfs_types.h
> > > b/drivers/gpu/drm/xe/xe_configfs_types.h
> > > new file mode 100644
> > > index 000000000000..935097aafa96
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/xe/xe_configfs_types.h
> > > @@ -0,0 +1,59 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright © 2026 Intel Corporation
> > > + */
> > > +#ifndef _XE_CONFIGFS_TYPES_H_
> > > +#define _XE_CONFIGFS_TYPES_H_
> > > +
> > > +#include <linux/configfs.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/types.h>
> > > +
> > > +#include "xe_hw_engine_types.h"
> > > +#include "xe_pci_types.h"
> > > +#include "xe_sriov_types.h"
> > > +
> > > +struct config_item;
> > > +
> > > +/* Similar to struct xe_bb, but not tied to HW (yet) */
> > > +struct wa_bb {
> > > +       u32 *cs;
> > > +       u32 len; /* in dwords */
> > > +};
> > > +
> > > +struct xe_config_group_device {
> > > +       struct config_group group;
> > > +       struct config_group sriov;
> > > +
> > > +       struct xe_config_device {
> > > +               struct wa_bb
> > > ctx_restore_mid_bb[XE_ENGINE_CLASS_MAX];
> > > +               struct wa_bb
> > > ctx_restore_post_bb[XE_ENGINE_CLASS_MAX];
> > > +               bool enable_psmi;
> > > +               bool enable_survivability_mode;
> > > +               u64 engines_allowed;
> > > +               u64 gt_types_allowed;
> > > +               struct {
> > > +                       bool admin_only_pf;
> > > +                       unsigned int max_vfs;
> > > +               } sriov;
> > > +       } config;
> > > +
> > > +       /* protects attributes */
> > > +       struct mutex lock;
> > > +       /* matching descriptor */
> > > +       const struct xe_device_desc *desc;
> > > +       /* tentative SR-IOV mode */
> > > +       enum xe_sriov_mode mode;
> > > +};
> > > +
> > > +static inline struct xe_config_group_device
> > > *xe_configfs_to_group_device(struct config_item *item)
> > > +{
> > > +       return container_of(to_config_group(item), struct
> > > xe_config_group_device, group);
> > > +}
> > > +
> > > +static inline struct xe_config_device
> > > *xe_configfs_to_device(struct config_item *item)
> > > +{
> > > +       return &xe_configfs_to_group_device(item)->config;
> > > +}
> > > +
> > > +#endif /* _XE_CONFIGFS_TYPES_H_ */
> > 
>