[PATCH 0/6] Make devres cleanup and component compatible

[PATCH 0/6] Make devres cleanup and component compatible

[Lucas De Marchi]

While trying to convert the xe driver probe sequence to use more devm, I
stumbled upon it not being compatible with component driver that is used
by xe to work with mei and audio.

First patch makes that possible with the 2nd and 3rd being some drive by
improvements.

The last 3 patches here are just to show it being used in xe,
but they require https://lore.kernel.org/intel-xe/20250212193600.475089-1-lucas.demarchi@intel.com/.
If this is acceptable I may even drop the first patch of that series and
convert it straight to devm rather than with an intermediate step.

Lucas De Marchi (6):
  drivers: base: devres: Allow to release group on device release
  drivers: base: devres: Fix find_group() documentation
  drivers: base: component: Add debug message for unbind
  drm/xe: Stop setting drvdata to NULL
  drm/xe: Switch from xe to devm actions
  drm/xe: Drop remove callback support

 drivers/base/component.c                |  3 +
 drivers/base/devres.c                   | 12 +++-
 drivers/gpu/drm/xe/display/xe_display.c |  4 +-
 drivers/gpu/drm/xe/xe_device.c          | 79 -------------------------
 drivers/gpu/drm/xe/xe_device.h          |  4 --
 drivers/gpu/drm/xe/xe_device_sysfs.c    |  6 --
 drivers/gpu/drm/xe/xe_device_types.h    | 17 ------
 drivers/gpu/drm/xe/xe_gsc_proxy.c       |  4 +-
 drivers/gpu/drm/xe/xe_pci.c             | 11 +---
 9 files changed, 20 insertions(+), 120 deletions(-)

-- 
2.48.1

[PATCH 1/6] drivers: base: devres: Allow to release group on device release

[Lucas De Marchi]

When releasing a device, if the release action causes a group to be
released, a warning is emitted because it can't find the group. This
happens because devres_release_all() moves the entire list to a todo
list and also move the group markers. Considering r* normal resource
nodes and g1 a group resource node:

		    g1 -----------.
		    v		  v
	r1 -> r2 -> g1[0] -> r3-> g[1] -> r4

After devres_release_all(), dev->devres_head becomes empty and the todo
list it iterates on becomes:

			       g1
			       v
	r1 -> r2 -> r3-> r4 -> g1[0]

When a call to component_del() is made and takes down the aggregate
device, a warning like this happen:

	RIP: 0010:devres_release_group+0x362/0x530
	...
	Call Trace:
	 <TASK>
	 component_unbind+0x156/0x380
	 component_unbind_all+0x1d0/0x270
	 mei_component_master_unbind+0x28/0x80 [mei_hdcp]
	 take_down_aggregate_device+0xc1/0x160
	 component_del+0x1c6/0x3e0
	 intel_hdcp_component_fini+0xf1/0x170 [xe]
	 xe_display_fini+0x1e/0x40 [xe]

Because the devres group corresponding to the hdcp component cannot be
found. Just ignore this corner case: if the dev->devres_head is empty
and the caller is trying to remove a group, it's likely in the process
of device cleanup so just ignore it instead of warning.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---

The check for just an empty devres_head seems simple and sufficient,
although the caller of devres_release_group() can still shoot its own
foot. I thought about a flag on dev to check it's being removed, but I
want to know others' opinions first.

 drivers/base/devres.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 93e7779ef21e8..b955a2f9520bf 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -687,6 +687,13 @@ int devres_release_group(struct device *dev, void *id)
 		spin_unlock_irqrestore(&dev->devres_lock, flags);
 
 		release_nodes(dev, &todo);
