This driver registers the pci_tsm_ops with tsm subsystem.

Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
---
 drivers/virt/coco/Kconfig                |   2 +
 drivers/virt/coco/Makefile               |   1 +
 drivers/virt/coco/arm-cca-host/Kconfig   |  12 ++
 drivers/virt/coco/arm-cca-host/Makefile  |   5 +
 drivers/virt/coco/arm-cca-host/arm-cca.c | 209 +++++++++++++++++++++++
 drivers/virt/coco/arm-cca-host/rmm-da.h  |  29 ++++
 6 files changed, 258 insertions(+)
 create mode 100644 drivers/virt/coco/arm-cca-host/Kconfig
 create mode 100644 drivers/virt/coco/arm-cca-host/Makefile
 create mode 100644 drivers/virt/coco/arm-cca-host/arm-cca.c
 create mode 100644 drivers/virt/coco/arm-cca-host/rmm-da.h

diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
index 57248b088545..43e9508301bf 100644
--- a/drivers/virt/coco/Kconfig
+++ b/drivers/virt/coco/Kconfig
@@ -15,5 +15,7 @@ source "drivers/virt/coco/arm-cca-guest/Kconfig"
 
 source "drivers/virt/coco/guest/Kconfig"
 
+source "drivers/virt/coco/arm-cca-host/Kconfig"
+
 config TSM
 	tristate
diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
index 04e124b2d7cf..d0a859dd9eaf 100644
--- a/drivers/virt/coco/Makefile
+++ b/drivers/virt/coco/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_ARM_CCA_GUEST)	+= arm-cca-guest/
 
 obj-$(CONFIG_TSM) 		+= tsm-core.o
 obj-y				+= guest/
