Re: [PATCH 00/16] Add new debug infrastructure for configfs

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Sep 28, 2026 at 01:07:19PM -0400, Summers, Stuart wrote:
> On Sun, 2026-09-27 at 20:34 -0700, Matthew Brost wrote:
> > On Thu, Sep 24, 2026 at 11:01:19PM +0000, Stuart Summers wrote:
> > 
> > Thanks for the work here. I was going to look in depth but
> > immediately
> > have a question.
> > 
> > > Add a new configfs debug group. The intent of this structure is
> > > to allow us to separate ABI facing configfs entries from those
> > > which are purely for debug purposes. And it allows us more
> > > flexibility
> > 
> > Why not use debugfs for debug knobs and keep configfs as a strict
> > ABI? Is
> > the reasoning that everything in configfs is considered a boot-time
> > configuration?
> > 
> > In the past, I've added debug tunables as module parameters that
> > probably should have lived in either configfs or debugfs.
> > 
> > I'm just struggling to understand the distinction here. What really
> > defines a "normal" configfs setting versus a "debug" configfs
> > setting?
> > For example, let's say we have a tuning parameter that a
> > knowledgeable
> > customer may want to adjust. Where should that live?
> > 
> > I'm not opposed to this; I'm just a bit fuzzy on the rules and the
> > reasoning behind them. That said, if we can't get clear rules then
> > I'd
> > say lump everything together.
> 
> Yeah at least from my perspective, module parameters are things that
> apply to all devices on the system at probe time (yes we can change
> some of these at runtime, but typically they are probe-time
> parameters).
> 
> debugfs is used for either information reporting or for runtime
> configuration for debug purposes (vs sysfs for runtime configuration
> for "production" purposes - ABI)
> 
> configfs is used for probe-time configurations that need to be per
> device.
> 
> I know in i915 we had overloaded the modparams with a debugfs layer
> that looks a like what I have here for the X-params. At least when we
> were discussing for Xe, we wanted to move more towards configfs for
> these kinds of parameters.
> 
> I just want a way to be able to quickly and easily add new parameters
> upstream that are needed for low level software/hardware debug with a
> clear way to implement those. If we want to go the debugfs route, I'm
> sure we can make that work too, but I want it to be really clear where
> that should live. We do already have things like enable_psmi and
> enable_multi_queue here that are doing exactly this - probe time, per-
> device parameters.
> 
> In terms of how we define what is production and what is debug... I
> think that's going to be a case-by-case basis. Essentially what I was
> thinking is a production parameter is something a customer will be
> using as part of a "normal" runtime flow (like firmware update for
> survivability_mode). Whereas a debug parameter is something we don't
> want a customer to use without explicitly understanding what they are
> doing - they are wrapped in a debug kconfig and have explicit
> documentation. One example we don't have here now but could add in the
> future is something like the enable_rc6 where we really don't want
> customers to be setting this unless they are trying to debug something
> with us - it would at least take a kernel rebuild from what is provided
> in the distros.
> 
> I'm open to discussion. I just want to make sure we have a clear
> process here so we can facilitate the parameters we need for debug.

I agree with Stuart here. It is case by case and I don't believe we need
written hard rules here.
We should have easy ways to add debug stuff.
If it is runtime: debugfs
If it is needed at probe time: configfs protected by debug config
Module parameter should be really avoided for these debugs.

For production:
Runtime: It depends on the case: sysfs, ioctl, fwctl, netlink, io_uring, ...
Probe time: Configfs
Boot time and homogeneous across all devices: Module Parameter

But again, production like these only if really needed and understanding the
validation impacts: http://www.islinuxaboutchoice.com/

Thanks,
Rodrigo.