+	} else if (list_empty(&dev->devres_head)) {
+		/*
+		 * dev is probably dying via devres_release_all(): groups
+		 * have already been removed and are on the process of
+		 * being released - don't touch and don't warn.
+		 */
+		spin_unlock_irqrestore(&dev->devres_lock, flags);
 	} else {
 		WARN_ON(1);
 		spin_unlock_irqrestore(&dev->devres_lock, flags);
-- 
2.48.1

[PATCH 2/6] drivers: base: devres: Fix find_group() documentation

[Lucas De Marchi]

It returns the last open group, not the last group.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/base/devres.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index b955a2f9520bf..d8a733ea5e1ac 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -576,7 +576,10 @@ void *devres_open_group(struct device *dev, void *id, gfp_t gfp)
 }
 EXPORT_SYMBOL_GPL(devres_open_group);
 
-/* Find devres group with ID @id.  If @id is NULL, look for the latest. */
+/*
+ * Find devres group with ID @id.  If @id is NULL, look for the latest open
+ * group.
+ */
 static struct devres_group *find_group(struct device *dev, void *id)
 {
 	struct devres_node *node;
-- 
2.48.1

[PATCH 3/6] drivers: base: component: Add debug message for unbind

[Lucas De Marchi]

Like when binding component, add a debug message to the unbinding case
to make it easy to track the lifecycle. This also includes the component
pointer since that is used to open a group in devres, making it easier
to track the resources.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/base/component.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 741497324d78a..5d10600bbc25e 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -574,6 +574,9 @@ static void component_unbind(struct component *component,
 {
 	WARN_ON(!component->bound);
 
+	dev_dbg(adev->parent, "unbinding %s component %p (ops %ps)\n",
+		dev_name(component->dev), component, component->ops);
+
 	if (component->ops && component->ops->unbind)
 		component->ops->unbind(component->dev, adev->parent, data);
 	component->bound = false;
-- 
2.48.1

[PATCH 4/6] drm/xe: Stop setting drvdata to NULL

[Lucas De Marchi]

PCI subsystem is not supposed to call the remove() function when probe
fails and doesn't need a protection for that. The only places checking
for NULL drvdata, is on 2 sysfs files and they shouldn't be needed since
the files are removed and reads on open fds just return an error.

Remove the setting to NULL so it's possible to obtain the xe pointer
from callbacks like the component unbind from device_unbind_cleanup(),
i.e. after xe_pci_remove() already finished.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/xe/xe_device_sysfs.c | 6 ------
 drivers/gpu/drm/xe/xe_pci.c          | 7 +------
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.c b/drivers/gpu/drm/xe/xe_device_sysfs.c
index 7375937934fae..7efbd4c52791c 100644
--- a/drivers/gpu/drm/xe/xe_device_sysfs.c
+++ b/drivers/gpu/drm/xe/xe_device_sysfs.c
@@ -32,9 +32,6 @@ vram_d3cold_threshold_show(struct device *dev,
 	struct xe_device *xe = pdev_to_xe_device(pdev);
 	int ret;
 
-	if (!xe)
-		return -EINVAL;
-
 	xe_pm_runtime_get(xe);
 	ret = sysfs_emit(buf, "%d\n", xe->d3cold.vram_threshold);
 	xe_pm_runtime_put(xe);
@@ -51,9 +48,6 @@ vram_d3cold_threshold_store(struct device *dev, struct device_attribute *attr,
 	u32 vram_d3cold_threshold;
 	int ret;
 
-	if (!xe)
-		return -EINVAL;
-
 	ret = kstrtou32(buff, 0, &vram_d3cold_threshold);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 70b697fde5b96..41ec6825b9bcc 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -770,11 +770,7 @@ static int xe_info_init(struct xe_device *xe,
 
 static void xe_pci_remove(struct pci_dev *pdev)
 {
-	struct xe_device *xe;
-
-	xe = pdev_to_xe_device(pdev);
-	if (!xe) /* driver load aborted, nothing to cleanup */
-		return;
+	struct xe_device *xe = pdev_to_xe_device(pdev);
 
 	if (IS_SRIOV_PF(xe))
 		xe_pci_sriov_configure(pdev, 0);
@@ -784,7 +780,6 @@ static void xe_pci_remove(struct pci_dev *pdev)
 
 	xe_device_remove(xe);
 	xe_pm_runtime_fini(xe);
-	pci_set_drvdata(pdev, NULL);
 }
 
 /*
-- 
2.48.1

[PATCH 5/6] drm/xe: Switch from xe to devm actions

[Lucas De Marchi]

Now that component drivers are compatible with devm, switch to using it
instead of our own.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/xe/display/xe_display.c | 4 ++--
 drivers/gpu/drm/xe/xe_gsc_proxy.c       | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index bdcc56e51433f..302f533af037d 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -161,7 +161,7 @@ int xe_display_init_early(struct xe_device *xe)
 	return err;
 }
 
-static void xe_display_fini(struct xe_device *__xe, void *arg)
+static void xe_display_fini(void *arg)
 {
 	struct xe_device *xe = arg;
 	struct intel_display *display = &xe->display;
@@ -183,7 +183,7 @@ int xe_display_init(struct xe_device *xe)
 	if (err)
 		return err;
 
-	return xe_device_add_action_or_reset(xe, xe_display_fini, xe);
+	return devm_add_action_or_reset(xe->drm.dev, xe_display_fini, xe);
 }
 
 void xe_display_register(struct xe_device *xe)
diff --git a/drivers/gpu/drm/xe/xe_gsc_proxy.c b/drivers/gpu/drm/xe/xe_gsc_proxy.c
index 6aa76a7843cfa..8cf70b228ff3b 100644
--- a/drivers/gpu/drm/xe/xe_gsc_proxy.c
+++ b/drivers/gpu/drm/xe/xe_gsc_proxy.c
@@ -423,7 +423,7 @@ static int proxy_channel_alloc(struct xe_gsc *gsc)
 	return 0;
 }
 
-static void xe_gsc_proxy_remove(struct xe_device *__xe, void *arg)
+static void xe_gsc_proxy_remove(void *arg)
 {
 	struct xe_gsc *gsc = arg;
 	struct xe_gt *gt = gsc_to_gt(gsc);
@@ -490,7 +490,7 @@ int xe_gsc_proxy_init(struct xe_gsc *gsc)
 
 	gsc->proxy.component_added = true;
 
-	return xe_device_add_action_or_reset(xe, xe_gsc_proxy_remove, gsc);
+	return devm_add_action_or_reset(xe->drm.dev, xe_gsc_proxy_remove, gsc);
 }
 
 /**
-- 
2.48.1

[PATCH 6/6] drm/xe: Drop remove callback support

[Lucas De Marchi]

Now that devres supports component driver cleanup during driver removal
cleanup, the xe custom support for removal callbacks is not needed
anymore. Drop it.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/xe/xe_device.c       | 79 ----------------------------
 drivers/gpu/drm/xe/xe_device.h       |  4 --
 drivers/gpu/drm/xe/xe_device_types.h | 17 ------
 drivers/gpu/drm/xe/xe_pci.c          |  4 +-
 4 files changed, 1 insertion(+), 103 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 4b4039cf29fd4..d83400bbff8b1 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -65,12 +65,6 @@
 
 #include <generated/xe_wa_oob.h>
 
-struct xe_device_remove_action {
-	struct list_head node;
-	xe_device_remove_action_t remove;
-	void *data;
-};
-
 static int xe_file_open(struct drm_device *dev, struct drm_file *file)
 {
 	struct xe_device *xe = to_xe_device(dev);
@@ -752,9 +746,6 @@ int xe_device_probe(struct xe_device *xe)
 	int err;
 	u8 id;
 
-	xe->probing = true;
-	INIT_LIST_HEAD(&xe->remove_action_list);
-
 	xe_pat_init_early(xe);
 
 	err = xe_sriov_init(xe);
@@ -904,8 +895,6 @@ int xe_device_probe(struct xe_device *xe)
 
 	xe_vsec_init(xe);
 
-	xe->probing = false;
-
 	return devm_add_action_or_reset(xe->drm.dev, xe_device_sanitize, xe);
 
 err_unregister_display:
@@ -916,72 +905,6 @@ int xe_device_probe(struct xe_device *xe)
 	return err;
 }
 
-/**
- * xe_device_call_remove_actions - Call the remove actions
- * @xe: xe device instance
- *
- * This is only to be used by xe_pci and xe_device to call the remove actions
- * while removing the driver or handling probe failures.
- */
-void xe_device_call_remove_actions(struct xe_device *xe)
-{
-	struct xe_device_remove_action *ra, *tmp;
-
-	list_for_each_entry_safe(ra, tmp, &xe->remove_action_list, node) {
-		ra->remove(xe, ra->data);
-		list_del(&ra->node);
-		kfree(ra);
-	}
-
-	xe->probing = false;
-}
-
-/**
- * xe_device_add_action_or_reset - Add an action to run on driver removal
- * @xe: xe device instance
- * @ra: pointer to the object embedded into the object to cleanup
- * @remove: function to execute. The @ra is passed as argument
- *
- * Example:
- *
- * .. code-block:: c
- *
- *	static void foo_remove(struct xe_device_remove_action *ra)
- *	{
- *		struct xe_foo *foo = container_of(ra, struct xe_foo, remove_action);
- *		...
- *	}
- *
- *	int xe_foo_init(struct xe_foo *foo)
- *	{
- *		...
- *		xe_device_add_remove_action(xe, &foo->remove_action, foo_remove);
- *		...
- *		return 0;
- *	};
- */
-int xe_device_add_action_or_reset(struct xe_device *xe,
-				  xe_device_remove_action_t action,
-				  void *data)
-{
-	struct xe_device_remove_action *ra;
-
-	drm_WARN_ON(&xe->drm, !xe->probing);
-
-	ra = kmalloc(sizeof(*ra), GFP_KERNEL);
-	if (!ra) {
-		action(xe, data);
-		return -ENOMEM;
-	}
-
-	INIT_LIST_HEAD(&ra->node);
-	ra->remove = action;
-	ra->data = data;
-	list_add(&ra->node, &xe->remove_action_list);
-
-	return 0;
-}
-
 void xe_device_remove(struct xe_device *xe)
 {
 	xe_display_unregister(xe);
@@ -991,8 +914,6 @@ void xe_device_remove(struct xe_device *xe)
 	xe_display_driver_remove(xe);
 
 	xe_heci_gsc_fini(xe);
-
-	xe_device_call_remove_actions(xe);
 }
 
 void xe_device_shutdown(struct xe_device *xe)
diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
index a6fedf1ef3c7b..0bc3bc8e68030 100644
--- a/drivers/gpu/drm/xe/xe_device.h
+++ b/drivers/gpu/drm/xe/xe_device.h
@@ -45,10 +45,6 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
 				   const struct pci_device_id *ent);
 int xe_device_probe_early(struct xe_device *xe);
 int xe_device_probe(struct xe_device *xe);
-int xe_device_add_action_or_reset(struct xe_device *xe,
-				  xe_device_remove_action_t action,
-				  void *data);
-void xe_device_call_remove_actions(struct xe_device *xe);
 void xe_device_remove(struct xe_device *xe);
 void xe_device_shutdown(struct xe_device *xe);
 
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index b322d49c83c77..833c29fed3a37 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -35,7 +35,6 @@
 #include "intel_display_device.h"
 #endif
 
-struct xe_device;
 struct xe_ggtt;
 struct xe_pat_ops;
 struct xe_pxp;
@@ -71,8 +70,6 @@ struct xe_pxp;
 		 const struct xe_tile * : (const struct xe_device *)((tile__)->xe),	\
 		 struct xe_tile * : (tile__)->xe)
 
-typedef void (*xe_device_remove_action_t)(struct xe_device *xe, void *data);
-
 /**
  * struct xe_vram_region - memory region structure
  * This is used to describe a memory region in xe
@@ -431,20 +428,6 @@ struct xe_device {
 	/** @tiles: device tiles */
 	struct xe_tile tiles[XE_MAX_TILES_PER_DEVICE];
 
-	/**
-	 * @remove_action_list: list of actions to execute on device remove.
-	 * Use xe_device_add_remove_action() for that. Actions can only be added
-	 * during probe and are executed during the call from PCI subsystem to
-	 * remove the driver from the device.
-	 */
-	struct list_head remove_action_list;
-
-	/**
-	 * @probing: cover the section in which @remove_action_list can be used
-	 * to post cleaning actions
-	 */
-	bool probing;
-
 	/**
 	 * @mem_access: keep track of memory access in the device, possibly
 	 * triggering additional actions when they occur.
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 41ec6825b9bcc..447eacb355d7c 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -900,10 +900,8 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		return err;
 
 	err = xe_device_probe(xe);
-	if (err) {
-		xe_device_call_remove_actions(xe);
+	if (err)
 		return err;
-	}
 
 	err = xe_pm_init(xe);
 	if (err)
-- 
2.48.1

Re: [PATCH 1/6] drivers: base: devres: Allow to release group on device release

[Greg Kroah-Hartman]

On Wed, Feb 12, 2025 at 12:05:37PM -0800, Lucas De Marchi wrote:
> When releasing a device, if the release action causes a group to be
> released, a warning is emitted because it can't find the group. This
> happens because devres_release_all() moves the entire list to a todo
> list and also move the group markers. Considering r* normal resource
> nodes and g1 a group resource node:
> 
> 		    g1 -----------.
> 		    v		  v
> 	r1 -> r2 -> g1[0] -> r3-> g[1] -> r4
> 
> After devres_release_all(), dev->devres_head becomes empty and the todo
> list it iterates on becomes:
> 
> 			       g1
> 			       v
> 	r1 -> r2 -> r3-> r4 -> g1[0]
> 
> When a call to component_del() is made and takes down the aggregate
> device, a warning like this happen:
> 
> 	RIP: 0010:devres_release_group+0x362/0x530
> 	...
> 	Call Trace:
> 	 <TASK>
> 	 component_unbind+0x156/0x380
> 	 component_unbind_all+0x1d0/0x270
> 	 mei_component_master_unbind+0x28/0x80 [mei_hdcp]
> 	 take_down_aggregate_device+0xc1/0x160
> 	 component_del+0x1c6/0x3e0
> 	 intel_hdcp_component_fini+0xf1/0x170 [xe]
> 	 xe_display_fini+0x1e/0x40 [xe]
> 
> Because the devres group corresponding to the hdcp component cannot be
> found. Just ignore this corner case: if the dev->devres_head is empty
> and the caller is trying to remove a group, it's likely in the process
> of device cleanup so just ignore it instead of warning.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 3/6] drivers: base: component: Add debug message for unbind

[Greg Kroah-Hartman]

On Wed, Feb 12, 2025 at 12:05:39PM -0800, Lucas De Marchi wrote:
> Like when binding component, add a debug message to the unbinding case
> to make it easy to track the lifecycle. This also includes the component
> pointer since that is used to open a group in devres, making it easier
> to track the resources.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/6] drivers: base: devres: Fix find_group() documentation

[Greg Kroah-Hartman]

On Wed, Feb 12, 2025 at 12:05:38PM -0800, Lucas De Marchi wrote:
> It returns the last open group, not the last group.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/base/devres.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index b955a2f9520bf..d8a733ea5e1ac 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -576,7 +576,10 @@ void *devres_open_group(struct device *dev, void *id, gfp_t gfp)
>  }
>  EXPORT_SYMBOL_GPL(devres_open_group);
>  
> -/* Find devres group with ID @id.  If @id is NULL, look for the latest. */
> +/*
> + * Find devres group with ID @id.  If @id is NULL, look for the latest open
> + * group.
> + */
>  static struct devres_group *find_group(struct device *dev, void *id)
>  {
>  	struct devres_node *node;
> -- 
> 2.48.1
> 

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 1/6] drivers: base: devres: Allow to release group on device release

[Lucas De Marchi]

On Thu, Feb 20, 2025 at 01:24:20PM +0100, Greg Kroah-Hartman wrote:
>On Wed, Feb 12, 2025 at 12:05:37PM -0800, Lucas De Marchi wrote:
>> When releasing a device, if the release action causes a group to be
>> released, a warning is emitted because it can't find the group. This
>> happens because devres_release_all() moves the entire list to a todo
>> list and also move the group markers. Considering r* normal resource
>> nodes and g1 a group resource node:
>>
>> 		    g1 -----------.
>> 		    v		  v
>> 	r1 -> r2 -> g1[0] -> r3-> g[1] -> r4
>>
>> After devres_release_all(), dev->devres_head becomes empty and the todo
>> list it iterates on becomes:
>>
>> 			       g1
>> 			       v
>> 	r1 -> r2 -> r3-> r4 -> g1[0]
>>
>> When a call to component_del() is made and takes down the aggregate
>> device, a warning like this happen:
>>
>> 	RIP: 0010:devres_release_group+0x362/0x530
>> 	...
>> 	Call Trace:
>> 	 <TASK>
>> 	 component_unbind+0x156/0x380
>> 	 component_unbind_all+0x1d0/0x270
>> 	 mei_component_master_unbind+0x28/0x80 [mei_hdcp]
>> 	 take_down_aggregate_device+0xc1/0x160
>> 	 component_del+0x1c6/0x3e0
>> 	 intel_hdcp_component_fini+0xf1/0x170 [xe]
>> 	 xe_display_fini+0x1e/0x40 [xe]
>>
>> Because the devres group corresponding to the hdcp component cannot be
>> found. Just ignore this corner case: if the dev->devres_head is empty
>> and the caller is trying to remove a group, it's likely in the process
>> of device cleanup so just ignore it instead of warning.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>
>Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks. Is it ok to take these 3 through the drm tree or are you taking
it through yours?

Lucas De Marchi

Re: [PATCH 1/6] drivers: base: devres: Allow to release group on device release

[Greg Kroah-Hartman]

On Thu, Feb 20, 2025 at 05:48:10PM -0600, Lucas De Marchi wrote:
> On Thu, Feb 20, 2025 at 01:24:20PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Feb 12, 2025 at 12:05:37PM -0800, Lucas De Marchi wrote:
> > > When releasing a device, if the release action causes a group to be
> > > released, a warning is emitted because it can't find the group. This
> > > happens because devres_release_all() moves the entire list to a todo
> > > list and also move the group markers. Considering r* normal resource
> > > nodes and g1 a group resource node:
> > > 
> > > 		    g1 -----------.
> > > 		    v		  v
> > > 	r1 -> r2 -> g1[0] -> r3-> g[1] -> r4
> > > 
> > > After devres_release_all(), dev->devres_head becomes empty and the todo
> > > list it iterates on becomes:
> > > 
> > > 			       g1
> > > 			       v
> > > 	r1 -> r2 -> r3-> r4 -> g1[0]
> > > 
> > > When a call to component_del() is made and takes down the aggregate
> > > device, a warning like this happen:
> > > 
> > > 	RIP: 0010:devres_release_group+0x362/0x530
> > > 	...
> > > 	Call Trace:
> > > 	 <TASK>
> > > 	 component_unbind+0x156/0x380
> > > 	 component_unbind_all+0x1d0/0x270
> > > 	 mei_component_master_unbind+0x28/0x80 [mei_hdcp]
> > > 	 take_down_aggregate_device+0xc1/0x160
> > > 	 component_del+0x1c6/0x3e0
> > > 	 intel_hdcp_component_fini+0xf1/0x170 [xe]
> > > 	 xe_display_fini+0x1e/0x40 [xe]
> > > 
> > > Because the devres group corresponding to the hdcp component cannot be
> > > found. Just ignore this corner case: if the dev->devres_head is empty
> > > and the caller is trying to remove a group, it's likely in the process
> > > of device cleanup so just ignore it instead of warning.
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > 
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Thanks. Is it ok to take these 3 through the drm tree or are you taking
> it through yours?

As the drm patches depened on these, I figured they should all go
through the drm tree, so please feel free to take them.

thanks,

greg k-h

Re: [PATCH 6/6] drm/xe: Drop remove callback support

[Rodrigo Vivi]

On Wed, Feb 12, 2025 at 12:05:42PM -0800, Lucas De Marchi wrote:
> Now that devres supports component driver cleanup during driver removal
> cleanup, the xe custom support for removal callbacks is not needed
> anymore. Drop it.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c       | 79 ----------------------------
>  drivers/gpu/drm/xe/xe_device.h       |  4 --
>  drivers/gpu/drm/xe/xe_device_types.h | 17 ------
>  drivers/gpu/drm/xe/xe_pci.c          |  4 +-
>  4 files changed, 1 insertion(+), 103 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 4b4039cf29fd4..d83400bbff8b1 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -65,12 +65,6 @@
>  
>  #include <generated/xe_wa_oob.h>
>  
> -struct xe_device_remove_action {
> -	struct list_head node;
> -	xe_device_remove_action_t remove;
> -	void *data;
> -};
> -
>  static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>  {
>  	struct xe_device *xe = to_xe_device(dev);
> @@ -752,9 +746,6 @@ int xe_device_probe(struct xe_device *xe)
>  	int err;
>  	u8 id;
>  
> -	xe->probing = true;
> -	INIT_LIST_HEAD(&xe->remove_action_list);
> -
>  	xe_pat_init_early(xe);
>  
>  	err = xe_sriov_init(xe);
> @@ -904,8 +895,6 @@ int xe_device_probe(struct xe_device *xe)
>  
>  	xe_vsec_init(xe);
>  
> -	xe->probing = false;
> -
>  	return devm_add_action_or_reset(xe->drm.dev, xe_device_sanitize, xe);
>  
>  err_unregister_display:
> @@ -916,72 +905,6 @@ int xe_device_probe(struct xe_device *xe)
>  	return err;
>  }
>  
> -/**
> - * xe_device_call_remove_actions - Call the remove actions
> - * @xe: xe device instance
> - *
> - * This is only to be used by xe_pci and xe_device to call the remove actions
> - * while removing the driver or handling probe failures.
> - */
> -void xe_device_call_remove_actions(struct xe_device *xe)
> -{
> -	struct xe_device_remove_action *ra, *tmp;
> -
> -	list_for_each_entry_safe(ra, tmp, &xe->remove_action_list, node) {
> -		ra->remove(xe, ra->data);
> -		list_del(&ra->node);
> -		kfree(ra);
> -	}
> -
> -	xe->probing = false;
> -}
> -
> -/**
> - * xe_device_add_action_or_reset - Add an action to run on driver removal
> - * @xe: xe device instance
> - * @ra: pointer to the object embedded into the object to cleanup
> - * @remove: function to execute. The @ra is passed as argument
> - *
> - * Example:
> - *
> - * .. code-block:: c
> - *
> - *	static void foo_remove(struct xe_device_remove_action *ra)
> - *	{
> - *		struct xe_foo *foo = container_of(ra, struct xe_foo, remove_action);
> - *		...
> - *	}
> - *
> - *	int xe_foo_init(struct xe_foo *foo)
> - *	{
> - *		...
> - *		xe_device_add_remove_action(xe, &foo->remove_action, foo_remove);
> - *		...
> - *		return 0;
> - *	};
> - */
> -int xe_device_add_action_or_reset(struct xe_device *xe,
> -				  xe_device_remove_action_t action,
> -				  void *data)
> -{
> -	struct xe_device_remove_action *ra;
> -
> -	drm_WARN_ON(&xe->drm, !xe->probing);
> -
> -	ra = kmalloc(sizeof(*ra), GFP_KERNEL);
> -	if (!ra) {
> -		action(xe, data);
> -		return -ENOMEM;
> -	}
> -
> -	INIT_LIST_HEAD(&ra->node);
> -	ra->remove = action;
> -	ra->data = data;
> -	list_add(&ra->node, &xe->remove_action_list);
> -
> -	return 0;
> -}
> -
>  void xe_device_remove(struct xe_device *xe)
>  {
>  	xe_display_unregister(xe);
> @@ -991,8 +914,6 @@ void xe_device_remove(struct xe_device *xe)
>  	xe_display_driver_remove(xe);
>  
>  	xe_heci_gsc_fini(xe);
> -
> -	xe_device_call_remove_actions(xe);
>  }
>  
>  void xe_device_shutdown(struct xe_device *xe)
> diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
> index a6fedf1ef3c7b..0bc3bc8e68030 100644
> --- a/drivers/gpu/drm/xe/xe_device.h
> +++ b/drivers/gpu/drm/xe/xe_device.h
> @@ -45,10 +45,6 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>  				   const struct pci_device_id *ent);
>  int xe_device_probe_early(struct xe_device *xe);
>  int xe_device_probe(struct xe_device *xe);
> -int xe_device_add_action_or_reset(struct xe_device *xe,
> -				  xe_device_remove_action_t action,
> -				  void *data);
> -void xe_device_call_remove_actions(struct xe_device *xe);
>  void xe_device_remove(struct xe_device *xe);
>  void xe_device_shutdown(struct xe_device *xe);
>  
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index b322d49c83c77..833c29fed3a37 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -35,7 +35,6 @@
>  #include "intel_display_device.h"
>  #endif
>  
> -struct xe_device;
>  struct xe_ggtt;
>  struct xe_pat_ops;
>  struct xe_pxp;
> @@ -71,8 +70,6 @@ struct xe_pxp;
>  		 const struct xe_tile * : (const struct xe_device *)((tile__)->xe),	\
>  		 struct xe_tile * : (tile__)->xe)
>  
> -typedef void (*xe_device_remove_action_t)(struct xe_device *xe, void *data);
> -
>  /**
>   * struct xe_vram_region - memory region structure
>   * This is used to describe a memory region in xe
> @@ -431,20 +428,6 @@ struct xe_device {
>  	/** @tiles: device tiles */
>  	struct xe_tile tiles[XE_MAX_TILES_PER_DEVICE];
>  
> -	/**
> -	 * @remove_action_list: list of actions to execute on device remove.
> -	 * Use xe_device_add_remove_action() for that. Actions can only be added
> -	 * during probe and are executed during the call from PCI subsystem to
> -	 * remove the driver from the device.
> -	 */
> -	struct list_head remove_action_list;
> -
> -	/**
> -	 * @probing: cover the section in which @remove_action_list can be used
> -	 * to post cleaning actions
> -	 */
> -	bool probing;
> -
>  	/**
>  	 * @mem_access: keep track of memory access in the device, possibly
>  	 * triggering additional actions when they occur.
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 41ec6825b9bcc..447eacb355d7c 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -900,10 +900,8 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		return err;
>  
>  	err = xe_device_probe(xe);
> -	if (err) {
> -		xe_device_call_remove_actions(xe);
> +	if (err)
>  		return err;
> -	}
>  

For the series:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>  	err = xe_pm_init(xe);
>  	if (err)
> -- 
> 2.48.1
>