+obj-$(CONFIG_ARM_CCA_HOST)	+= arm-cca-host/
diff --git a/drivers/virt/coco/arm-cca-host/Kconfig b/drivers/virt/coco/arm-cca-host/Kconfig
new file mode 100644
index 000000000000..0f19fbf47613
--- /dev/null
+++ b/drivers/virt/coco/arm-cca-host/Kconfig
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# TSM (TEE Security Manager) host drivers
+#
+config ARM_CCA_HOST
+	tristate "Arm CCA Host driver"
+	depends on ARM64
+	depends on PCI_TSM
+	select TSM
+
+	help
+	  The driver provides TSM backend for ARM CCA
diff --git a/drivers/virt/coco/arm-cca-host/Makefile b/drivers/virt/coco/arm-cca-host/Makefile
new file mode 100644
index 000000000000..ad353b07e95a
--- /dev/null
+++ b/drivers/virt/coco/arm-cca-host/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+obj-$(CONFIG_ARM_CCA_HOST) += arm-cca-host.o
+
+arm-cca-host-$(CONFIG_TSM) +=  arm-cca.o
diff --git a/drivers/virt/coco/arm-cca-host/arm-cca.c b/drivers/virt/coco/arm-cca-host/arm-cca.c
new file mode 100644
index 000000000000..c8b0e6db1f47
--- /dev/null
+++ b/drivers/virt/coco/arm-cca-host/arm-cca.c
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2025 ARM Ltd.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/pci-tsm.h>
+#include <linux/pci-ide.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/tsm.h>
+#include <linux/vmalloc.h>
+
+#include "rmm-da.h"
+
+/* Number of streams that we can support at the hostbridge level */
+#define CCA_HB_PLATFORM_STREAMS 4
+
+/* Total number of stream id supported at root port level */
+#define MAX_STREAM_ID	256
+
+DEFINE_FREE(vfree, void *, if (!IS_ERR_OR_NULL(_T)) vfree(_T))
+static struct pci_tsm *cca_tsm_pci_probe(struct pci_dev *pdev)
+{
+	int rc;
+	struct pci_host_bridge *hb;
+	struct cca_host_dsc_pf0 *dsc_pf0 __free(vfree) = NULL;
+
+	if (pdev->is_virtfn)
+		return NULL;
+
+	if (!is_pci_tsm_pf0(pdev)) {
+		struct pci_tsm *tsm = kzalloc(sizeof(*tsm), GFP_KERNEL);
+
+		if (!tsm)
+			goto err_out;
+
+		pci_tsm_initialize(pdev, tsm);
+		return tsm;
+	}
+
+	if (!pdev->ide_cap)
+		goto err_out;
+
+	dsc_pf0 = vcalloc(sizeof(*dsc_pf0), GFP_KERNEL);
+	if (!dsc_pf0)
+		goto err_out;
+
+	rc = pci_tsm_pf0_initialize(pdev, &dsc_pf0->pci);
+	if (rc)
+		return NULL;
+	/*
+	 * FIXME!!
+	 * update the hostbridge details. This should go into
+	 * some host bridge probe/init routine.
+	 * than the selective index supported by the endpoint
+	 */
+	hb = pci_find_host_bridge(pdev->bus);
+	pci_ide_init_nr_streams(hb, CCA_HB_PLATFORM_STREAMS);
+
+	pci_info(pdev, "tsm enabled\n");
+	return &no_free_ptr(dsc_pf0)->pci.tsm;
+
+err_out:
+	return NULL;
+}
+
+static void cca_tsm_pci_remove(struct pci_tsm *tsm)
+{
+	struct pci_dev *pdev = tsm->pdev;
+	struct cca_host_dsc_pf0 *dsc_pf0;
+
+	if (WARN_ON(pdev->is_virtfn))
+		return;
+
+	if (!is_pci_tsm_pf0(pdev)) {
+
+		pci_dbg(tsm->pdev, "tsm disabled\n");
+		kfree(pdev->tsm);
+		return;
+	}
+
+	dsc_pf0 = to_cca_dsc_pf0(pdev);
+	pci_dbg(tsm->pdev, "tsm disabled\n");
+	vfree(dsc_pf0);
+}
+
+/* per root port unique with multiple restrictions. For now global */
+static DECLARE_BITMAP(cca_stream_ids, MAX_STREAM_ID);
+
+static int cca_tsm_connect(struct pci_dev *pdev)
+{
+	struct pci_dev *rp = pcie_find_root_port(pdev);
+	struct cca_host_dsc_pf0 *dsc_pf0;
+	struct pci_ide *ide;
+	int rc, stream_id;
+
+	/* Only function 0 supports connect in host */
+	if (WARN_ON(!is_pci_tsm_pf0(pdev)))
+		return -EIO;
+
+	dsc_pf0 = to_cca_dsc_pf0(pdev);
+	/* Allocate stream id */
+	stream_id = find_first_zero_bit(cca_stream_ids, MAX_STREAM_ID);
+	if (stream_id == MAX_STREAM_ID)
+		return -EBUSY;
+	set_bit(stream_id, cca_stream_ids);
+
+	ide = pci_ide_stream_alloc(pdev);
+	if (!ide) {
+		rc = -ENOMEM;
+		goto err_stream_alloc;
+	}
+
+	dsc_pf0->sel_stream = ide;
+	ide->stream_id = stream_id;
+	rc = pci_ide_stream_register(ide);
+	if (rc)
+		goto err_stream;
+
+	pci_ide_stream_setup(pdev, ide);
+	pci_ide_stream_setup(rp, ide);
+
+	rc = tsm_ide_stream_register(pdev, ide);
+	if (rc)
+		goto err_tsm;
+
+	/*
+	 * Once ide is setup enable the stream at endpoint
+	 * Root port will be done by RMM
+	 */
+	pci_ide_stream_enable(pdev, ide);
+	return 0;
+
+err_tsm:
+	pci_ide_stream_teardown(rp, ide);
+	pci_ide_stream_teardown(pdev, ide);
+	pci_ide_stream_unregister(ide);
+err_stream:
+	pci_ide_stream_free(ide);
+err_stream_alloc:
+	clear_bit(stream_id, cca_stream_ids);
+
+	return rc;
+}
+
+static void cca_tsm_disconnect(struct pci_dev *pdev)
+{
+	struct pci_dev *rp = pcie_find_root_port(pdev);
+	struct cca_host_dsc_pf0 *dsc_pf0;
+	struct pci_ide *ide;
+
+	if (WARN_ON(!is_pci_tsm_pf0(pdev)))
+		return;
+
+	dsc_pf0 = to_cca_dsc_pf0(pdev);
+	ide = dsc_pf0->sel_stream;
+	dsc_pf0->sel_stream = NULL;
+	pci_ide_stream_disable(pdev, ide);
+	tsm_ide_stream_unregister(ide);
+	pci_ide_stream_teardown(rp, ide);
+	pci_ide_stream_teardown(pdev, ide);
+	pci_ide_stream_unregister(ide);
+	clear_bit(ide->stream_id, cca_stream_ids);
+	pci_ide_stream_free(ide);
+}
+
+static const struct pci_tsm_ops cca_pci_ops = {
+	.probe = cca_tsm_pci_probe,
+	.remove = cca_tsm_pci_remove,
+	.connect = cca_tsm_connect,
+	.disconnect = cca_tsm_disconnect,
+};
+
+static void cca_tsm_remove(void *tsm_core)
+{
+	tsm_unregister(tsm_core);
+}
+
+static int cca_tsm_probe(struct platform_device *pdev)
+{
+	struct tsm_core_dev *tsm_core;
+
+	tsm_core = tsm_register(&pdev->dev, NULL, &cca_pci_ops);
+	if (IS_ERR(tsm_core))
+		return PTR_ERR(tsm_core);
+
+	return devm_add_action_or_reset(&pdev->dev, cca_tsm_remove, tsm_core);
+}
+
+static const struct platform_device_id arm_cca_host_id_table[] = {
+	{ RMI_DEV_NAME, 0},
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, arm_cca_host_id_table);
+
+
+static struct platform_driver cca_tsm_platform_driver = {
+	.probe = cca_tsm_probe,
+	.id_table = arm_cca_host_id_table,
+	.driver = {
+		.name = "cca_tsm",
+	},
+};
+
+MODULE_IMPORT_NS("PCI_IDE");
+module_platform_driver(cca_tsm_platform_driver);
+MODULE_DESCRIPTION("ARM CCA Host TSM driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/virt/coco/arm-cca-host/rmm-da.h b/drivers/virt/coco/arm-cca-host/rmm-da.h
new file mode 100644
index 000000000000..840cb584acdd
--- /dev/null
+++ b/drivers/virt/coco/arm-cca-host/rmm-da.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024 ARM Ltd.
+ */
+
+#ifndef RMM_DA_H_
+#define RMM_DA_H_
+
+#include <linux/pci.h>
+#include <linux/pci-ide.h>
+#include <linux/pci-tsm.h>
+#include <asm/rmi_smc.h>
+
+struct cca_host_dsc_pf0 {
+	struct pci_tsm_pf0 pci;
+	struct pci_ide *sel_stream;
+};
+
+static inline struct cca_host_dsc_pf0 *to_cca_dsc_pf0(struct pci_dev *pdev)
+{
+	struct pci_tsm *tsm = pdev->tsm;
+
+	if (!tsm || pdev->is_virtfn || !is_pci_tsm_pf0(pdev))
+		return NULL;
+
+	return container_of(tsm, struct cca_host_dsc_pf0, pci.tsm);
+}
+
+#endif
-- 
2.43.0

On Mon, 28 Jul 2025 19:21:49 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:

> This driver registers the pci_tsm_ops with tsm subsystem.
> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

Hi Aneesh,

For this main comment is around use of __free..  Dan wrote up guidance and
added to cleanup.h after many email threads kept running into same issues
and Linus added his requirements for that stuff to be acceptable.

Anyhow, easy to fix - comments inline.

> diff --git a/drivers/virt/coco/arm-cca-host/Kconfig b/drivers/virt/coco/arm-cca-host/Kconfig
> new file mode 100644
> index 000000000000..0f19fbf47613
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-host/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# TSM (TEE Security Manager) host drivers
> +#
> +config ARM_CCA_HOST
> +	tristate "Arm CCA Host driver"
> +	depends on ARM64
> +	depends on PCI_TSM
> +	select TSM
> +
> +	help
> +	  The driver provides TSM backend for ARM CCA

That's going to make for grumpy checkpatch!   More help.


> diff --git a/drivers/virt/coco/arm-cca-host/arm-cca.c b/drivers/virt/coco/arm-cca-host/arm-cca.c
> new file mode 100644
> index 000000000000..c8b0e6db1f47
> --- /dev/null
> +++ b/drivers/virt/coco/arm-cca-host/arm-cca.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025 ARM Ltd.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/pci-tsm.h>
> +#include <linux/pci-ide.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/tsm.h>
> +#include <linux/vmalloc.h>
cleanup.h and maybe others missing.  Basically follow include what you use principles
(flexed a little for headers that are front ends to others).

> +
> +#include "rmm-da.h"
> +
> +/* Number of streams that we can support at the hostbridge level */
> +#define CCA_HB_PLATFORM_STREAMS 4
> +
> +/* Total number of stream id supported at root port level */
> +#define MAX_STREAM_ID	256
> +
> +DEFINE_FREE(vfree, void *, if (!IS_ERR_OR_NULL(_T)) vfree(_T))
> +static struct pci_tsm *cca_tsm_pci_probe(struct pci_dev *pdev)
> +{
> +	int rc;
> +	struct pci_host_bridge *hb;
> +	struct cca_host_dsc_pf0 *dsc_pf0 __free(vfree) = NULL;

Read the stuff in cleanup.h and work out why this needs
changing to be inline below and not use this NULL pattern here
(unless you like grumpy Linus ;)

Note that with the err_out, even if you do that you'll still be
breaking with the guidance doc (and actually causing undefined
behavior :)  Get rid of those gotos if you want to use __free()


> +
> +	if (pdev->is_virtfn)
> +		return NULL;
> +
> +	if (!is_pci_tsm_pf0(pdev)) {
> +		struct pci_tsm *tsm = kzalloc(sizeof(*tsm), GFP_KERNEL);
> +
> +		if (!tsm)
> +			goto err_out;
> +
> +		pci_tsm_initialize(pdev, tsm);
> +		return tsm;
> +	}
> +
> +	if (!pdev->ide_cap)
> +		goto err_out;
> +
> +	dsc_pf0 = vcalloc(sizeof(*dsc_pf0), GFP_KERNEL);
> +	if (!dsc_pf0)
> +		goto err_out;
> +
> +	rc = pci_tsm_pf0_initialize(pdev, &dsc_pf0->pci);
> +	if (rc)
> +		return NULL;
> +	/*
> +	 * FIXME!!
> +	 * update the hostbridge details. This should go into
> +	 * some host bridge probe/init routine.
> +	 * than the selective index supported by the endpoint
> +	 */
> +	hb = pci_find_host_bridge(pdev->bus);
> +	pci_ide_init_nr_streams(hb, CCA_HB_PLATFORM_STREAMS);
> +
> +	pci_info(pdev, "tsm enabled\n");

Ok. RFC I guess.  Still pci_dbg()

> +	return &no_free_ptr(dsc_pf0)->pci.tsm;
> +
> +err_out:

Why? Random mix of direct returns of NULL above and goto here.

> +	return NULL;
> +}
> +
> +static void cca_tsm_pci_remove(struct pci_tsm *tsm)
> +{
> +	struct pci_dev *pdev = tsm->pdev;
> +	struct cca_host_dsc_pf0 *dsc_pf0;
> +
> +	if (WARN_ON(pdev->is_virtfn))
> +		return;
> +
> +	if (!is_pci_tsm_pf0(pdev)) {
> +
> +		pci_dbg(tsm->pdev, "tsm disabled\n");
> +		kfree(pdev->tsm);
> +		return;
> +	}
> +
> +	dsc_pf0 = to_cca_dsc_pf0(pdev);
> +	pci_dbg(tsm->pdev, "tsm disabled\n");
> +	vfree(dsc_pf0);
> +}
> +
> +/* per root port unique with multiple restrictions. For now global */
> +static DECLARE_BITMAP(cca_stream_ids, MAX_STREAM_ID);

> +
> +static void cca_tsm_disconnect(struct pci_dev *pdev)
> +{
> +	struct pci_dev *rp = pcie_find_root_port(pdev);
> +	struct cca_host_dsc_pf0 *dsc_pf0;
> +	struct pci_ide *ide;
> +
> +	if (WARN_ON(!is_pci_tsm_pf0(pdev)))
> +		return;
> +
> +	dsc_pf0 = to_cca_dsc_pf0(pdev);
> +	ide = dsc_pf0->sel_stream;
> +	dsc_pf0->sel_stream = NULL;
> +	pci_ide_stream_disable(pdev, ide);
> +	tsm_ide_stream_unregister(ide);
> +	pci_ide_stream_teardown(rp, ide);
> +	pci_ide_stream_teardown(pdev, ide);
> +	pci_ide_stream_unregister(ide);
> +	clear_bit(ide->stream_id, cca_stream_ids);
> +	pci_ide_stream_free(ide);

Ordering subtly different from error path above.
If there is a reason for that add a comment.

> +}

> +static void cca_tsm_remove(void *tsm_core)
> +{
> +	tsm_unregister(tsm_core);
> +}
> +
> +static int cca_tsm_probe(struct platform_device *pdev)
> +{
> +	struct tsm_core_dev *tsm_core;
> +
> +	tsm_core = tsm_register(&pdev->dev, NULL, &cca_pci_ops);
> +	if (IS_ERR(tsm_core))
> +		return PTR_ERR(tsm_core);
> +
> +	return devm_add_action_or_reset(&pdev->dev, cca_tsm_remove, tsm_core);

So this makes two with the one in Dan's test code. 
devm_tsm_register() seems to be a useful generic thing to add (implementation
being exactly what you have here.

> +}
> +
> +static const struct platform_device_id arm_cca_host_id_table[] = {
> +	{ RMI_DEV_NAME, 0},
Space before } and don't provide data until there is a use for it.
	{ RMI_DEV_NAME }

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, arm_cca_host_id_table);
> +
Consistency on spacing.  I'd go for just 1 blank line for separation
of things.
> +
> +static struct platform_driver cca_tsm_platform_driver = {
> +	.probe = cca_tsm_probe,
> +	.id_table = arm_cca_host_id_table,
> +	.driver = {
> +		.name = "cca_tsm",
> +	},
> +};
> +
> +MODULE_IMPORT_NS("PCI_IDE");
> +module_platform_driver(cca_tsm_platform_driver);
> +MODULE_DESCRIPTION("ARM CCA Host TSM driver");
> +MODULE_LICENSE("GPL");

Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:

> On Mon, 28 Jul 2025 19:21:49 +0530
> "Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:
>

...

>> +
>> +#include "rmm-da.h"
>> +
>> +/* Number of streams that we can support at the hostbridge level */
>> +#define CCA_HB_PLATFORM_STREAMS 4
>> +
>> +/* Total number of stream id supported at root port level */
>> +#define MAX_STREAM_ID	256
>> +
>> +DEFINE_FREE(vfree, void *, if (!IS_ERR_OR_NULL(_T)) vfree(_T))
>> +static struct pci_tsm *cca_tsm_pci_probe(struct pci_dev *pdev)
>> +{
>> +	int rc;
>> +	struct pci_host_bridge *hb;
>> +	struct cca_host_dsc_pf0 *dsc_pf0 __free(vfree) = NULL;
>
> Read the stuff in cleanup.h and work out why this needs
> changing to be inline below and not use this NULL pattern here
> (unless you like grumpy Linus ;)
>
> Note that with the err_out, even if you do that you'll still be
> breaking with the guidance doc (and actually causing undefined
> behavior :)  Get rid of those gotos if you want to use __free()
>
>

I’ve already fixed up similar cases by removing the goto based on cleanup.h
docs in other functions.I must have missed this one.

By the way, isn't using the `NULL` pattern acceptable when there are
no additional lock variables involved (ie, unwind order doesn't matter)?
Or should we always follow the pattern below regardless?

	struct cca_host_dsc_pf0 *dsc_pf0 __free(vfree) =
		vcalloc(sizeof(*dsc_pf0), GFP_KERNEL);

-aneesh

On Wed, 30 Jul 2025 14:28:55 +0530
Aneesh Kumar K.V <aneesh.kumar@kernel.org> wrote:

> Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:
> 
> > On Mon, 28 Jul 2025 19:21:49 +0530
> > "Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:
> >  
> 
> ...
> 
> >> +
> >> +#include "rmm-da.h"
> >> +
> >> +/* Number of streams that we can support at the hostbridge level */
> >> +#define CCA_HB_PLATFORM_STREAMS 4
> >> +
> >> +/* Total number of stream id supported at root port level */
> >> +#define MAX_STREAM_ID	256
> >> +
> >> +DEFINE_FREE(vfree, void *, if (!IS_ERR_OR_NULL(_T)) vfree(_T))
> >> +static struct pci_tsm *cca_tsm_pci_probe(struct pci_dev *pdev)
> >> +{
> >> +	int rc;
> >> +	struct pci_host_bridge *hb;
> >> +	struct cca_host_dsc_pf0 *dsc_pf0 __free(vfree) = NULL;  
> >
> > Read the stuff in cleanup.h and work out why this needs
> > changing to be inline below and not use this NULL pattern here
> > (unless you like grumpy Linus ;)
> >
> > Note that with the err_out, even if you do that you'll still be
> > breaking with the guidance doc (and actually causing undefined
> > behavior :)  Get rid of those gotos if you want to use __free()
> >
> >  
> 
> I’ve already fixed up similar cases by removing the goto based on cleanup.h
> docs in other functions.I must have missed this one.
> 
> By the way, isn't using the `NULL` pattern acceptable when there are
> no additional lock variables involved (ie, unwind order doesn't matter)?
> Or should we always follow the pattern below regardless?
> 
> 	struct cca_host_dsc_pf0 *dsc_pf0 __free(vfree) =
> 		vcalloc(sizeof(*dsc_pf0), GFP_KERNEL);

Always do this.  It's not really about what happens today but
more what we might break by failing to notice a future patch causes
problems.  Keeping the unwind ordering tightly couple with setup
means we basically can't get it wrong (famous last words ;)

Jonathan


> 
> -aneesh

On Tue, Jul 29, 2025 at 06:22:44PM +0100, Jonathan Cameron wrote:
> > +}
> 
> > +static void cca_tsm_remove(void *tsm_core)
> > +{
> > +	tsm_unregister(tsm_core);
> > +}
> > +
> > +static int cca_tsm_probe(struct platform_device *pdev)
> > +{
> > +	struct tsm_core_dev *tsm_core;
> > +
> > +	tsm_core = tsm_register(&pdev->dev, NULL, &cca_pci_ops);
> > +	if (IS_ERR(tsm_core))
> > +		return PTR_ERR(tsm_core);
> > +
> > +	return devm_add_action_or_reset(&pdev->dev, cca_tsm_remove, tsm_core);
> 
> So this makes two with the one in Dan's test code. 
> devm_tsm_register() seems to be a useful generic thing to add (implementation
> being exactly what you have here.

Pelase no, this is insane, you have a probed driver with a
probe/remove function pairing already. Why on earth would you use devm
just to call a remove function :(

Just put tsm_unregister() in the normal driver remove like it is
supposed to be done and use the drvdata to pass the tsm_core_dev
pointer. It is easy and normal, look at fwctl for a very simple
example.

devm is useful to solve complex things, these trivial things should be
done normally..

Jason

On Tue, 29 Jul 2025 20:22:43 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Tue, Jul 29, 2025 at 06:22:44PM +0100, Jonathan Cameron wrote:
> > > +}  
> >   
> > > +static void cca_tsm_remove(void *tsm_core)
> > > +{
> > > +	tsm_unregister(tsm_core);
> > > +}
> > > +
> > > +static int cca_tsm_probe(struct platform_device *pdev)
> > > +{
> > > +	struct tsm_core_dev *tsm_core;
> > > +
> > > +	tsm_core = tsm_register(&pdev->dev, NULL, &cca_pci_ops);
> > > +	if (IS_ERR(tsm_core))
> > > +		return PTR_ERR(tsm_core);
> > > +
> > > +	return devm_add_action_or_reset(&pdev->dev, cca_tsm_remove, tsm_core);  
> > 
> > So this makes two with the one in Dan's test code. 
> > devm_tsm_register() seems to be a useful generic thing to add (implementation
> > being exactly what you have here.  
> 
> Pelase no, this is insane, you have a probed driver with a
> probe/remove function pairing already. Why on earth would you use devm
> just to call a remove function :(
> 
> Just put tsm_unregister() in the normal driver remove like it is
> supposed to be done and use the drvdata to pass the tsm_core_dev
> pointer. It is easy and normal, look at fwctl for a very simple
> example.
> 
> devm is useful to solve complex things, these trivial things should be
> done normally..

Sure, that would be fine for now.  If we end up with a large complex flow that
happens to have a tsm_register() in amongst various managed resources
we can revisit.  If they all end up looking like this then a manual call
in remove is fine.

> 
> Jason

On Wed, Jul 30, 2025 at 11:28:04AM +0100, Jonathan Cameron wrote:
> > devm is useful to solve complex things, these trivial things should be
> > done normally..
> 
> Sure, that would be fine for now.  If we end up with a large complex flow that
> happens to have a tsm_register() in amongst various managed resources
> we can revisit.  If they all end up looking like this then a manual call
> in remove is fine.

IMHO just don't use devm, it is so easy to use devm wrong and get out
of order clean up. It works well for extremely simple case where 100%
of cleanup is in devm (but then it is questionable if the overhead is
worthwehile), and it is necessary for extremely hard cases where
writing a manual unwind is too hard. But the middle ground it tends to
just make ordering bugs and not provide alot of value, IMHO.

Jason