Re: [PATCH 1/2] platform: arm64: Add Radxa SVC GLINK driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The rpmsg callback can be invoked before driver probe initializes data, leading to a NULL pointer dereference.
- [Medium] A spurious completion from a timed-out request can incorrectly fulfill a subsequent request due to `complete()` being called outside `rsp_lock`.
- [Medium] Strict response size checking rejects valid larger responses from newer firmware, breaking forward compatibility.
- [Low] The driver is placed in `drivers/platform/arm64/` instead of `drivers/hwmon/` despite its primary function being hardware monitoring.
- [Low] Redundant custom locking used for hwmon sysfs attributes.
--

commit 5b64da1a0118d30a0e8b618d6109d99215fbbd93
Author: Xilin Wu <sophon@radxa.com>

platform: arm64: Add Radxa SVC GLINK driver

This commit adds an rpmsg client driver for the RADXA_SVC_ADSP_APPS firmware 
service on Radxa boards with Qualcomm SoCs. It exposes fan control 
functionality via the standard hwmon interface.

> diff --git a/drivers/platform/arm64/Kconfig b/drivers/platform/arm64/Kconfig
> index e32e01b2a9bdd..60c1e541fe899 100644
> --- a/drivers/platform/arm64/Kconfig
> +++ b/drivers/platform/arm64/Kconfig
[ ... ]
> +config RADXA_SVC_GLINK
> +	tristate "Radxa SVC GLINK driver"
> +	depends on ARCH_QCOM || COMPILE_TEST

[Severity: Low]
Should this driver be located in drivers/hwmon/ instead? HWMON guidelines
mandate that drivers whose primary functionality is hardware monitoring must
reside in drivers/hwmon/. Placing it in the platform directory bypasses
proper maintainer review and violates subsystem layering, given that this
driver exclusively registers a hardware monitoring device for fan control.

> +	depends on RPMSG
> +	depends on HWMON

[ ... ]
> diff --git a/drivers/platform/arm64/radxa_svc_glink.c b/drivers/platform/arm64/radxa_svc_glink.c
> new file mode 100644
> index 0000000000000..1c91bfabcd126
> --- /dev/null
> +++ b/drivers/platform/arm64/radxa_svc_glink.c
[ ... ]
> +static int radxa_svc_request(struct radxa_svc_glink *svc, u32 opcode,
> +			     const void *req_payload, size_t req_len,
> +			     void *rsp_payload, size_t *rsp_len)
> +{
[ ... ]
> +	if (req_len)
> +		memcpy(tx_buf + sizeof(*hdr), req_payload, req_len);
> +
> +	reinit_completion(&svc->rsp);
> +
> +	spin_lock_irqsave(&svc->rsp_lock, flags);

[Severity: Medium]
Can this reinit_completion() race with a delayed completion from a previous
timed-out request? If a request times out, radxa_svc_request() cleans up and
returns, but if the rpmsg callback thread had already set do_complete = true
under lock and got preempted before calling complete(), that delayed complete()
will execute shortly after. When a new request starts and calls
reinit_completion(), the delayed complete() from the previous transaction
would incorrectly fulfill this new request with stale payload data.

> +	if (svc->shutting_down) {
> +		spin_unlock_irqrestore(&svc->rsp_lock, flags);
[ ... ]
> +	spin_lock_irqsave(&svc->rsp_lock, flags);
> +	if (svc->shutting_down) {
> +		ret = -ENODEV;
> +	} else if (svc->rsp_opcode != opcode) {
> +		ret = -EIO;
> +	} else {
> +		ret = svc->rsp_status;
> +		if (rsp_payload && rsp_len) {
> +			size_t copy_len = min(*rsp_len, svc->rsp_len);
> +
> +			memcpy(rsp_payload, svc->rsp_payload, copy_len);
> +			if (*rsp_len < svc->rsp_len && !ret)
> +				ret = -EMSGSIZE;

[Severity: Medium]
Will this strict size check break forward compatibility? If a newer firmware
version appends additional fields to the response, svc->rsp_len will be
greater than *rsp_len. Changing ret to -EMSGSIZE here will cause the driver
to fail rather than safely ignoring the trailing unknown bytes.

> +			*rsp_len = svc->rsp_len;
> +		}
> +	}
> +	spin_unlock_irqrestore(&svc->rsp_lock, flags);
[ ... ]
> +static int radxa_svc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
> +				    int len, void *priv, u32 addr)
> +{
> +	struct radxa_svc_glink *svc = dev_get_drvdata(&rpdev->dev);
> +	const struct radxa_svc_hdr *hdr = data;
> +	unsigned long flags;
[ ... ]
> +	seq = le32_to_cpu(hdr->seq);
> +
> +	spin_lock_irqsave(&svc->rsp_lock, flags);

[Severity: High]
Could this result in a NULL pointer dereference during early initialization?
Because radxa_svc_rpmsg_driver sets the callback in its definition, the rpmsg
core can activate the endpoint and invoke this callback before the driver's
probe function completes. If an unsolicited firmware message arrives before
dev_set_drvdata() is executed in radxa_svc_rpmsg_probe(), dev_get_drvdata()
will return NULL here, causing a crash when dereferencing svc->rsp_lock.

> +	if (svc->pending && seq == svc->pending_seq) {
> +		svc->rsp_opcode = le32_to_cpu(hdr->opcode);
> +		svc->rsp_status = (s32)le32_to_cpu(hdr->status);
> +		svc->rsp_len = payload_len;
> +		memcpy(svc->rsp_payload, data + header_size, payload_len);
> +		svc->pending = false;
> +		do_complete = true;
> +	}
> +	spin_unlock_irqrestore(&svc->rsp_lock, flags);
> +
> +	if (do_complete)
> +		complete(&svc->rsp);

[Severity: Medium]
This isn't a bug in this exact location, but calling complete() outside the
spinlock here allows it to race with the reinit_completion() of a subsequent
request as detailed above.

> +
> +	return 0;
[ ... ]
> +static int radxa_svc_hwmon_read(struct device *dev,
> +				enum hwmon_sensor_types type, u32 attr,
> +				int channel, long *val)
> +{
> +	struct radxa_svc_glink *svc = dev_get_drvdata(dev);
> +	struct radxa_svc_fan_state_resp state = {};
[ ... ]
> +	case hwmon_pwm_enable:
> +		mutex_lock(&svc->fan_lock);

[Severity: Low]
Is this custom locking redundant? The hardware monitoring subsystem core
automatically serializes sysfs operations for attributes registered via the
info parameter of hwmon_device_register_with_info(). Relying on the hwmon core
for serialization avoids API layering violations.

> +		ret = radxa_svc_fan_get_pwm_mode(svc, val);
> +		mutex_unlock(&svc->fan_lock);
> +
> +		return ret;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int radxa_svc_hwmon_write(struct device *dev,
> +				 enum hwmon_sensor_types type, u32 attr,
> +				 int channel, long val)
> +{
> +	struct radxa_svc_glink *svc = dev_get_drvdata(dev);
> +	struct radxa_svc_fan_control_resp control = {};
> +	int ret;
> +
> +	if (type != hwmon_pwm || channel)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&svc->fan_lock);

[Severity: Low]
Like above, this custom fan_lock is likely redundant since the hwmon core
already serializes sysfs writes.

> +
> +	switch (attr) {
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-radxa-svc-v1-0-7c028de6a387@radxa.com?part=1