Re: [PATCH v3 05/16] accel/thames: Add driver for the C7x DSPs in TI SoCs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] accel/thames: persistent device link to rejected core in thames_core_init()
- [High] accel/thames: embedded mbox_dev lifetime and reuse in thames_platform_probe()
- [High] accel/thames: broken rproc auto_boot check and power leak in thames_core_init()
- [Medium] accel/thames: unconditional platform device creation in thames_register()
--

commit 33330c246e61374c538a20616d02df3f30a783af
Author: Tomeu Vizoso <tomeu.vizoso@gmail.com>

accel/thames: Add driver for the C7x DSPs in TI SoCs

This commit adds the initial DRM compute accelerator driver for Texas
Instruments C7x DSP cores on the J722S SoC. It discovers available DSP
cores via device tree, attaches to their remoteproc instances, performs a
shared-memory and mailbox doorbell handshake to validate firmware ABI and
capabilities, and exposes a device capability query ioctl to userspace.

> diff --git a/drivers/accel/thames/thames_core.c b/drivers/accel/thames/thames_core.c
> new file mode 100644
> index 0000000000000..d74c74b8f8ebf
> --- /dev/null
> +++ b/drivers/accel/thames/thames_core.c
[ ... ]
> +/* Embedded in struct thames_core: nothing to free. */
> +static void thames_core_mbox_dev_release(struct device *dev)
> +{
> +}
> +
> +/*
> + * A device of the driver's own carrying the DSP node, so the mailbox core finds
> + * the node's mboxes without borrowing the K3 driver's device.
> + */
> +static int thames_core_request_doorbell(struct thames_core *core)
> +{
> +	struct mbox_client *cl = &core->mbox_client;
> +	struct device *mbox_dev = &core->mbox_dev;
> +	int err;
> +
> +	device_initialize(mbox_dev);
> +	mbox_dev->parent = core->tdev->ddev.dev;
> +	mbox_dev->release = thames_core_mbox_dev_release;
> +	device_set_node(mbox_dev, of_fwnode_handle(core->of_node));
> +	dev_set_name(mbox_dev, "thames-mbox.%u", core->index);
> +
> +	err = device_add(mbox_dev);
> +	if (err) {
> +		put_device(mbox_dev);
> +		return err;
> +	}
> +
> +	cl->dev = mbox_dev;
> +	cl->rx_callback = thames_mbox_rx;
> +	cl->tx_block = false;
> +	cl->knows_txdone = false;
> +
> +	core->mbox_chan = mbox_request_channel(cl, 1);
> +	if (IS_ERR(core->mbox_chan)) {
> +		err = PTR_ERR(core->mbox_chan);
> +		core->mbox_chan = NULL;
> +		device_del(mbox_dev);
> +		put_device(mbox_dev);
> +		return err;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Can embedding struct device mbox_dev inside struct thames_core with an empty
thames_core_mbox_dev_release() callback corrupt kobject state or cause a
use-after-free?

In thames_core_init(), thames_core_request_doorbell() registers core->mbox_dev
via device_initialize() and device_add() before rproc_boot(),
thames_core_wait_handshake(), and thames_core_read_handshake(). If core 0
fails any of those steps (for example, when its firmware handshake is
refused), thames_core_free_doorbell() calls device_del() and put_device(), and
thames_platform_probe() in drivers/accel/thames/thames_drv.c immediately does:

	if (err) {
		dev_warn(&pdev->dev, "Leaving out the core at %pOF: %d\n", np, err);
		of_node_put(np);
		memset(&tdev->cores[core], 0, sizeof(tdev->cores[core]));
		continue;
	}

Without incrementing core, the next loop iteration zeroes and re-registers the
same tdev->cores[0].mbox_dev memory in-place via device_initialize() and
device_add(). Furthermore, tdev is allocated with devm_drm_dev_alloc(), so its
lifetime is tied to tdev->ddev rather than mbox_dev's kobject refcount. If a
sysfs reader, uevent handler, or CONFIG_DEBUG_KOBJECT_RELEASE still holds a
reference to mbox_dev->kobj after put_device(), zeroing and re-initializing
tdev->cores[0] or freeing tdev on unbind will access or overwrite live kobject
memory.

[ ... ]
> +int thames_core_init(struct thames_core *core)
> +{
> +	struct thames_caps caps = {};
> +	struct rproc *rproc;
> +	int err;
> +
> +	/* Without a phandle the lookup would defer forever. */
> +	if (!core->of_node->phandle) {
> +		dev_err(core->tdev->ddev.dev, "%pOF has no phandle to find its remoteproc by\n",
> +			core->of_node);
> +		return -EINVAL;
> +	}
> +
> +	rproc = rproc_get_by_phandle(core->of_node->phandle);
> +	if (!rproc)
> +		return dev_err_probe(core->tdev->ddev.dev, -EPROBE_DEFER,
> +				     "%pOF: no remoteproc registered for it yet\n", core->of_node);
> +
> +	/*
> +	 * remoteproc's own boot is still to come: booting the core here as well
> +	 * would leave two power counts, and a reset or a suspend could not stop
> +	 * it.
> +	 */
> +	if (rproc->state == RPROC_OFFLINE && rproc->auto_boot) {
> +		rproc_put(rproc);
> +		return dev_err_probe(core->tdev->ddev.dev, -EPROBE_DEFER,
> +				     "%pOF: remoteproc has not booted it yet (auto-boot pending)\n",
> +				     core->of_node);
> +	}
> +
> +	core->rproc = rproc;
> +	core->dev = rproc->dev.parent;
> +	snprintf(core->name, sizeof(core->name), "DSP core %s (index %u)", dev_name(core->dev),
> +		 core->index);
> +
> +	/*
> +	 * The PM core then suspends this driver before the DSP, and an unbind
> +	 * of the K3 driver unbinds this one first.
> +	 */
> +	if (!device_link_add(core->tdev->ddev.dev, core->dev, DL_FLAG_AUTOPROBE_CONSUMER)) {
> +		dev_err(core->tdev->ddev.dev, "Failed to link to %s\n", dev_name(core->dev));
> +		err = -EINVAL;
> +		goto err_put;
> +	}
> +
> +	err = thames_core_read_windows(core);
> +	if (err)
> +		goto err_put;
> +
> +	err = thames_core_get_timers(core);
> +	if (err)
> +		goto err_put;
> +
> +	err = thames_core_map_ipc(core);
> +	if (err)
> +		goto err_timers;
> +
> +	err = thames_core_request_doorbell(core);
> +	if (err)
> +		goto err_unmap;
> +
> +	if (rproc->state == RPROC_OFFLINE) {
> +		err = rproc_boot(rproc);
> +		if (err)
> +			goto err_doorbell;
> +	}
> +
> +	err = thames_core_wait_handshake(core);
> +	if (err)
> +		goto err_doorbell;
> +
> +	err = thames_core_read_handshake(core, &caps);
> +	if (err)
> +		goto err_doorbell;
> +
> +	if (!core->tdev->num_cores)
> +		core->tdev->caps = caps;
> +
> +	return 0;
> +
> +err_doorbell:
> +	thames_core_free_doorbell(core);
> +err_unmap:
> +	thames_core_unmap_ipc(core);
> +err_timers:
> +	thames_core_put_timers(core);
> +err_put:
> +	rproc_put(rproc);
> +	core->rproc = NULL;
> +	return err;
> +}

[Severity: High]
Does adding the managed device link via device_link_add() before validating
the core's resources and firmware handshake leave an active supplier link to a
rejected DSP core?

In thames_core_init(), device_link_add(core->tdev->ddev.dev, core->dev,
DL_FLAG_AUTOPROBE_CONSUMER) is called before thames_core_read_windows(),
thames_core_get_timers(), thames_core_map_ipc(),
thames_core_request_doorbell(), rproc_boot(), thames_core_wait_handshake(),
and thames_core_read_handshake(). If any of those steps fail (for instance,
when one of the two DSP cores runs non-Thames firmware and is refused at
handshake), the error path unwinds without removing the managed device link,
and thames_platform_probe() continues probing with the remaining DSP core.

When thames_platform_probe() succeeds using the other core,
device_links_driver_bound() activates all managed links on
core->tdev->ddev.dev, including the link to the refused core->dev. If the
user later unbinds the remoteproc driver from the refused DSP core,
device_links_unbind_consumers() will forcefully unbind the thames driver and
unplug the active DRM device, and device_links_check_suppliers() will block
thames from re-probing with -EPROBE_DEFER while that core's remoteproc remains
unbound.

[Severity: High]
In addition, how can the offline boot path in thames_core_init() be reached
given that rproc->auto_boot remains true?

In rproc_alloc(), rproc->auto_boot is initialized to true, and neither
k3_dsp_rproc_probe() nor the remoteproc core clears rproc->auto_boot after
rproc_trigger_auto_boot() completes, fails, or when a core is stopped via
sysfs. Because of the earlier check:

	if (rproc->state == RPROC_OFFLINE && rproc->auto_boot) {
		rproc_put(rproc);
		return dev_err_probe(core->tdev->ddev.dev, -EPROBE_DEFER, ...);
	}

any core in RPROC_OFFLINE causes thames_core_init() to return -EPROBE_DEFER
(which aborts thames_platform_probe() for all cores), making the subsequent
if (rproc->state == RPROC_OFFLINE) rproc_boot(rproc) block unreachable unless
rproc->state changes concurrently without rproc->lock. Even when
rproc_auto_boot_callback() is genuinely in flight, returning -EPROBE_DEFER
relies on driver_deferred_probe_trigger(), which is only invoked when a device
binds to a driver; since this firmware creates no virtio or remoteproc
subdevices, completing rproc_boot() will not re-trigger deferred probing.

Also, if rproc_boot(rproc) in thames_core_init() is reached via a race on
rproc->state and succeeds, but thames_core_wait_handshake() or
thames_core_read_handshake() then fails, jumping to err_doorbell drops the
rproc reference via rproc_put() without calling rproc_shutdown(), leaking the
rproc->power count. And in both thames_core_init() and thames_core_suspend(),
rproc->state is read without holding rproc->lock.

> diff --git a/drivers/accel/thames/thames_drv.c b/drivers/accel/thames/thames_drv.c
> new file mode 100644
> index 0000000000000..de93bc87756c3
> --- /dev/null
> +++ b/drivers/accel/thames/thames_drv.c
[ ... ]
> +static int __init thames_register(void)
> +{
> +	int ret;
> +
> +	drm_dev = platform_device_register_simple("thames", -1, NULL, 0);
> +	if (IS_ERR(drm_dev))
> +		return PTR_ERR(drm_dev);
> +
> +	ret = platform_driver_register(&thames_platform_driver);
> +	if (ret)
> +		platform_device_unregister(drm_dev);
> +
> +	return ret;
> +}

[Severity: Medium]
Should thames_register() check whether a compatible DSP node is available in
the device tree before calling platform_device_register_simple()?

On multiplatform kernels where CONFIG_DRM_ACCEL_THAMES is built-in or loaded
on a SoC without an available "ti,am62a-c7xv-dsp" node, thames_register()
unconditionally creates the "thames" platform device. This triggers
thames_platform_probe(), which logs an error ("No DSP core attached") and
fails with -ENODEV, while platform_driver_register() still returns 0 and
leaves the unbound platform device registered in sysfs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-thames-v3-0-3ab40aa17157@tomeuvizoso.net?part=5