Re: [PATCH v2 1/3] drm/xe/log: DO NOT REVIEW

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Corrected hardware errors are logged at ERROR level, triggering false alerts
- [Low] Missing `const` support for generic kernel device structs in type selection macros
- [Low] Composite component classes are unhandled in the unknown component fallback
--

--- Patch [1]: [PATCH v2 1/3] drm/xe/log: DO NOT REVIEW ---
commit c27ccbea00833a08d0fbc465493aa86a335d74ca
Author: Mallesh Koujalagi <[email protected]>
drm/xe/log: DO NOT REVIEW

This commit introduces the Xe SIGID logging framework. It defines the stable
error signature identifiers and implements the logging macros to emit
structured log entries.

Link: https://patchwork.freedesktop.org/series/171022/

> diff --git a/drivers/gpu/drm/xe/xe_any.h b/drivers/gpu/drm/xe/xe_any.h
> index 0000000000000..c49f87ccdb477
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_any.h

[ ... ]

> +/**
> + * xe_any_to_xe() - Obtain the &xe_device pointer.
> + * @any: the &pci_dev or the &xe_device or &xe_tile or &xe_gt pointer
> + *
> + * Return: the @xe_device pointer or backpointer.
> + */
> +#define xe_any_to_xe(any)							\
> +	_Generic((any),								\
> +		 __xe_any_to_self_assoc(struct xe_device, (any)),		\
> +		 __xe_any_to_other_assoc(/* */, xe_tile, xe_device, (any)),	\
> +		 __xe_any_to_other_assoc(const, xe_tile, xe_device, (any)),	\
> +		 __xe_any_to_other_assoc(/* */, xe_gt, xe_device, (any)),	\
> +		 __xe_any_to_other_assoc(const, xe_gt, xe_device, (any)),	\
> +		 __xe_any_to_other_assoc(, drm_device, xe_device, (any)),	\
> +		 __xe_any_to_other_assoc(, pci_dev, xe_device, (any)),		\
> +		 __xe_any_to_other_assoc(, device, xe_device, (any)))

[Severity: Low]
Is it possible to hit a compile-time error here if this macro is called
with a const struct pci_dev *, const struct device *, or const struct
drm_device *? The _Generic selection lacks the const variants for standard
device pointers, though they are present for the driver-specific structs.

> diff --git a/drivers/gpu/drm/xe/xe_log.c b/drivers/gpu/drm/xe/xe_log.c
> index 0000000000000..32fbf83fd9924
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_log.c

[ ... ]

> +static const char *log_unknown_component_prefix(u32 component)
> +{
> +	u32 class = FIELD_GET(XE_LOG_COMPONENT_CLASS_MASK, component);
> +	u32 type = FIELD_GET(XE_LOG_COMPONENT_TYPE_MASK, component);
> +
> +	WARN(IS_ENABLED(CONFIG_DRM_XE_DEBUG), "LOG: unrecognized component %u.%u\n", class, type);
> +	switch (class) {
> +#define MAKE_XE_LOG_COMPONENT_CLASS_PREFIX(_CLASS) \
> +	case XE_LOG_COMPONENT_CLASS_##_CLASS: return #_CLASS "? "
> +	MAKE_XE_LOG_COMPONENT_CLASS_PREFIX(SYSTEM);
> +	MAKE_XE_LOG_COMPONENT_CLASS_PREFIX(DRIVER);
> +	MAKE_XE_LOG_COMPONENT_CLASS_PREFIX(FEATURE);
> +	MAKE_XE_LOG_COMPONENT_CLASS_PREFIX(FIRMWARE);
> +	MAKE_XE_LOG_COMPONENT_CLASS_PREFIX(HARDWARE);
> +#undef MAKE_XE_LOG_COMPONENT_CLASS_PREFIX
> +	}
> +	return "COMP? ";
> +}

[Severity: Low]
Does this code handle composite component classes? It appears that
MAKE_XE_LOG_COMPONENT_CLASS_PREFIX is not instantiated for DRIVER_FEATURE,
DRIVER_FIRMWARE, or DRIVER_HARDWARE. If a dynamically constructed component ID
belongs to one of these classes, it looks like it will fall through to the
"COMP? " string instead of generating a class-specific prefix.

[ ... ]

> +static void log_dmesg_vprintk(struct pci_dev *pdev, int cper_sev, struct va_format *vaf)
> +{
> +	if (cper_sev == CPER_SEV_INFORMATIONAL)
> +		pci_info(pdev, __LOG_DRM_PRINTK_FMT("%pV", vaf));
> +	else
> +		pci_err(pdev, __LOG_DRM_PRINTK_ERR_FMT("%pV", vaf));
> +}

[Severity: Medium]
Will this incorrectly log successfully corrected hardware errors at ERROR level?
If a corrected fault is emitted via xe_log_emit_corrected() using
CPER_SEV_CORRECTED, it takes the else branch and uses pci_err() along with
the *ERROR* string format. Could this pollute dmesg and cause automated system
health monitoring tools to issue false hardware failure alerts?

> diff --git a/drivers/gpu/drm/xe/xe_log.h b/drivers/gpu/drm/xe/xe_log.h
> index 0000000000000..7b90e39b97be2
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_log.h

[ ... ]

> +#define xe_log_location_type(any)							\
> +	_Generic((any),									\
> +		 struct xe_gt * : XE_LOG_LOCATION_TYPE_GT,				\
> +		 const struct xe_gt * : XE_LOG_LOCATION_TYPE_GT,			\
> +		 struct xe_tile * : XE_LOG_LOCATION_TYPE_TILE,				\
> +		 const struct xe_tile * : XE_LOG_LOCATION_TYPE_TILE,			\
> +		 struct xe_device * : XE_LOG_LOCATION_TYPE_DEVICE,			\
> +		 const struct xe_device * : XE_LOG_LOCATION_TYPE_DEVICE,		\
> +		 struct pci_dev * : XE_LOG_LOCATION_TYPE_DEVICE,			\
> +		 struct device * : XE_LOG_LOCATION_TYPE_DEVICE)

[Severity: Low]
Does this code also omit the const variants for standard device pointers?
Like xe_any_to_xe(), if this macro is called with a const struct pci_dev * or
const struct device *, it looks like it will result in a compile-time error due
to an unhandled type in the _Generic selection.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1