Re: [PATCH] drm: xe: select configfs

[Matt Roper <matthew.d.roper@intel.com>]

On Fri, Nov 13, 2026 at 12:58:21PM -0500, Rodrigo Vivi wrote:
> On Fri, Nov 13, 2026 at 08:22:12AM -0800, Matt Roper wrote:
> > On Wed, Nov 11, 2026 at 02:01:45PM -0500, Rodrigo Vivi wrote:
> > > On Mon, Oct 26, 2026 at 11:25:50AM +0100, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann <arnd@arndb.de>
> > > > 
> > > > The original addition of configfs to xe missed a compile-time dependency,
> > > > which leads to a link failure when configfs is enabled as a loadable module:
> > > > 
> > > > ld.lld-21: error: undefined symbol: xe_configfs_get_survivability_mode
> > > > >>> referenced by xe_survivability_mode.c:224
> > > > >>>               drivers/gpu/drm/xe/xe_survivability_mode.o:(xe_survivability_mode_is_requested) in archive vmlinux.a
> > > > 
> > > > A recent workaround added a 'IS_REACHABLE()' check around the calls into
> > > > configfs, which makes it link in this configuration, but this is generally
> > > > a bad idea because it makes the actual behavior inconsistent and confusing
> > > > to users that expect configfs to work if both xe and configfs are enabled.
> > > > 
> > > > Use 'select CONFIGFS_FS' instead, like every other configfs user does,
> > > > and remove the unnecessary stub functions. There is little practical
> > > > difference here as anyone using XE graphics is likely to have a complex
> > > > configuration that already selects configfs anyway, and the link error
> > > > or the IS_REACHABLE() would only manifest during randconfig testing.
> > > > 
> > > > Fixes: 30499cbd7961 ("drm/xe: Fix build with DRM_XE=y and CONFIGFS_FS=m")
> > > > Fixes: 16280ded45fb ("drm/xe: Add configfs to enable survivability mode")
> > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > > ---
> > > >  drivers/gpu/drm/xe/Kconfig       |  1 +
> > > >  drivers/gpu/drm/xe/xe_configfs.h | 30 ------------------------------
> > > >  2 files changed, 1 insertion(+), 30 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> > > > index 4fc8abdc5cb6..b52a8dc28f97 100644
> > > > --- a/drivers/gpu/drm/xe/Kconfig
> > > > +++ b/drivers/gpu/drm/xe/Kconfig
> > > > @@ -11,6 +11,7 @@ config DRM_XE
> > > >  	# the shmem_readpage() which depends upon tmpfs
> > > >  	select SHMEM
> > > >  	select TMPFS
> > > > +	select CONFIGFS_FS
> > > 
> > > Perhaps another option would be what Jani had suggested:
> > > 
> > > config DRM_XE_CONFIGFS
> > > 	def_bool y
> > > 	depends on DRM_XE && CONFIGFS_FS
> > > 	depends on CONFIGFS_FS=y || DRM_XE=m
> > > 
> > > But anyway, we are depending more and more on the configfs for
> > > production choices. So, let's go with you select and close this
> > > for good.
> > 
> > I really don't think this patch was the way we wanted to go.
> > IS_REACHABLE was giving us exactly the behavior that we wanted,
> 
> But it caused other issues like reported in the commit msg of this patch.
> With the risk of other complications like Jani had pointed out earlier.

No, the existing code had already fixed the issue reported in the commit
message; this commit isn't fixing any further issues.  The justification
for this patch was "users who don't understand the reachability
implications of 'M' vs 'Y' might not get the behavior they expect if
they make mistakes while hand-modifying their config."  But that's just
user error; those exact configuration settings (Xe="Y" and configfs="M")
might have been precisely what some other user wanted.

> 
> > but now
> > we're getting behavior that we did not want.  Xe absolutely does not
> > depend on configfs in any way.  _If_ configfs is there we'll enable some
> > config options in it, but none of those are mandatory for our driver as
> > a whole.  We're perfectly happy to build and run without configfs if
> > it's not selected in the config.  We're also happy to build and run
> > without configfs functionality if the user wanted Xe built directly into
> > the kernel, but configfs was built as an unreachable module solely for
> > other non-Xe purposes.
> > 
> > While this change won't matter to 99% of the cases where someone is
> > running with a typical distro kernel, this is the kind of thing that
> > makes embedded users unhappy since they're very carefully managing their
> > configurations and we've taken away their ability to avoid configfs now.
> 
> I believe some other user could be angry as well for ending in a case
> where they need manual survivability mode to flash the firmware, but
> they don't have this option available because in his config this
> option was not reachable.

Kconfig lets users select the options they want.  If they accidentally
create a config that doesn't include the options they need (either
because they left them at 'N' or because they set them at 'M' to prevent
them from being accessible as part of the built-in drivers), then that's
their own mistake and they can correct it themselves.

