Re: [PATCH v5 2/2] drm/xe/guc: Add prefix to guc log buffer macros

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 10/21/2025 11:13 PM, Dong, Zhanjun wrote:
> 
> 
> On 2025-10-21 1:49 p.m., Michal Wajdeczko wrote:
>>
>>
>> On 10/9/2025 11:57 PM, Zhanjun Dong wrote:
...

>>> +#define GUC_LOG_SIZE ((SZ_4K) + \
>>> +              (XE_GUC_LOG_EVENT_DATA_BUFFER_SIZE) + \
>>> +              (XE_GUC_LOG_CRASH_DUMP_BUFFER_SIZE) + \
>>> +              (XE_GUC_LOG_STATE_CAPTURE_BUFFER_SIZE))
>>
>> too many ( ), only one set is needed to wrap all additions
> 
> will change it to
> +#define GUC_LOG_SIZE (SZ_4K) + \
> +              (XE_GUC_LOG_EVENT_DATA_BUFFER_SIZE) + \
> +              (XE_GUC_LOG_CRASH_DUMP_BUFFER_SIZE) + \
> +              (XE_GUC_LOG_STATE_CAPTURE_BUFFER_SIZE)
> 
> I follow the rule that add () to all macros referenced as operand within an expression.
> This way if the operand macro was change in the future, the logic in this macro, like operator precedence will not be effected.
> 

but it is the responsibility of each macro definition to provide consistent outcome

while it is possible, it would be an huge mistake to define something like this:

	#define SZ_1K	1023 + 1

as that would break not just your code, but 1000's other places

so IMO GUC_LOG_SIZE should be defined as:

	#define GUC_LOG_SIZE (SZ_4K + \
		XE_GUC_LOG_EVENT_DATA_BUFFER_SIZE + \
		XE_GUC_LOG_CRASH_DUMP_BUFFER_SIZE + \
		XE_GUC_LOG_STATE_CAPTURE_BUFFER_SIZE)

and just make sure that all other XE_GUC_LOG_xxx macros follows the same BKM