> 
> Thanks,
> Stuart
> 
> > 
> > Matt
> > 
> > > in how we define those parameters used for debug.
> > > 
> > > Add a new infrastructure to this debug configfs group that lets us
> > > easily define the parameters in a quick list. This is primarily
> > > useful for simple, single-type parameters such as enable/disable
> > > features or simple values passed. For more complex parameters,
> > > we will still need to define these separately.
> > > 
> > > Pull the GuC target related changes from [1] to fit within
> > > this new structure and add a new definition for guc_log_level
> > > on top of the existing module parameter (to ensure we aren't
> > > impacting existing users of the module parameter).
> > > 
> > > Note that the debug parameters here are all to be used "at your
> > > own risk". Without having in depth knowledge of how these impact
> > > the software and hardware, there could be unforeseen consequences
> > > of setting them. As such, they are all wrapped in a
> > > CONFIG_DRM_XE_DEBUG configfs option.
> > > 
> > > In terms of the patches here, I'm sorting the existing parameters
> > > by name/type. I know we have a few other module parameters that
> > > could migrate here, but I didn't want to overload this series
> > > too much, so the focus for now is on the existing configfs entries
> > > and demonstrating the new structures with the GuC log level and
> > > target parameters.
> > > 
> > > I used GitHub Copilot with Claude pretty extensively through the
> > > process here and attributed as such. Happy to answer any questions
> > > around this. Took a bit of time getting back to this series around
> > > other work, and in that time I was playing around with a few
> > > different
> > > models, hence some of the patches are showing multiple of them. I
> > > tried to attribute each as I was implementing the changes.
> > > 
> > > I also decided to drop John Harrison from the NPK patch. It has
> > > been modified quite a bit from the original, but more importantly
> > > John is no longer with Intel and that email address isn't available
> > > any more. If it makes a difference here, John and I had both
> > > separately
> > > implemented this same change at different occasions for debug. The
> > > one I used to start that initial series was cherry-picked from his
> > > latest variant.
> > > 
> > > v2:
> > >  - In this second revision I did confirm that the guc_log_level
> > >    module parameter is taking precedence over the configfs
> > > parameter
> > >    and ensured the other parameters seem to be autogenerating and
> > >    working as expected.
> > >  - I tried to address all the review feedback from the first
> > >    revision, [2].
> > >  - I also did another pass on the sorting since there were a few
> > >    discrepancies I noticed in the first revision. I kept Gustavo's
> > >    R-B on that one, but would like an ack before merging at least
> > >    to confirm the patch is sane.
> > >  - And finally I moved the getter functions into the X-macro
> > >    generators so we can autogenerate more of the similar functions
> > >    between the different parameters in that debug param list.
> > > v3:
> > >  - Address a couple of comments from Sashiko around GuC log level
> > >    input checking and proper guard implementation.
> > > v4:
> > >  - More review feedback from Sashiko addressed...
> > > v5:
> > >  - Move the goto to a return (more Sashiko feedback) in the GuC
> > >    log level setter before moving to the X-macro solution.
> > > v6:
> > >  - Make the autogenerated X-macro function names more specific to
> > >    avoid naming collisions (Sashiko again).
> > > v7:
> > >  - Fix the couple of pre-existing bugs called out by Sashiko in
> > >    the prior rev...
> > >  - Make CONFIGFS_FS a required config for xe to avoid issues with
> > >    stale values in the fallback getters.
> > >  - Renamed disable_vram_page_offline to enable_vram_page_offline
> > >    for a more consistent naming scheme (this was a new configfs
> > >    entry added since the prior rev).
> > >  - Converted survivability_mode to a u8 bitmap to allow for
> > >    extendability in the future. Only bit 0 is defined, so the
> > > behavior
> > >    should be the same.
> > >  - Added an enable_media module parameter at the end of the series.
> > >    gt_types_allowed is debug-only, so this gives production builds
> > > a
> > >    supported way to leave the media GT alone. The modparam takes
> > >    precedence over configfs.
> > >  - Adjust the sorting to be alphabetical for the documentation
> > >    specifically (Matt)
> > > 
> > > [1]: https://patchwork.freedesktop.org/series/162087/
> > > [2]: https://patchwork.freedesktop.org/series/165879/
> > > 
> > > Stuart Summers (16):
> > >   drm/xe: Guard configfs attribute reads in getters
> > >   drm/xe/configfs: Fix out-of-bounds read in parse_wa_bb_lines()
> > >   drm/xe/configfs: Copy wa_bb out under the configfs lock
> > >   drm/xe: Require CONFIGFS_FS
> > >   drm/xe: Invert vram_page_offline configfs attribute
> > >   drm/xe: Make survivability_mode configfs attribute a bitmap
> > >   drm/xe: Sort xe_config_device fields
> > >   drm/xe: Split out configfs data structures
> > >   drm/xe: Add a new debug focused configfs group
> > >   drm/xe: Move debug configfs entries to xe_configfs_debug.c
> > >   drm/xe/guc: Add configfs support for guc_log_level
> > >   drm/xe/guc: Add support for NPK as a GuC log target
> > >   drm/xe: Add infrastructure for debug configfs parameters
> > >   drm/xe: Migrate existing debug configfs entries to params
> > >     infrastructure
> > >   drm/xe: Taint kernel when debug configfs parameters are set
> > >   drm/xe: Add enable_media module parameter
> > > 
> > >  drivers/gpu/drm/xe/Kconfig                    |    1 +
> > >  drivers/gpu/drm/xe/Makefile                   |    3 +-
> > >  drivers/gpu/drm/xe/abi/guc_log_abi.h          |    8 +
> > >  drivers/gpu/drm/xe/xe_configfs.c              | 1056 ++-----------
> > > ----
> > >  drivers/gpu/drm/xe/xe_configfs.h              |  124 +-
> > >  drivers/gpu/drm/xe/xe_configfs_debug.c        |  899
> > > ++++++++++++++
> > >  drivers/gpu/drm/xe/xe_configfs_debug.h        |   48 +
> > >  drivers/gpu/drm/xe/xe_configfs_debug_params.c |  158 +++
> > >  drivers/gpu/drm/xe/xe_configfs_debug_params.h |  194 +++
> > >  drivers/gpu/drm/xe/xe_configfs_types.h        |   60 +
> > >  drivers/gpu/drm/xe/xe_defaults.h              |    6 +
> > >  drivers/gpu/drm/xe/xe_drm_ras_types.h         |    4 +-
> > >  drivers/gpu/drm/xe/xe_guc.c                   |   14 +-
> > >  drivers/gpu/drm/xe/xe_guc_ads.c               |    1 +
> > >  drivers/gpu/drm/xe/xe_guc_log.c               |    3 +-
> > >  drivers/gpu/drm/xe/xe_hw_engine.c             |    1 +
> > >  drivers/gpu/drm/xe/xe_lrc.c                   |   39 +-
> > >  drivers/gpu/drm/xe/xe_module.c                |    6 +
> > >  drivers/gpu/drm/xe/xe_module.h                |    1 +
> > >  drivers/gpu/drm/xe/xe_pci.c                   |   20 +-
> > >  drivers/gpu/drm/xe/xe_psmi.c                  |    3 +-
> > >  drivers/gpu/drm/xe/xe_ras.c                   |    9 +-
> > >  drivers/gpu/drm/xe/xe_rtp.c                   |    3 +-
> > >  drivers/gpu/drm/xe/xe_survivability_mode.c    |    7 +-
> > >  drivers/gpu/drm/xe/xe_ttm_vram_mgr.c          |    2 +-
> > >  25 files changed, 1615 insertions(+), 1055 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/xe/xe_configfs_debug.c
> > >  create mode 100644 drivers/gpu/drm/xe/xe_configfs_debug.h
> > >  create mode 100644 drivers/gpu/drm/xe/xe_configfs_debug_params.c
> > >  create mode 100644 drivers/gpu/drm/xe/xe_configfs_debug_params.h
> > >  create mode 100644 drivers/gpu/drm/xe/xe_configfs_types.h
> > > 
> > > -- 
> > > 2.43.0
> > > 
>