In some embedded environments, there are very strict certification steps
that have to be applied to code built directly into the kernel, and
sometimes a bit more lee-way for the .ko's that may or may not get
loaded later.  If an embedded device knows they have no use for Xe's
configfs settings, they may not want configfs built into the kernel due
to the extra burdens that places on them.  There may be other drivers or
facilities that are not part of their critical path that are built as
modules, and having configfs also built as a .ko for those other areas
may be useful.  So Xe='Y' + configs='M' is a deliberate choice to match
a specialized environment.  But we've just broken that flexibility with
this change.

> 
> Soon SRIOV will be another production case that will depend on the configfs.
> I know, one might argue that we should then select only if SRIOV is enabled.
> 
> But I don't like the complication this is taking on a very simple thing.

I'd argue that this *isn't* a simple thing, even if throwing a 'select'
in the code and removing the stubs is easy for us as developers.  The
reality is that crafting a fine-tuned kernel config for some specific
purpose is always going to be a complex task.  Generic configs like
those provided by distros sidestep the complexity by including anything
that someone potentially might want, but if you're fine-tuning a config
then you really have to pay attention to all the dependencies and
consider exactly what you do or don't want included as builtin or
module.  It's easy to make mistakes, but that's always been the case if
you're hand-crafting a minimal kernel configuration.

> 
> > 
> > Also, as mentioned earlier in the threads, using 'select' on a
> > user-visible config option rather than a hidden option often causes
> > cascading problems down the road because it ignores any dependencies of
> > the option being selected (configfs in this case).  At the moment I
> > don't think configfs has any dependencies of its own, but if it gets
> > some down the road, this can cause problems.
> 
> I'm not strongly in favor of any approach, as long that it doesn't cause
> other issues and regressions. So, if someone want to try to bring this
> again, proving it is a good config combination solution that won't
> cause further issue I will be glad to accept.

There weren't any issues before this patch (except for hypothetical user
error), but now we've removed flexibility that was important to a
[small] subset of users.


Matt

> 
> Thanks,
> Rodrigo.
> 
> > 
> > 
> > Matt
> > 
> > > 
> > > I'm pushing this to drm-xe-next.
> > > 
> > > Thanks for the patch.
> > > 
> > > 
> > > >  	select DRM_BUDDY
> > > >  	select DRM_CLIENT_SELECTION
> > > >  	select DRM_KMS_HELPER
> > > > diff --git a/drivers/gpu/drm/xe/xe_configfs.h b/drivers/gpu/drm/xe/xe_configfs.h
> > > > index 327b648944ff..d726800de26a 100644
> > > > --- a/drivers/gpu/drm/xe/xe_configfs.h
> > > > +++ b/drivers/gpu/drm/xe/xe_configfs.h
> > > > @@ -14,7 +14,6 @@
> > > >  
> > > >  struct pci_dev;
> > > >  
> > > > -#if IS_REACHABLE(CONFIG_CONFIGFS_FS)
> > > >  int xe_configfs_init(void);
> > > >  void xe_configfs_exit(void);
> > > >  void xe_configfs_check_device(struct pci_dev *pdev);
> > > > @@ -36,34 +35,5 @@ u32 xe_configfs_get_ctx_restore_post_bb(struct pci_dev *pdev,
> > > >  unsigned int xe_configfs_get_max_vfs(struct pci_dev *pdev);
> > > >  bool xe_configfs_admin_only_pf(struct pci_dev *pdev);
> > > >  #endif
> > > > -#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 bool xe_configfs_primary_gt_allowed(struct pci_dev *pdev) { return true; }
> > > > -static inline bool xe_configfs_media_gt_allowed(struct pci_dev *pdev) { return true; }
> > > > -static inline u64 xe_configfs_get_engines_allowed(struct pci_dev *pdev) { return U64_MAX; }
> > > > -static inline bool xe_configfs_get_psmi_enabled(struct pci_dev *pdev) { return false; }
> > > > -static inline bool xe_configfs_get_enable_multi_queue(struct pci_dev *pdev) { return true; }
> > > > -static inline u32 xe_configfs_get_migrate_ulls_period_ms(struct pci_dev *pdev) { return 5; }
> > > > -static inline bool xe_configfs_get_disable_vram_page_offline(struct pci_dev *pdev) { return false; }
> > > > -static inline u32 xe_configfs_get_ctx_restore_mid_bb(struct pci_dev *pdev,
> > > > -						     enum xe_engine_class class,
> > > > -						     const u32 **cs) { return 0; }
> > > > -static inline u32 xe_configfs_get_ctx_restore_post_bb(struct pci_dev *pdev,
> > > > -						      enum xe_engine_class class,
> > > > -						      const u32 **cs) { return 0; }
> > > > -#ifdef CONFIG_PCI_IOV
> > > > -static inline unsigned int xe_configfs_get_max_vfs(struct pci_dev *pdev)
> > > > -{
> > > > -	return xe_modparam.max_vfs;
> > > > -}
> > > > -static inline bool xe_configfs_admin_only_pf(struct pci_dev *pdev)
> > > > -{
> > > > -	return XE_DEFAULT_ADMIN_ONLY_PF;
> > > > -}
> > > > -#endif
> > > > -#endif
> > > >  
> > > >  #endif
> > > > -- 
> > > > 2.53.0
> > > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation