Re: [PATCH V19 10/15] drm/xe/configfs: Add disable_vram_page_offline attribute

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `disable_vram_page_offline_show` function incorrectly negates the configured boolean value, causing sysfs/configfs reads to contradict the actual configuration state.
- [Medium] The dummy fallback implementation of `xe_configfs_get_disable_vram_page_offline()` incorrectly returns `true` when configfs is disabled, contradicting the default configuration.
--

commit d087a9cf3ce1baaaa80f9dce6c4475133cc05dbf
Author: Tejas Upadhyay <tejas.upadhyay@intel.com>

drm/xe/configfs: Add disable_vram_page_offline attribute

Add a new configfs attribute disable_vram_page_offline to control
how bad VRAM pages are handled. The attribute can only be set before
binding to the device and defaults to false (offlining enabled).

> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> --- a/drivers/gpu/drm/xe/xe_configfs.c
> +++ b/drivers/gpu/drm/xe/xe_configfs.c
[ ... ]
> @@ -616,6 +631,33 @@ static ssize_t enable_multi_queue_store(struct config_item *item, const char *pa
>  	return len;
>  }
>  
> +static ssize_t disable_vram_page_offline_show(struct config_item *item, char *page)
> +{
> +	struct xe_config_device *dev = to_xe_config_device(item);
> +
> +	return sprintf(page, "%s\n", str_yes_no(!dev->disable_vram_page_offline));

[Severity: Medium]
Does this logic negate the configured boolean value? Since the attribute
is named disable_vram_page_offline, if a user writes '1' to it (setting
it to true), won't the '!' operator cause this to output 'N' on read? This
could break userspace assumptions where reading yields the opposite of what
was just written.

> +}
> +
> +static ssize_t disable_vram_page_offline_store(struct config_item *item,
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_configfs.h b/drivers/gpu/drm/xe/xe_configfs.h
> --- a/drivers/gpu/drm/xe/xe_configfs.h
> +++ b/drivers/gpu/drm/xe/xe_configfs.h
[ ... ]
> @@ -44,6 +45,7 @@ static inline bool xe_configfs_media_gt_allowed(struct pci_dev *pdev) { return t
>  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 bool xe_configfs_get_disable_vram_page_offline(struct pci_dev *pdev) { return true; }

[Severity: Medium]
Should this dummy fallback return false instead of true? The commit message
and device_defaults indicate that the default state is false (offlining is
active). When the kernel is compiled without configfs support, returning
true here would silently disable VRAM page offlining by default.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831064942.315720-17-tejas.upadhyay@intel.com?part=10