Re: [PATCH 1/2] drm/xe/ras: Fix error handling in register_nodes()

["Tauro, Riana" <riana.tauro@intel.com>]

On 4/14/2026 6:00 AM, Lin, Shuicheng wrote:
> On Sat, Apr 11, 2026 12:56 AM Raag Jadav wrote:
>> On Tue, Apr 07, 2026 at 10:59:12PM +0000, Shuicheng Lin wrote:
>>> Fix two issues in register_nodes():
>>>
>>> 1. When the loop fails mid-way, previously registered nodes are not
>>>     cleaned up. Add goto-based error unwinding that walks backwards
>>>     through completed iterations.
>> Did you check with the author if this is expected behaviour?
> Author is in the Cc list.
>
>>> 2. When allocate_and_copy_counters() fails, assign_node_params()
>>>     leaves ras->info[severity] as an ERR_PTR and returns. The caller
>>>     then passes that ERR_PTR to kfree() via cleanup_node_param(),
>>>     causing an invalid free. Fix by making assign_node_params()
>>>     self-contained on error: NULL out the stale ERR_PTR and free
>>>     device_name before returning.
>> Can this be rather fixed using a local pointer?
> Yes, a local pointer could make the code simpler.
> As author will have new series, I would leave author to refine it.
> Thanks.
>

I did not mean that. I meant all the small cosmetic changes (Patch 2 in 
the series) can be part of any
bigger change.

Thanks
Riana

>
> Shuicheng
>
>> Raag
>>
>>> Fixes: b40db12b542f ("drm/xe/xe_drm_ras: Add support for XE DRM RAS")
>>> Cc: Riana Tauro <riana.tauro@intel.com>
>>> Assisted-by: Claude:claude-opus-4.6
>>> Signed-off-by: Shuicheng Lin <shuicheng.lin@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_drm_ras.c | 31 ++++++++++++++++++++++--------
>> -
>>>   1 file changed, 22 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_drm_ras.c
>>> b/drivers/gpu/drm/xe/xe_drm_ras.c index e07dc23a155e..802e4bcb731c
>>> 100644
>>> --- a/drivers/gpu/drm/xe/xe_drm_ras.c
>>> +++ b/drivers/gpu/drm/xe/xe_drm_ras.c
>>> @@ -73,6 +73,7 @@ static int assign_node_params(struct xe_device *xe,
>> struct drm_ras_node *node,
>>>   	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>>>   	struct xe_drm_ras *ras = &xe->ras;
>>>   	const char *device_name;
>>> +	int ret;
>>>
>>>   	device_name = kasprintf(GFP_KERNEL, "%04x:%02x:%02x.%d",
>>>   				pci_domain_nr(pdev->bus), pdev->bus-
>>> number, @@ -89,8 +90,11 @@
>>> static int assign_node_params(struct xe_device *xe, struct drm_ras_node
>> *node,
>>>   	node->priv = xe;
>>>
>>>   	ras->info[severity] = allocate_and_copy_counters(xe);
>>> -	if (IS_ERR(ras->info[severity]))
>>> -		return PTR_ERR(ras->info[severity]);
>>> +	if (IS_ERR(ras->info[severity])) {
>>> +		ret = PTR_ERR(ras->info[severity]);
>>> +		ras->info[severity] = NULL;
>>> +		goto err_free_name;
>>> +	}
>>>
>>>   	if (severity == DRM_XE_RAS_ERR_SEV_CORRECTABLE)
>>>   		node->query_error_counter =
>> query_correctable_error_counter; @@
>>> -98,6 +102,11 @@ static int assign_node_params(struct xe_device *xe,
>> struct drm_ras_node *node,
>>>   		node->query_error_counter =
>> query_uncorrectable_error_counter;
>>>   	return 0;
>>> +
>>> +err_free_name:
>>> +	kfree(device_name);
>>> +	node->device_name = NULL;
>>> +	return ret;
>>>   }
>>>
>>>   static void cleanup_node_param(struct xe_drm_ras *ras, const enum
>>> drm_xe_ras_error_severity severity) @@ -114,26 +123,30 @@ static void
>>> cleanup_node_param(struct xe_drm_ras *ras, const enum drm_xe_ras_err
>>> static int register_nodes(struct xe_device *xe)  {
>>>   	struct xe_drm_ras *ras = &xe->ras;
>>> -	int i;
>>> +	int i, ret;
>>>
>>>   	for_each_error_severity(i) {
>>>   		struct drm_ras_node *node = &ras->node[i];
>>> -		int ret;
>>>
>>>   		ret = assign_node_params(xe, node, i);
>>> -		if (ret) {
>>> -			cleanup_node_param(ras, i);
>>> -			return ret;
>>> -		}
>>> +		if (ret)
>>> +			goto err_unwind;
>>>
>>>   		ret = drm_ras_node_register(node);
>>>   		if (ret) {
>>>   			cleanup_node_param(ras, i);
>>> -			return ret;
>>> +			goto err_unwind;
>>>   		}
>>>   	}
>>>
>>>   	return 0;
>>> +
>>> +err_unwind:
>>> +	while (i--) {
>>> +		drm_ras_node_unregister(&ras->node[i]);
>>> +		cleanup_node_param(ras, i);
>>> +	}
>>> +	return ret;
>>>   }
>>>
>>>   static void xe_drm_ras_unregister_nodes(struct drm_device *device,
>>> void *arg)
>>> --
>>> 2.43.0
>>>