Re: [PATCH v1 3/3] drm/xe/drm_ras: Add per node cleanup action

[Raag Jadav <raag.jadav@intel.com>]

On Wed, May 20, 2026 at 12:53:52PM +0530, Tauro, Riana wrote:
> On 5/15/2026 1:58 AM, Raag Jadav wrote:
> > cleanup_node_param() is not registered in case of counter allocation
> > failure, which results in stale memory of previous node that isn't
> > cleaned up on unwind.
> 
> It is registered.
> 
> ret = assign_node_params(xe, node, i);
> if (ret)
>     cleanup_node_param(ras, i);

Is that also true for previous node params (in case second node
registration fails)?

> >   Add per node cleanup action which guarantees
> > cleanup on unwind and also simplifies the cleanup logic.
> > 
> > Fixes: b40db12b542f ("drm/xe/xe_drm_ras: Add support for XE DRM RAS")
> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_drm_ras.c | 42 +++++++++++++--------------------
> >   1 file changed, 17 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_drm_ras.c b/drivers/gpu/drm/xe/xe_drm_ras.c
> > index 89640ffb1c33..40abde29a26f 100644
> > --- a/drivers/gpu/drm/xe/xe_drm_ras.c
> > +++ b/drivers/gpu/drm/xe/xe_drm_ras.c
> > @@ -131,14 +131,20 @@ static int assign_node_params(struct xe_device *xe, struct drm_ras_node *node,
> >   	return 0;
> >   }
> > -static void cleanup_node_param(struct xe_drm_ras *ras, const enum drm_xe_ras_error_severity severity)
> > +static void cleanup_node_param(struct drm_ras_node *node)
> >   {
> > -	struct drm_ras_node *node = &ras->node[severity];
> > -
> >   	kfree(node->device_name);
> >   	node->device_name = NULL;
> >   }
> > +static void cleanup_node(struct drm_device *drm, void *arg)
> > +{
> > +	struct drm_ras_node *node = arg;
> > +
> > +	drm_ras_node_unregister(node);
> > +	cleanup_node_param(node);
> > +}
> > +
> >   static int register_nodes(struct xe_device *xe)
> >   {
> >   	struct xe_drm_ras *ras = &xe->ras;
> > @@ -150,13 +156,19 @@ static int register_nodes(struct xe_device *xe)
> >   		ret = assign_node_params(xe, node, i);
> >   		if (ret) {
> > -			cleanup_node_param(ras, i);
> > +			cleanup_node_param(node);
> 
> At this point drm_ras node is not registered.

Yes, and I don't believe we're attempting to unregister here :)

> >   			return ret;
> >   		}
> >   		ret = drm_ras_node_register(node);
> >   		if (ret) {
> > -			cleanup_node_param(ras, i);
> > +			cleanup_node_param(node);
> 
> Ditto

Ditto.

Raag

> > +			return ret;
> > +		}
> > +
> > +		ret = drmm_add_action_or_reset(&xe->drm, cleanup_node, node);
> > +		if (ret) {
> > +			cleanup_node(&xe->drm, node);
> >   			return ret;
> >   		}
> >   	}
> > @@ -164,20 +176,6 @@ static int register_nodes(struct xe_device *xe)
> >   	return 0;
> >   }
> > -static void xe_drm_ras_unregister_nodes(struct drm_device *device, void *arg)
> > -{
> > -	struct xe_device *xe = arg;
> > -	struct xe_drm_ras *ras = &xe->ras;
> > -	int i;
> > -
> > -	for_each_error_severity(i) {
> > -		struct drm_ras_node *node = &ras->node[i];
> > -
> > -		drm_ras_node_unregister(node);
> > -		cleanup_node_param(ras, i);
> > -	}
> > -}
> > -
> >   /**
> >    * xe_drm_ras_init() - Initialize DRM RAS
> >    * @xe: xe device instance
> > @@ -204,11 +202,5 @@ int xe_drm_ras_init(struct xe_device *xe)
> >   		return err;
> >   	}
> > -	err = drmm_add_action_or_reset(&xe->drm, xe_drm_ras_unregister_nodes, xe);
> > -	if (err) {
> > -		drm_err(&xe->drm, "Failed to add action for Xe DRM RAS (%pe)\n", ERR_PTR(err));
> > -		return err;
> > -	}
> > -
> >   	return 0;
> >   }