Re: [PATCH 3/3] drm/xe/configfs: Don't expose survivability_mode if not applicable

["Summers, Stuart" <stuart.summers@intel.com>]

On Tue, 2025-09-02 at 20:16 +0200, Michal Wajdeczko wrote:
> 
> 
> On 9/2/2025 7:25 PM, Summers, Stuart wrote:
> > On Tue, 2025-09-02 at 15:17 +0200, Michal Wajdeczko wrote:
> > > The survivability_mode attribute is applicable only for DGFX and
> > > platforms newer than BATTLEMAGE. Use .is_visible() hook to hide
> > > this attribute when above conditions are not met. Remove code
> > > that
> > > was trying to fix such configuration during the runtime.
> > > 
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Riana Tauro <riana.tauro@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_configfs.c           | 24 ++++++----------
> > > ----
> > > --
> > >  drivers/gpu/drm/xe/xe_configfs.h           |  2 --
> > >  drivers/gpu/drm/xe/xe_survivability_mode.c | 11 +---------
> > >  3 files changed, 7 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_configfs.c
> > > b/drivers/gpu/drm/xe/xe_configfs.c
> > > index 43f000260752..0337811864cd 100644
> > > --- a/drivers/gpu/drm/xe/xe_configfs.c
> > > +++ b/drivers/gpu/drm/xe/xe_configfs.c
> > > @@ -369,7 +369,12 @@ static bool
> > > xe_config_device_is_visible(struct
> > > config_item *item,
> > >  {
> > >         struct xe_config_group_device *dev =
> > > to_xe_config_group_device(item);
> > >  
> > > -       return dev->desc; /* shall be always true */
> > > +       if (attr == &attr_survivability_mode) {
> > > +               if (!dev->desc->is_dgfx || dev->desc->platform <
> > > XE_BATTLEMAGE)
> > > +                       return false;
> > > +       }
> > > +
> > > +       return true;
> > 
> > Why change the return here? Can we either leave this as dev->desc
> > or
> > otherwise use return true for the initial implementation (previous
> > patch)?
> 
> the reason is simple: in patch 2/3 I just wanted to show how to
> obtain
> the device xe device desc here to make some decisions based on that.
> 
> with plain "return true" in 2/3 I would have to mark desc var as
> "maybe_unused"
> 
> and OTOH leaving "return desc" here in 3/3 is redundant since we
> already might have de-referenced the pointer
> 
> so this one extra line in diff was IMO minimal cost to have two
> separate patches
> (as I didn't want to combine survivability_mode change with
> introduction of is_visible)

Yeah it makes sense to me too. Let's go with what you have here.

For the series:
Reviewed-by: Stuart Summers <stuart.summers@intel.com>

I was briefly wondering if it makes sense to keep the print message we
had from before, but given we're using the standard interface for
configfs now to check the visibility, any error handling should be done
at the interface definition point and not in the driver like we had
before with the explicit check. So looks good to me what you have.

Thanks,
Stuart

> 
> > 
> > Everything else in the series looks good to me. This does seem like
> > a
> > better way to approach this.
> 
> thanks,
> 
> > 
> > Thanks,
> > Stuart
> > 
> > >  }
> > >  
> > >  static struct configfs_group_operations
> > > xe_config_device_group_ops =
> > > {
> > > @@ -558,23 +563,6 @@ bool
> > > xe_configfs_get_survivability_mode(struct
> > > pci_dev *pdev)
> > >         return mode;
> > >  }
> > >  
> > > -/**
> > > - * xe_configfs_clear_survivability_mode - clear configfs
> > > survivability mode
> > > - * @pdev: pci device
> > > - */
> > > -void xe_configfs_clear_survivability_mode(struct pci_dev *pdev)
> > > -{
> > > -       struct xe_config_group_device *dev =
> > > find_xe_config_group_device(pdev);
> > > -
> > > -       if (!dev)
> > > -               return;
> > > -
> > > -       guard(mutex)(&dev->lock);
> > > -       dev->config.survivability_mode = 0;
> > > -
> > > -       config_group_put(&dev->group);
> > > -}
> > > -
> > >  /**
> > >   * xe_configfs_get_engines_allowed - get engine allowed mask
> > > from
> > > configfs
> > >   * @pdev: pci device
> > > diff --git a/drivers/gpu/drm/xe/xe_configfs.h
> > > b/drivers/gpu/drm/xe/xe_configfs.h
> > > index 58c8c3164000..1402e863b71c 100644
> > > --- a/drivers/gpu/drm/xe/xe_configfs.h
> > > +++ b/drivers/gpu/drm/xe/xe_configfs.h
> > > @@ -15,7 +15,6 @@ 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);
> > >  bool xe_configfs_get_psmi_enabled(struct pci_dev *pdev);
> > >  #else
> > > @@ -23,7 +22,6 @@ 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; }
> > >  static inline bool xe_configfs_get_psmi_enabled(struct pci_dev
> > > *pdev) { return false; }
> > >  #endif
> > > diff --git a/drivers/gpu/drm/xe/xe_survivability_mode.c
> > > b/drivers/gpu/drm/xe/xe_survivability_mode.c
> > > index 79426ea46861..19a1732e33d4 100644
> > > --- a/drivers/gpu/drm/xe/xe_survivability_mode.c
> > > +++ b/drivers/gpu/drm/xe/xe_survivability_mode.c
> > > @@ -287,19 +287,10 @@ bool
> > > xe_survivability_mode_is_requested(struct
> > > xe_device *xe)
> > >         u32 data;
> > >         bool survivability_mode;
> > >  
> > > -       if (!IS_DGFX(xe) || IS_SRIOV_VF(xe))
> > > +       if (!IS_DGFX(xe) || IS_SRIOV_VF(xe) || xe->info.platform
> > > <
> > > XE_BATTLEMAGE)
> > >                 return false;
> > >  
> > >         survivability_mode =
> > > xe_configfs_get_survivability_mode(pdev);
> > > -
> > > -       if (xe->info.platform < XE_BATTLEMAGE) {
> > > -               if (survivability_mode) {
> > > -                       dev_err(&pdev->dev, "Survivability Mode
> > > is
> > > not supported on this card\n");
> > > -
> > >                        xe_configfs_clear_survivability_mode(pdev);
> > > -               }
> > > -               return false;
> > > -       }
> > > -
> > >         /* Enable survivability mode if set via configfs */
> > >         if (survivability_mode)
> > >                 return true;
> > 
>