* [PATCH v2 1/3] drm/drm_ras: Remove admin-perm flag from list-nodes operation
2026-10-29 20:29 [PATCH v2 0/3] Extend drm_ras for qaic telemetry Zack McKevitt
@ 2026-10-29 20:29 ` Zack McKevitt
2026-10-30 16:37 ` Jeff Hugo
2026-10-29 20:29 ` [PATCH v2 2/3] drm/drm_ras: Extend DRM RAS interface to include telemetry nodes Zack McKevitt
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Zack McKevitt @ 2026-10-29 20:29 UTC (permalink / raw)
To: dri-devel, netdev, riana.tauro, aravind.iddamsetty, rodrigo.vivi,
raag.jadav, simona.vetter, airlied, kuba, ogabbay, lizhi.hou,
karol.wachowski
Cc: jeff.hugo, carl.vanderlip, troy.hanson, youssef.abdulrahman,
Zack McKevitt
Drivers may want to avoid requiring CAP_NET_ADMIN for subsequent
drm-ras operations. However, userspace must still be able to access
the list of RAS nodes to determine the node IDs. So, drop the
CAP_NET_ADMIN requirement for dumping RAS nodes so that drivers
with different permission requirements are still able to use the
interface.
Additionally, initialize the return value to zero in
drm_ras_nl_list_nodes_dumpit to avoid leaking uninitialized kernel
memory (especailly now that this function no longer requires admin
permissions).
This prepares the interface to be used for telemetry by the qaic
driver.
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>
---
Documentation/netlink/specs/drm_ras.yaml | 1 -
drivers/gpu/drm/drm_ras.c | 2 +-
drivers/gpu/drm/drm_ras_nl.c | 2 +-
3 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/Documentation/netlink/specs/drm_ras.yaml b/Documentation/netlink/specs/drm_ras.yaml
index 4c2ac9a1ba3f..a88fdab7a1c7 100644
--- a/Documentation/netlink/specs/drm_ras.yaml
+++ b/Documentation/netlink/specs/drm_ras.yaml
@@ -112,7 +112,6 @@ operations:
the node IDs. These IDs are required for all subsequent
operations on nodes, such as querying error counters.
attribute-set: node-attrs
- flags: [admin-perm]
dump:
reply:
attributes:
diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c
index b099eb67836e..4f4777060a4e 100644
--- a/drivers/gpu/drm/drm_ras.c
+++ b/drivers/gpu/drm/drm_ras.c
@@ -130,7 +130,7 @@ int drm_ras_nl_list_nodes_dumpit(struct sk_buff *skb,
struct drm_ras_node *node;
struct nlattr *hdr;
unsigned long id;
- int ret;
+ int ret = 0;
xa_for_each_start(&drm_ras_xa, id, node, ctx->restart) {
hdr = genlmsg_iput(skb, info);
diff --git a/drivers/gpu/drm/drm_ras_nl.c b/drivers/gpu/drm/drm_ras_nl.c
index c9f5e0ceb3b5..ff0dc406c27c 100644
--- a/drivers/gpu/drm/drm_ras_nl.c
+++ b/drivers/gpu/drm/drm_ras_nl.c
@@ -46,7 +46,7 @@ static const struct genl_split_ops drm_ras_nl_ops[] = {
{
.cmd = DRM_RAS_CMD_LIST_NODES,
.dumpit = drm_ras_nl_list_nodes_dumpit,
- .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
+ .flags = GENL_CMD_CAP_DUMP,
},
{
.cmd = DRM_RAS_CMD_GET_ERROR_COUNTER,
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/3] drm/drm_ras: Remove admin-perm flag from list-nodes operation
2026-10-29 20:29 ` [PATCH v2 1/3] drm/drm_ras: Remove admin-perm flag from list-nodes operation Zack McKevitt
@ 2026-10-30 16:37 ` Jeff Hugo
0 siblings, 0 replies; 10+ messages in thread
From: Jeff Hugo @ 2026-10-30 16:37 UTC (permalink / raw)
To: Zack McKevitt, dri-devel, netdev, riana.tauro, aravind.iddamsetty,
rodrigo.vivi, raag.jadav, simona.vetter, airlied, kuba, ogabbay,
lizhi.hou, karol.wachowski
Cc: carl.vanderlip, troy.hanson, youssef.abdulrahman
On 10/29/2026 2:29 PM, Zack McKevitt wrote:
> Drivers may want to avoid requiring CAP_NET_ADMIN for subsequent
> drm-ras operations. However, userspace must still be able to access
> the list of RAS nodes to determine the node IDs. So, drop the
> CAP_NET_ADMIN requirement for dumping RAS nodes so that drivers
> with different permission requirements are still able to use the
> interface.
>
> Additionally, initialize the return value to zero in
> drm_ras_nl_list_nodes_dumpit to avoid leaking uninitialized kernel
> memory (especailly now that this function no longer requires admin
> permissions).
>
> This prepares the interface to be used for telemetry by the qaic
> driver.
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>
Reviewed-by: Jeff Hugo <jeff.hugo@oss.qualcomm.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] drm/drm_ras: Extend DRM RAS interface to include telemetry nodes
2026-10-29 20:29 [PATCH v2 0/3] Extend drm_ras for qaic telemetry Zack McKevitt
2026-10-29 20:29 ` [PATCH v2 1/3] drm/drm_ras: Remove admin-perm flag from list-nodes operation Zack McKevitt
@ 2026-10-29 20:29 ` Zack McKevitt
2026-10-30 16:45 ` Jeff Hugo
2026-10-30 20:30 ` sashiko-bot
2026-10-29 20:30 ` [PATCH v2 3/3] accel/qaic: Add callbacks for DRM RAS netlink telemetry Zack McKevitt
2026-10-30 16:36 ` [PATCH v2 0/3] Extend drm_ras for qaic telemetry Jeff Hugo
3 siblings, 2 replies; 10+ messages in thread
From: Zack McKevitt @ 2026-10-29 20:29 UTC (permalink / raw)
To: dri-devel, netdev, riana.tauro, aravind.iddamsetty, rodrigo.vivi,
raag.jadav, simona.vetter, airlied, kuba, ogabbay, lizhi.hou,
karol.wachowski
Cc: jeff.hugo, carl.vanderlip, troy.hanson, youssef.abdulrahman,
Zack McKevitt
Add a new node type, attribute set, and doit/dump functions for
performing telemetry read and write operations using the DRM RAS
interface.
ynl-regen.sh was used to regenerate drm_ras_nl.c, drm_ras_nl.h, and
include/uapi/drm/drm_ras.h from the drm_ras.yaml spec.
Signed-off-by: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>
---
Documentation/netlink/specs/drm_ras.yaml | 82 +++++-
drivers/gpu/drm/drm_ras.c | 308 ++++++++++++++++++++++-
drivers/gpu/drm/drm_ras_nl.c | 53 ++++
drivers/gpu/drm/drm_ras_nl.h | 9 +
include/drm/drm_ras.h | 57 +++++
include/uapi/drm/drm_ras.h | 19 +-
6 files changed, 514 insertions(+), 14 deletions(-)
diff --git a/Documentation/netlink/specs/drm_ras.yaml b/Documentation/netlink/specs/drm_ras.yaml
index a88fdab7a1c7..a02aaed8772f 100644
--- a/Documentation/netlink/specs/drm_ras.yaml
+++ b/Documentation/netlink/specs/drm_ras.yaml
@@ -16,11 +16,11 @@ definitions:
type: enum
name: node-type
value-start: 1
- entries: [error-counter]
+ entries: [error-counter, telemetry]
doc: >-
- Type of the node. Currently, only error-counter nodes are
- supported, which expose reliability counters for a hardware/software
- component.
+ Type of the node. The error-counter nodes expose reliability counters
+ for a hardware/software component, and telemetry nodes expose an
+ interface for reading and writing telemetry values to a device.
attribute-sets:
-
@@ -100,6 +100,34 @@ attribute-sets:
name: error-value
type: u32
doc: Current value of the error counter.
+ -
+ name: telemetry-attrs
+ attributes:
+ -
+ name: node-id
+ type: u32
+ doc: >-
+ Unique identifier for the node.
+ Assigned dynamically by the DRM RAS core upon registration.
+ -
+ name: field
+ type: u32
+ doc: Telemetry field to be read/written, e.g., SoC temp.
+ -
+ name: field-name
+ type: string
+ doc: Name of the telemetry field defined by the device's uapi.
+ -
+ name: value
+ type: sint
+ doc: Value of telemetry field that was read/written.
+ -
+ name: status
+ type: s32
+ doc: >-
+ Status of the telemetry operation.
+ Populated by the device in response to the performed
+ telemetry operation. Errors may be hardware specific.
operations:
list:
@@ -198,6 +226,52 @@ operations:
- node-id
- error-id
- error-threshold
+ -
+ name: telemetry-get
+ doc: Perform a read of the specified telemetry field.
+ attribute-set: telemetry-attrs
+ do:
+ pre: drm-ras-nl-pre-telemetry
+ request:
+ attributes:
+ - node-id
+ - field
+ reply:
+ attributes:
+ - value
+ - status
+ dump:
+ request:
+ attributes:
+ - node-id
+ reply:
+ attributes: &telemetry-field-attrs
+ - field
+ - field-name
+ -
+ name: telemetry-set
+ doc: >-
+ Perform a write of the specified telemetry field.
+ Telemetry writes are device specific, and may be used to set
+ device limits or other capacities, for example.
+ attribute-set: telemetry-attrs
+ flags: [admin-perm]
+ do:
+ pre: drm-ras-nl-pre-telemetry
+ request:
+ attributes:
+ - node-id
+ - field
+ - value
+ reply:
+ attributes:
+ - status
+ dump:
+ request:
+ attributes:
+ - node-id
+ reply:
+ attributes: *telemetry-field-attrs
mcast-groups:
list:
diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c
index 4f4777060a4e..5b9c00f28834 100644
--- a/drivers/gpu/drm/drm_ras.c
+++ b/drivers/gpu/drm/drm_ras.c
@@ -53,6 +53,15 @@
* 7. SET_ERROR_THRESHOLD: Set error threshold of a given counter.
* Userspace must provide Node ID, Error ID and threshold to be set.
*
+ * 8. TELEMETRY_GET: Read a telemetry value.
+ * Userspace must provide Node ID and telemetry field to be read.
+ * Returns the status of the read and the value read (if valid).
+ *
+ * 9. TELEMETRY_SET: Write a telemetry value.
+ * Userspace must provide Node ID, the telemetry field to write, and the value
+ * to be written.
+ * Returns the status of the write.
+ *
* Node registration:
*
* - drm_ras_node_register(): Registers a new node and assigns
@@ -63,7 +72,6 @@
* Node type:
*
* - ERROR_COUNTER:
- * + Currently, only error counters are supported.
* + The driver must implement the query_error_counter() callback to provide
* the name and the value of the error counter.
* + The driver must provide a error_counter_range.last value informing the
@@ -81,6 +89,15 @@
* that are raised by the hardware.
* + The driver is responsible for error threshold bounds checking.
*
+ * - TELEMETRY:
+ * + The driver must implement the telemetry_get() callbacks, telemetry_set()
+ * is optional.
+ * + The driver must provide a list of gettable telemetry fields corresponding
+ * to their uAPI definitions and values. Settable telemetry fields are also
+ * required if providing telemetry_set().
+ * + The driver can optionally provide a pre_doit callback, telemetry_pre(),
+ * to enforce permissions checks on telemetry operations.
+ *
* Netlink handlers:
*
* - drm_ras_nl_list_nodes_dumpit(): Implements the LIST_NODES
@@ -95,6 +112,16 @@
* operation, fetching the error threshold of a specific counter.
* - drm_ras_nl_set_error_threshold_doit(): Implements the SET_ERROR_THRESHOLD doit
* operation, setting the error threshold of a specific counter.
+ * - drm_ras_nl_pre_telemetry(): Implements the TELEMETRY_READ and TELEMETRY_WRITE
+ * pre_doit operation, returning 0 if valid.
+ * - drm_ras_nl_telemetry_get_dumpit(): Implements the TELEMETRY_GET dumpit
+ * operation, fetching all readable telemetry values for a telemetry node.
+ * - drm_ras_nl_telemetry_get_doit(): Implements the TELEMETRY_GET doit operation,
+ * invoking the driver's telemetry_get callback and returning the results.
+ * - drm_ras_nl_telemetry_set_dumpit(): Implements the TELEMETRY_SET dumpit
+ * operation, fetching all writeable telemetry values for a telemetry node.
+ * - drm_ras_nl_telemetry_set_doit(): Implements the TELEMETRY_SET doit operation,
+ * invoking the driver's telemetry_set callback and returning the results.
*/
static DEFINE_XARRAY_ALLOC(drm_ras_xa);
@@ -617,6 +644,264 @@ int drm_ras_nl_set_error_threshold_doit(struct sk_buff *skb, struct genl_info *i
return set_node_error_threshold(node_id, error_id, threshold);
}
+/**
+ * drm_ras_nl_pre_telemetry - Enforce permission checks
+ * @ops: Generic netlink operations
+ * @skb: Netlink message buffer
+ * @info: Generic Netlink info containing attributes of the request
+ *
+ * Enforces a telemetry node's pre_doit conditions (if they exist).
+ *
+ * Returns 0 if no pre_doit is specified, otherwise returns the pre_doit.
+ */
+int drm_ras_nl_pre_telemetry(const struct genl_split_ops *ops, struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct drm_ras_node *node;
+ u32 node_id;
+
+ if (!info->attrs || GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID))
+ return -EINVAL;
+
+ node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]);
+ node = xa_load(&drm_ras_xa, node_id);
+ if (!node)
+ return -ENOENT;
+
+ if (node->telemetry_pre)
+ return node->telemetry_pre(node);
+
+ return 0;
+}
+
+static int telemetry_fields_dumpit(struct sk_buff *skb, struct netlink_callback *cb,
+ const struct drm_ras_telemetry_field *fields)
+{
+ const struct genl_info *info = genl_info_dump(cb);
+ struct drm_ras_ctx *ctx = (void *)cb->ctx;
+ const struct drm_ras_telemetry_field *field;
+ struct nlattr *hdr;
+ unsigned long idx;
+ int ret = 0;
+
+ if (!fields)
+ return -EOPNOTSUPP;
+
+ for (idx = ctx->restart; fields[idx].name; idx++) {
+ field = &fields[idx];
+
+ hdr = genlmsg_iput(skb, info);
+ if (!hdr) {
+ ret = -EMSGSIZE;
+ break;
+ }
+
+ ret = nla_put_u32(skb, DRM_RAS_A_TELEMETRY_ATTRS_FIELD, field->value);
+ if (ret) {
+ genlmsg_cancel(skb, hdr);
+ break;
+ }
+
+ ret = nla_put_string(skb, DRM_RAS_A_TELEMETRY_ATTRS_FIELD_NAME, field->name);
+ if (ret) {
+ genlmsg_cancel(skb, hdr);
+ break;
+ }
+
+ genlmsg_end(skb, hdr);
+ }
+
+ if (ret == -EMSGSIZE)
+ ctx->restart = idx;
+
+ return ret;
+}
+
+/**
+ * drm_ras_nl_telemetry_get_dumpit() - Dump readable telemetry fields.
+ * @skb: Netlink message buffer
+ * @cb: Callback context for multi-part dumps
+ *
+ * Displays the set of readable telemetry fields that is specified by
+ * the device for the given node.
+ *
+ * Return: 0 on success, or negative errno on failure.
+ */
+int drm_ras_nl_telemetry_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+{
+ const struct genl_info *info = genl_info_dump(cb);
+ struct drm_ras_node *node;
+ u32 node_id;
+
+ if (!info->attrs || GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID))
+ return -EINVAL;
+
+ node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]);
+
+ node = xa_load(&drm_ras_xa, node_id);
+ if (!node)
+ return -ENOENT;
+
+ return telemetry_fields_dumpit(skb, cb, node->telemetry_fields_gettable);
+}
+
+/**
+ * drm_ras_nl_telemetry_get_doit() - Query a telemetry value
+ * @skb: Netlink message buffer
+ * @info: Generic Netlink info containing attributes of the request
+ *
+ * Extracts the telemetry field from the netlink attributes and
+ * reads that value from the device.
+ *
+ * Return: 0 on success, or negative errno on failure.
+ */
+int drm_ras_nl_telemetry_get_doit(struct sk_buff *skb, struct genl_info *info)
+{
+ struct drm_ras_node *node;
+ struct sk_buff *msg;
+ struct nlattr *hdr;
+ u32 node_id, field;
+ s32 status = 0;
+ s64 value = 0;
+ int ret = 0;
+
+ if (!info->attrs ||
+ GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID) ||
+ GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_FIELD))
+ return -EINVAL;
+
+ field = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_FIELD]);
+ node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]);
+
+ node = xa_load(&drm_ras_xa, node_id);
+ if (!node || !node->telemetry_get)
+ return -ENOENT;
+
+ msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ hdr = genlmsg_iput(msg, info);
+ if (!hdr) {
+ nlmsg_free(msg);
+ return -EMSGSIZE;
+ }
+
+ ret = node->telemetry_get(node, &field, &value, &status);
+ if (ret) {
+ nlmsg_free(msg);
+ return ret;
+ }
+
+ ret = nla_put_s32(msg, DRM_RAS_A_TELEMETRY_ATTRS_STATUS, status);
+ if (ret) {
+ genlmsg_cancel(msg, hdr);
+ nlmsg_free(msg);
+ return ret;
+ }
+
+ ret = nla_put_sint(msg, DRM_RAS_A_TELEMETRY_ATTRS_VALUE, value);
+ if (ret) {
+ genlmsg_cancel(msg, hdr);
+ nlmsg_free(msg);
+ return ret;
+ }
+
+ genlmsg_end(msg, hdr);
+
+ return genlmsg_reply(msg, info);
+}
+
+/**
+ * drm_ras_nl_telemetry_set_dumpit() - Dump writeable telemetry fields.
+ * @skb: Netlink message buffer
+ * @cb: Callback context for multi-part dumps
+ *
+ * Displays the set of writeable telemetry fields that is specified by
+ * the device for the given node.
+ *
+ * Return: 0 on success, or negative errno on failure.
+ */
+int drm_ras_nl_telemetry_set_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
+{
+ const struct genl_info *info = genl_info_dump(cb);
+ struct drm_ras_node *node;
+ u32 node_id;
+
+ if (!info->attrs || GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID))
+ return -EINVAL;
+
+ node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]);
+
+ node = xa_load(&drm_ras_xa, node_id);
+ if (!node)
+ return -ENOENT;
+
+ return telemetry_fields_dumpit(skb, cb, node->telemetry_fields_settable);
+}
+
+/**
+ * drm_ras_nl_telemetry_set_doit() - Write a telemetry value
+ * @skb: Netlink message buffer
+ * @info: Generic Netlink info containing attributes of the request
+ *
+ * Extracts the telemetry field and value to be written from the netlink
+ * attributes and writes that value to the specified field on the device.
+ *
+ * Return: 0 on success, or negative errno on failure.
+ */
+int drm_ras_nl_telemetry_set_doit(struct sk_buff *skb, struct genl_info *info)
+{
+ struct drm_ras_node *node;
+ struct sk_buff *msg;
+ struct nlattr *hdr;
+ u32 node_id, field;
+ s32 status = 0;
+ s64 value = 0;
+ int ret = 0;
+
+ if (!info->attrs ||
+ GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID) ||
+ GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_VALUE) ||
+ GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_FIELD))
+ return -EINVAL;
+
+ value = nla_get_sint(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_VALUE]);
+ field = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_FIELD]);
+ node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]);
+
+ node = xa_load(&drm_ras_xa, node_id);
+ if (!node || !node->telemetry_set)
+ return -ENOENT;
+
+ msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ hdr = genlmsg_iput(msg, info);
+ if (!hdr) {
+ nlmsg_free(msg);
+ return -EMSGSIZE;
+ }
+
+ ret = node->telemetry_set(node, &field, &value, &status);
+ if (ret) {
+ nlmsg_free(msg);
+ return ret;
+ }
+
+ ret = nla_put_s32(msg, DRM_RAS_A_TELEMETRY_ATTRS_STATUS, status);
+ if (ret) {
+ genlmsg_cancel(msg, hdr);
+ nlmsg_free(msg);
+ return ret;
+ }
+
+ genlmsg_end(msg, hdr);
+
+ return genlmsg_reply(msg, info);
+}
+
/**
* drm_ras_node_register() - Register a new RAS node
* @node: Node structure to register
@@ -631,14 +916,21 @@ int drm_ras_node_register(struct drm_ras_node *node)
if (!node->device_name || !node->node_name)
return -EINVAL;
- /* Currently, only Error Counter Endpoints are supported */
- if (node->type != DRM_RAS_NODE_TYPE_ERROR_COUNTER)
- return -EINVAL;
-
- /* Mandatory entries for Error Counter Node */
- if (node->type == DRM_RAS_NODE_TYPE_ERROR_COUNTER &&
- (!node->error_counter_range.last || !node->query_error_counter))
+ /* Currently, only Error Counter and Telemetry endpoints are supported */
+ switch (node->type) {
+ case DRM_RAS_NODE_TYPE_ERROR_COUNTER:
+ /* Mandatory entries for Error Counter Node */
+ if (!node->error_counter_range.last || !node->query_error_counter)
+ return -EINVAL;
+ break;
+ case DRM_RAS_NODE_TYPE_TELEMETRY:
+ /* Must provide telemetry_get, telemetry_set is optional */
+ if (!node->telemetry_get || !node->telemetry_fields_gettable)
+ return -EINVAL;
+ break;
+ default:
return -EINVAL;
+ }
return xa_alloc(&drm_ras_xa, &node->id, node, xa_limit_32b, GFP_KERNEL);
}
diff --git a/drivers/gpu/drm/drm_ras_nl.c b/drivers/gpu/drm/drm_ras_nl.c
index ff0dc406c27c..19e3683fb35a 100644
--- a/drivers/gpu/drm/drm_ras_nl.c
+++ b/drivers/gpu/drm/drm_ras_nl.c
@@ -41,6 +41,29 @@ static const struct nla_policy drm_ras_set_error_threshold_nl_policy[DRM_RAS_A_E
[DRM_RAS_A_ERROR_COUNTER_ATTRS_ERROR_THRESHOLD] = { .type = NLA_U32, },
};
+/* DRM_RAS_CMD_TELEMETRY_GET - do */
+static const struct nla_policy drm_ras_telemetry_get_do_nl_policy[DRM_RAS_A_TELEMETRY_ATTRS_FIELD + 1] = {
+ [DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID] = { .type = NLA_U32, },
+ [DRM_RAS_A_TELEMETRY_ATTRS_FIELD] = { .type = NLA_U32, },
+};
+
+/* DRM_RAS_CMD_TELEMETRY_GET - dump */
+static const struct nla_policy drm_ras_telemetry_get_dump_nl_policy[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID + 1] = {
+ [DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID] = { .type = NLA_U32, },
+};
+
+/* DRM_RAS_CMD_TELEMETRY_SET - do */
+static const struct nla_policy drm_ras_telemetry_set_do_nl_policy[DRM_RAS_A_TELEMETRY_ATTRS_VALUE + 1] = {
+ [DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID] = { .type = NLA_U32, },
+ [DRM_RAS_A_TELEMETRY_ATTRS_FIELD] = { .type = NLA_U32, },
+ [DRM_RAS_A_TELEMETRY_ATTRS_VALUE] = { .type = NLA_SINT, },
+};
+
+/* DRM_RAS_CMD_TELEMETRY_SET - dump */
+static const struct nla_policy drm_ras_telemetry_set_dump_nl_policy[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID + 1] = {
+ [DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID] = { .type = NLA_U32, },
+};
+
/* Ops table for drm_ras */
static const struct genl_split_ops drm_ras_nl_ops[] = {
{
@@ -83,6 +106,36 @@ static const struct genl_split_ops drm_ras_nl_ops[] = {
.maxattr = DRM_RAS_A_ERROR_COUNTER_ATTRS_ERROR_THRESHOLD,
.flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
},
+ {
+ .cmd = DRM_RAS_CMD_TELEMETRY_GET,
+ .pre_doit = drm_ras_nl_pre_telemetry,
+ .doit = drm_ras_nl_telemetry_get_doit,
+ .policy = drm_ras_telemetry_get_do_nl_policy,
+ .maxattr = DRM_RAS_A_TELEMETRY_ATTRS_FIELD,
+ .flags = GENL_CMD_CAP_DO,
+ },
+ {
+ .cmd = DRM_RAS_CMD_TELEMETRY_GET,
+ .dumpit = drm_ras_nl_telemetry_get_dumpit,
+ .policy = drm_ras_telemetry_get_dump_nl_policy,
+ .maxattr = DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID,
+ .flags = GENL_CMD_CAP_DUMP,
+ },
+ {
+ .cmd = DRM_RAS_CMD_TELEMETRY_SET,
+ .pre_doit = drm_ras_nl_pre_telemetry,
+ .doit = drm_ras_nl_telemetry_set_doit,
+ .policy = drm_ras_telemetry_set_do_nl_policy,
+ .maxattr = DRM_RAS_A_TELEMETRY_ATTRS_VALUE,
+ .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+ },
+ {
+ .cmd = DRM_RAS_CMD_TELEMETRY_SET,
+ .dumpit = drm_ras_nl_telemetry_set_dumpit,
+ .policy = drm_ras_telemetry_set_dump_nl_policy,
+ .maxattr = DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID,
+ .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
+ },
};
static const struct genl_multicast_group drm_ras_nl_mcgrps[] = {
diff --git a/drivers/gpu/drm/drm_ras_nl.h b/drivers/gpu/drm/drm_ras_nl.h
index 9aef097ea10f..ad3b28f2d809 100644
--- a/drivers/gpu/drm/drm_ras_nl.h
+++ b/drivers/gpu/drm/drm_ras_nl.h
@@ -12,6 +12,9 @@
#include <uapi/drm/drm_ras.h>
+int drm_ras_nl_pre_telemetry(const struct genl_split_ops *ops,
+ struct sk_buff *skb, struct genl_info *info);
+
int drm_ras_nl_list_nodes_dumpit(struct sk_buff *skb,
struct netlink_callback *cb);
int drm_ras_nl_get_error_counter_doit(struct sk_buff *skb,
@@ -24,6 +27,12 @@ int drm_ras_nl_get_error_threshold_doit(struct sk_buff *skb,
struct genl_info *info);
int drm_ras_nl_set_error_threshold_doit(struct sk_buff *skb,
struct genl_info *info);
+int drm_ras_nl_telemetry_get_doit(struct sk_buff *skb, struct genl_info *info);
+int drm_ras_nl_telemetry_get_dumpit(struct sk_buff *skb,
+ struct netlink_callback *cb);
+int drm_ras_nl_telemetry_set_doit(struct sk_buff *skb, struct genl_info *info);
+int drm_ras_nl_telemetry_set_dumpit(struct sk_buff *skb,
+ struct netlink_callback *cb);
enum {
DRM_RAS_NLGRP_ERROR_REPORT,
diff --git a/include/drm/drm_ras.h b/include/drm/drm_ras.h
index 1765a3130b09..fb10238999dd 100644
--- a/include/drm/drm_ras.h
+++ b/include/drm/drm_ras.h
@@ -10,6 +10,16 @@
#include <uapi/drm/drm_ras.h>
+/**
+ * struct drm_ras_telemetry_field - A descriptor of a telemetry field.
+ */
+struct drm_ras_telemetry_field {
+ /** @name: The enum or macro name defined by the device uAPI. */
+ const char *name;
+ /** @field: The numerical value of the field. */
+ u32 value;
+};
+
/**
* struct drm_ras_node - A DRM RAS Node
*/
@@ -100,6 +110,53 @@ struct drm_ras_node {
*/
int (*set_error_threshold)(struct drm_ras_node *node, u32 error_id, u32 threshold);
+ /* Telemetry Related Callback and Variables */
+
+ /** @telemetry_fields_gettable: A list of gettable telemetry fields for a device. */
+ const struct drm_ras_telemetry_field *telemetry_fields_gettable;
+
+ /** @telemetry_fields_settable: A list of settable telemetry fields for a device. */
+ const struct drm_ras_telemetry_field *telemetry_fields_settable;
+
+ /**
+ * @telemetry_pre:
+ *
+ * This callback is used to enforce any pre_doit conditions necessary
+ * for telemetry. This is an optional callback for telemetry_node.
+ *
+ * Returns 0 on success, negative error code on failure.
+ */
+ int (*telemetry_pre)(struct drm_ras_node *ep);
+
+ /**
+ * @telemetry_get:
+ *
+ * This callback is used for a drm-ras telemetry node to read a
+ * telemetry value. This is a mandatory callback for telemetry_node.
+ *
+ * The callback will read the desired device-specific field into value
+ * and update status corresponding to a device-specific error code.
+ *
+ * Returns 0 on success, negative error code on failure.
+ */
+ int (*telemetry_get)(struct drm_ras_node *ep, u32 *field, s64 *value,
+ s32 *status);
+
+ /**
+ * @telemetry_set:
+ *
+ * This callback is used for a drm-ras telemetry node to write a
+ * telemetry value. This is an optional callback for telemetry_node.
+ *
+ * The callback will write the value to the desired device-specific
+ * field and update status corresponding to a device-specific error
+ * code.
+ *
+ * Returns 0 on success, negative error code on failure.
+ */
+ int (*telemetry_set)(struct drm_ras_node *ep, u32 *field, s64 *value,
+ s32 *status);
+
/** @priv: Driver private data */
void *priv;
};
diff --git a/include/uapi/drm/drm_ras.h b/include/uapi/drm/drm_ras.h
index 2f832275ee6e..4776c2f9cb5a 100644
--- a/include/uapi/drm/drm_ras.h
+++ b/include/uapi/drm/drm_ras.h
@@ -11,11 +11,13 @@
#define DRM_RAS_FAMILY_VERSION 1
/*
- * Type of the node. Currently, only error-counter nodes are supported, which
- * expose reliability counters for a hardware/software component.
+ * Type of the node. The error-counter nodes expose reliability counters for a
+ * hardware/software component, and telemetry nodes expose an interface for
+ * reading and writing telemetry values to a device.
*/
enum drm_ras_node_type {
DRM_RAS_NODE_TYPE_ERROR_COUNTER = 1,
+ DRM_RAS_NODE_TYPE_TELEMETRY,
};
enum {
@@ -51,6 +53,17 @@ enum {
DRM_RAS_A_ERROR_EVENT_ATTRS_MAX = (__DRM_RAS_A_ERROR_EVENT_ATTRS_MAX - 1)
};
+enum {
+ DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID = 1,
+ DRM_RAS_A_TELEMETRY_ATTRS_FIELD,
+ DRM_RAS_A_TELEMETRY_ATTRS_FIELD_NAME,
+ DRM_RAS_A_TELEMETRY_ATTRS_VALUE,
+ DRM_RAS_A_TELEMETRY_ATTRS_STATUS,
+
+ __DRM_RAS_A_TELEMETRY_ATTRS_MAX,
+ DRM_RAS_A_TELEMETRY_ATTRS_MAX = (__DRM_RAS_A_TELEMETRY_ATTRS_MAX - 1)
+};
+
enum {
DRM_RAS_CMD_LIST_NODES = 1,
DRM_RAS_CMD_GET_ERROR_COUNTER,
@@ -58,6 +71,8 @@ enum {
DRM_RAS_CMD_ERROR_EVENT,
DRM_RAS_CMD_GET_ERROR_THRESHOLD,
DRM_RAS_CMD_SET_ERROR_THRESHOLD,
+ DRM_RAS_CMD_TELEMETRY_GET,
+ DRM_RAS_CMD_TELEMETRY_SET,
__DRM_RAS_CMD_MAX,
DRM_RAS_CMD_MAX = (__DRM_RAS_CMD_MAX - 1)
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/3] drm/drm_ras: Extend DRM RAS interface to include telemetry nodes
2026-10-29 20:29 ` [PATCH v2 2/3] drm/drm_ras: Extend DRM RAS interface to include telemetry nodes Zack McKevitt
@ 2026-10-30 16:45 ` Jeff Hugo
2026-10-30 20:30 ` sashiko-bot
1 sibling, 0 replies; 10+ messages in thread
From: Jeff Hugo @ 2026-10-30 16:45 UTC (permalink / raw)
To: Zack McKevitt, dri-devel, netdev, riana.tauro, aravind.iddamsetty,
rodrigo.vivi, raag.jadav, simona.vetter, airlied, kuba, ogabbay,
lizhi.hou, karol.wachowski
Cc: carl.vanderlip, troy.hanson, youssef.abdulrahman
On 10/29/2026 2:29 PM, Zack McKevitt wrote:
> Add a new node type, attribute set, and doit/dump functions for
> performing telemetry read and write operations using the DRM RAS
> interface.
>
> ynl-regen.sh was used to regenerate drm_ras_nl.c, drm_ras_nl.h, and
> include/uapi/drm/drm_ras.h from the drm_ras.yaml spec.
>
> Signed-off-by: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>
One minor nit below.
Reviewed-by: Jeff Hugo <jeff.hugo@oss.qualcomm.com>
> diff --git a/Documentation/netlink/specs/drm_ras.yaml b/Documentation/netlink/specs/drm_ras.yaml
> index a88fdab7a1c7..a02aaed8772f 100644
> --- a/Documentation/netlink/specs/drm_ras.yaml
> +++ b/Documentation/netlink/specs/drm_ras.yaml
> @@ -16,11 +16,11 @@ definitions:
> type: enum
> name: node-type
> value-start: 1
> - entries: [error-counter]
> + entries: [error-counter, telemetry]
> doc: >-
> - Type of the node. Currently, only error-counter nodes are
> - supported, which expose reliability counters for a hardware/software
> - component.
> + Type of the node. The error-counter nodes expose reliability counters
> + for a hardware/software component, and telemetry nodes expose an
> + interface for reading and writing telemetry values to a device.
>
> attribute-sets:
> -
> @@ -100,6 +100,34 @@ attribute-sets:
> name: error-value
> type: u32
> doc: Current value of the error counter.
> + -
> + name: telemetry-attrs
> + attributes:
> + -
> + name: node-id
> + type: u32
> + doc: >-
> + Unique identifier for the node.
> + Assigned dynamically by the DRM RAS core upon registration.
> + -
> + name: field
> + type: u32
> + doc: Telemetry field to be read/written, e.g., SoC temp.
> + -
> + name: field-name
> + type: string
> + doc: Name of the telemetry field defined by the device's uapi.
uapi -> uAPI
Might as well be consistent with the documentation elsewhere in this patch.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] drm/drm_ras: Extend DRM RAS interface to include telemetry nodes
2026-10-29 20:29 ` [PATCH v2 2/3] drm/drm_ras: Extend DRM RAS interface to include telemetry nodes Zack McKevitt
2026-10-30 16:45 ` Jeff Hugo
@ 2026-10-30 20:30 ` sashiko-bot
1 sibling, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-30 20:30 UTC (permalink / raw)
To: Zack McKevitt, dri-devel
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/drm_ras: unsynchronized node lookup in telemetry netlink handlers
- [Medium] drm/drm_ras: broken status handling and non-standard UAPI in telemetry ops
- [Medium] drm/drm_ras: missing field validation in telemetry doit callbacks
- [Low] drm/drm_ras: mismatched kernel-doc @field tag in drm_ras_telemetry_field
--
commit dc8b379237033d84f80f290cec4514d0c3cc4442
Author: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>
drm/drm_ras: Extend DRM RAS interface to include telemetry nodes
This commit adds a telemetry node type, netlink attribute set, and doit/dumpit
callbacks to the DRM RAS interface to support reading and writing device
telemetry fields.
> diff --git a/Documentation/netlink/specs/drm_ras.yaml b/Documentation/netlink/specs/drm_ras.yaml
> index a88fdab7a1c74..a02aaed8772fc 100644
> --- a/Documentation/netlink/specs/drm_ras.yaml
> +++ b/Documentation/netlink/specs/drm_ras.yaml
[ ... ]
> @@ -100,6 +100,34 @@ attribute-sets:
> name: error-value
> type: u32
> doc: Current value of the error counter.
> + -
> + name: telemetry-attrs
> + attributes:
> + -
> + name: node-id
> + type: u32
> + doc: >-
> + Unique identifier for the node.
> + Assigned dynamically by the DRM RAS core upon registration.
> + -
> + name: field
> + type: u32
> + doc: Telemetry field to be read/written, e.g., SoC temp.
> + -
> + name: field-name
> + type: string
> + doc: Name of the telemetry field defined by the device's uapi.
> + -
> + name: value
> + type: sint
> + doc: Value of telemetry field that was read/written.
> + -
> + name: status
> + type: s32
> + doc: >-
> + Status of the telemetry operation.
> + Populated by the device in response to the performed
> + telemetry operation. Errors may be hardware specific.
>
> operations:
> list:
> @@ -198,6 +226,52 @@ operations:
> - node-id
> - error-id
> - error-threshold
> + -
> + name: telemetry-get
> + doc: Perform a read of the specified telemetry field.
> + attribute-set: telemetry-attrs
> + do:
> + pre: drm-ras-nl-pre-telemetry
> + request:
> + attributes:
> + - node-id
> + - field
> + reply:
> + attributes:
> + - value
> + - status
> + dump:
> + request:
> + attributes:
> + - node-id
> + reply:
> + attributes: &telemetry-field-attrs
> + - field
> + - field-name
> + -
> + name: telemetry-set
> + doc: >-
> + Perform a write of the specified telemetry field.
> + Telemetry writes are device specific, and may be used to set
> + device limits or other capacities, for example.
> + attribute-set: telemetry-attrs
> + flags: [admin-perm]
> + do:
> + pre: drm-ras-nl-pre-telemetry
> + request:
> + attributes:
> + - node-id
> + - field
> + - value
> + reply:
> + attributes:
> + - status
> + dump:
> + request:
> + attributes:
> + - node-id
> + reply:
> + attributes: *telemetry-field-attrs
[Severity: Medium]
Is this Netlink uAPI structure and status attribute behavior intentional?
In drm_ras_nl_telemetry_get_doit() and drm_ras_nl_telemetry_set_doit(), if
node->telemetry_get() or node->telemetry_set() returns a non-zero value, the
handler frees msg and returns ret directly without serializing
DRM_RAS_A_TELEMETRY_ATTRS_STATUS:
ret = node->telemetry_get(node, &field, &value, &status);
if (ret) {
nlmsg_free(msg);
return ret;
}
In the consumer patch "accel/qaic: Add callbacks for DRM RAS netlink
telemetry", qaic_telemetry_get() and qaic_telemetry_set() assign *status from
telemetry_request() (which maps firmware errors to negative errno values) and
return *status directly. Consequently, whenever an error occurs, the reply is
aborted and DRM_RAS_A_TELEMETRY_ATTRS_STATUS is never sent, whereas whenever a
reply is sent, status is always 0. Conversely, if a driver returns 0 while
setting a non-zero *status, drm_ras_nl_telemetry_get_doit() unconditionally
serializes the uninitialized or zero DRM_RAS_A_TELEMETRY_ATTRS_VALUE and
bypasses standard Netlink error reporting.
In addition, telemetry-set attaches a dump operation to a SET command to list
writable field descriptors, and telemetry-get returns different object
representations in do ([value, status]) versus dump ([field, field-name]),
unlike get-error-counter where do and dump return the same attributes.
Finally, telemetry-attrs exposes opaque device-specific field IDs and unitless
values for thermal, power, and voltage sensors and limits (such as SoC/board
temperatures and TDP in the qaic consumer). Should those metrics be exposed
through hwmon rather than a device-specific key-value pass-through in DRM RAS?
> diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c
> index 4f4777060a4e8..5b9c00f28834b 100644
> --- a/drivers/gpu/drm/drm_ras.c
> +++ b/drivers/gpu/drm/drm_ras.c
[ ... ]
> @@ -617,6 +644,264 @@ int drm_ras_nl_set_error_threshold_doit(struct sk_buff *skb, struct genl_info *i
> return set_node_error_threshold(node_id, error_id, threshold);
> }
>
> +/**
> + * drm_ras_nl_pre_telemetry - Enforce permission checks
> + * @ops: Generic netlink operations
> + * @skb: Netlink message buffer
> + * @info: Generic Netlink info containing attributes of the request
> + *
> + * Enforces a telemetry node's pre_doit conditions (if they exist).
> + *
> + * Returns 0 if no pre_doit is specified, otherwise returns the pre_doit.
> + */
> +int drm_ras_nl_pre_telemetry(const struct genl_split_ops *ops, struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct drm_ras_node *node;
> + u32 node_id;
> +
> + if (!info->attrs || GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID))
> + return -EINVAL;
> +
> + node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]);
> + node = xa_load(&drm_ras_xa, node_id);
> + if (!node)
> + return -ENOENT;
> +
> + if (node->telemetry_pre)
> + return node->telemetry_pre(node);
> +
> + return 0;
> +}
[Severity: High]
What prevents node from being unregistered and freed while
drm_ras_nl_pre_telemetry(), drm_ras_nl_telemetry_get_doit(),
drm_ras_nl_telemetry_get_dumpit(), drm_ras_nl_telemetry_set_doit(), or
drm_ras_nl_telemetry_set_dumpit() is using it?
The drm_ras Generic Netlink family sets .parallel_ops = true, and xa_load()
only holds rcu_read_lock() internally during the lookup, returning an unpinned
struct drm_ras_node pointer without a lock or reference count.
In drm_ras_nl_telemetry_get_doit() and drm_ras_nl_telemetry_set_doit(), the
handler holds node across a sleeping genlmsg_new(..., GFP_KERNEL) allocation
and a sleeping driver callback (such as qaic_telemetry_get() ->
telemetry_request()), while drm_ras_node_unregister() only calls
xa_erase(&drm_ras_xa, node->id) and returns immediately before the driver
frees node and the underlying device structure.
Furthermore, because drm_ras_nl_pre_telemetry() and the doit handlers perform
separate unlocked xa_load(&drm_ras_xa, node_id) lookups instead of pinning the
validated node in pre_doit, can node_id be unregistered and reallocated to a
different node between pre_doit and doit?
[ ... ]
> +int drm_ras_nl_telemetry_get_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct drm_ras_node *node;
> + struct sk_buff *msg;
> + struct nlattr *hdr;
> + u32 node_id, field;
> + s32 status = 0;
> + s64 value = 0;
> + int ret = 0;
> +
> + if (!info->attrs ||
> + GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID) ||
> + GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_FIELD))
> + return -EINVAL;
> +
> + field = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_FIELD]);
> + node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]);
> +
> + node = xa_load(&drm_ras_xa, node_id);
> + if (!node || !node->telemetry_get)
> + return -ENOENT;
> +
> + msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + hdr = genlmsg_iput(msg, info);
> + if (!hdr) {
> + nlmsg_free(msg);
> + return -EMSGSIZE;
> + }
> +
> + ret = node->telemetry_get(node, &field, &value, &status);
[ ... ]
> +int drm_ras_nl_telemetry_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct drm_ras_node *node;
> + struct sk_buff *msg;
> + struct nlattr *hdr;
> + u32 node_id, field;
> + s32 status = 0;
> + s64 value = 0;
> + int ret = 0;
> +
> + if (!info->attrs ||
> + GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID) ||
> + GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_VALUE) ||
> + GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_FIELD))
> + return -EINVAL;
> +
> + value = nla_get_sint(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_VALUE]);
> + field = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_FIELD]);
> + node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]);
> +
> + node = xa_load(&drm_ras_xa, node_id);
> + if (!node || !node->telemetry_set)
> + return -ENOENT;
[ ... ]
> @@ -631,14 +916,21 @@ int drm_ras_node_register(struct drm_ras_node *node)
> if (!node->device_name || !node->node_name)
> return -EINVAL;
>
> - /* Currently, only Error Counter Endpoints are supported */
> - if (node->type != DRM_RAS_NODE_TYPE_ERROR_COUNTER)
> - return -EINVAL;
> -
> - /* Mandatory entries for Error Counter Node */
> - if (node->type == DRM_RAS_NODE_TYPE_ERROR_COUNTER &&
> - (!node->error_counter_range.last || !node->query_error_counter))
> + /* Currently, only Error Counter and Telemetry endpoints are supported */
> + switch (node->type) {
> + case DRM_RAS_NODE_TYPE_ERROR_COUNTER:
> + /* Mandatory entries for Error Counter Node */
> + if (!node->error_counter_range.last || !node->query_error_counter)
> + return -EINVAL;
> + break;
> + case DRM_RAS_NODE_TYPE_TELEMETRY:
> + /* Must provide telemetry_get, telemetry_set is optional */
> + if (!node->telemetry_get || !node->telemetry_fields_gettable)
> + return -EINVAL;
> + break;
[Severity: Medium]
Should drm_ras_nl_telemetry_get_doit() and drm_ras_nl_telemetry_set_doit()
validate the user-supplied field ID against node->telemetry_fields_gettable
and node->telemetry_fields_settable before invoking the driver callbacks?
Unlike the error counter handlers, which check error_id against
node->error_counter_range, drm_ras_nl_telemetry_get_doit() and
drm_ras_nl_telemetry_set_doit() pass the untrusted u32 field directly to
node->telemetry_get() and node->telemetry_set(). In the qaic consumer patch
("accel/qaic: Add callbacks for DRM RAS netlink telemetry"),
qaic_telemetry_get() and qaic_telemetry_set() pass *field directly to
telemetry_request(struct qaic_device *qdev, u8 cmd, ...), which truncates the
u32 value to u8 so out-of-range values like 0x101 alias to valid commands.
Also, since the documentation comment states that settable telemetry fields
are required when providing telemetry_set(), should drm_ras_node_register()
reject a node that sets node->telemetry_set without
node->telemetry_fields_settable, and should include/drm/drm_ras.h document
that telemetry_fields_gettable and telemetry_fields_settable must be
terminated by a NULL .name sentinel entry for telemetry_fields_dumpit()?
> diff --git a/include/drm/drm_ras.h b/include/drm/drm_ras.h
> index 1765a3130b09b..fb10238999ddb 100644
> --- a/include/drm/drm_ras.h
> +++ b/include/drm/drm_ras.h
> @@ -10,6 +10,16 @@
>
> #include <uapi/drm/drm_ras.h>
>
> +/**
> + * struct drm_ras_telemetry_field - A descriptor of a telemetry field.
> + */
> +struct drm_ras_telemetry_field {
> + /** @name: The enum or macro name defined by the device uAPI. */
> + const char *name;
> + /** @field: The numerical value of the field. */
> + u32 value;
> +};
[Severity: Low]
In struct drm_ras_telemetry_field, should the kernel-doc tag @field be @value
to match the struct member name?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261029203000.1763447-1-zachary.mckevitt@oss.qualcomm.com?part=2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] accel/qaic: Add callbacks for DRM RAS netlink telemetry
2026-10-29 20:29 [PATCH v2 0/3] Extend drm_ras for qaic telemetry Zack McKevitt
2026-10-29 20:29 ` [PATCH v2 1/3] drm/drm_ras: Remove admin-perm flag from list-nodes operation Zack McKevitt
2026-10-29 20:29 ` [PATCH v2 2/3] drm/drm_ras: Extend DRM RAS interface to include telemetry nodes Zack McKevitt
@ 2026-10-29 20:30 ` Zack McKevitt
2026-10-30 17:06 ` Jeff Hugo
2026-10-30 20:30 ` sashiko-bot
2026-10-30 16:36 ` [PATCH v2 0/3] Extend drm_ras for qaic telemetry Jeff Hugo
3 siblings, 2 replies; 10+ messages in thread
From: Zack McKevitt @ 2026-10-29 20:30 UTC (permalink / raw)
To: dri-devel, netdev, riana.tauro, aravind.iddamsetty, rodrigo.vivi,
raag.jadav, simona.vetter, airlied, kuba, ogabbay, lizhi.hou,
karol.wachowski
Cc: jeff.hugo, carl.vanderlip, troy.hanson, youssef.abdulrahman,
Zack McKevitt
Use the DRM RAS netlink infrastructure to perform device telemetry.
During PCI probe, create a DRM RAS node with DRM_RAS_NODE_TYPE_TELEMETRY
type and add callbacks to telemetry_request for read and write operations.
Co-developed-by: Jeff Hugo <jeff.hugo@oss.qualcomm.com>
Signed-off-by: Jeff Hugo <jeff.hugo@oss.qualcomm.com>
Signed-off-by: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>
---
drivers/accel/qaic/Makefile | 1 +
drivers/accel/qaic/qaic.h | 19 ++
drivers/accel/qaic/qaic_drv.c | 22 ++
drivers/accel/qaic/qaic_telemetry.c | 513 ++++++++++++++++++++++++++++
drivers/accel/qaic/qaic_telemetry.h | 18 +
include/uapi/drm/qaic_accel.h | 28 ++
6 files changed, 601 insertions(+)
create mode 100644 drivers/accel/qaic/qaic_telemetry.c
create mode 100644 drivers/accel/qaic/qaic_telemetry.h
diff --git a/drivers/accel/qaic/Makefile b/drivers/accel/qaic/Makefile
index 71f727b74da3..b2bbe9b3e077 100644
--- a/drivers/accel/qaic/Makefile
+++ b/drivers/accel/qaic/Makefile
@@ -13,6 +13,7 @@ qaic-y := \
qaic_ras.o \
qaic_ssr.o \
qaic_sysfs.o \
+ qaic_telemetry.o \
qaic_timesync.o \
sahara.o
diff --git a/drivers/accel/qaic/qaic.h b/drivers/accel/qaic/qaic.h
index 889fa51da7da..fa19c758f504 100644
--- a/drivers/accel/qaic/qaic.h
+++ b/drivers/accel/qaic/qaic.h
@@ -17,6 +17,7 @@
#include <linux/workqueue.h>
#include <drm/drm_device.h>
#include <drm/drm_gem.h>
+#include <drm/drm_ras.h>
#define QAIC_DBC_BASE SZ_128K
#define QAIC_DBC_SIZE SZ_4K
@@ -204,6 +205,24 @@ struct qaic_device {
void *ssr_mhi_buf;
/* DBC which is under SSR. Sentinel U32_MAX would mean that no SSR in progress */
u32 ssr_dbc;
+ /* MHI telemetry channel device */
+ struct mhi_device *tele_ch;
+ /* Head in list of requests queued in MHI telemetry device */
+ struct list_head tele_xfer_list;
+ /* Req. ID of request that will be queued next in MHI telemetry device */
+ u32 tele_next_seq_num;
+ /* Synchronizes MHI telemetry device transactions and its xfer list */
+ struct mutex tele_mutex;
+ /*
+ * TRUE: A tx MHI transaction has failed and a rx buffer is still queued
+ * in telemetry device. Such a buffer is considered lost rx buffer
+ * FALSE: No rx buffer is lost in telemetry device
+ */
+ bool tele_lost_buf;
+ /* Work queue for tasks related to MHI telemetry device */
+ struct workqueue_struct *tele_wq;
+ /* Netlink telemetry node */
+ struct drm_ras_node *ras_node;
/* Array of DBC struct of this device */
struct dma_bridge_chan dbc[] __counted_by(num_dbc);
};
diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
index 9e1e184efe0d..e24a4f271a58 100644
--- a/drivers/accel/qaic/qaic_drv.c
+++ b/drivers/accel/qaic/qaic_drv.c
@@ -32,6 +32,7 @@
#include "qaic_debugfs.h"
#include "qaic_ras.h"
#include "qaic_ssr.h"
+#include "qaic_telemetry.h"
#include "qaic_timesync.h"
#include "sahara.h"
@@ -389,6 +390,7 @@ static void qaic_notify_reset(struct qaic_device *qdev)
qdev->dev_state = QAIC_OFFLINE;
/* wake up any waiters to avoid waiting for timeouts at sync */
wake_all_cntl(qdev);
+ wake_all_telemetry(qdev);
for (i = 0; i < qdev->num_dbc; ++i)
wakeup_dbc(qdev, i);
synchronize_srcu(&qdev->dev_lock);
@@ -439,6 +441,9 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev,
if (ret)
return NULL;
ret = drmm_mutex_init(drm, &qdev->bootlog_mutex);
+ if (ret)
+ return NULL;
+ ret = drmm_mutex_init(drm, &qdev->tele_mutex);
if (ret)
return NULL;
@@ -451,6 +456,9 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev,
qdev->ssr_wq = qaicm_wq_init(drm, "qaic_ssr");
if (IS_ERR(qdev->ssr_wq))
return NULL;
+ qdev->tele_wq = qaicm_wq_init(drm, "qaic_tele");
+ if (IS_ERR(qdev->tele_wq))
+ return NULL;
ret = qaicm_srcu_init(drm, &qdev->dev_lock);
if (ret)
@@ -466,6 +474,7 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev,
INIT_LIST_HEAD(&qdev->cntl_xfer_list);
INIT_LIST_HEAD(&qdev->bootlog);
+ INIT_LIST_HEAD(&qdev->tele_xfer_list);
INIT_LIST_HEAD(&qddev->users);
for (i = 0; i < qdev->num_dbc; ++i) {
@@ -611,6 +620,13 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
return ret;
}
+ ret = qaic_drm_ras_register_node(qdev);
+ if (ret) {
+ qaic_mhi_free_controller(qdev->mhi_cntrl, link_up);
+ qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
+ return ret;
+ }
+
return 0;
}
@@ -624,6 +640,7 @@ static void qaic_pci_remove(struct pci_dev *pdev)
return;
qaic_dev_reset_clean_local_state(qdev);
+ qaic_drm_ras_unregister_node(qdev);
qaic_mhi_free_controller(qdev->mhi_cntrl, link_up);
qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
}
@@ -860,6 +877,10 @@ static int __init qaic_init(void)
goto free_bootlog;
}
+ ret = qaic_telemetry_register();
+ if (ret)
+ pr_debug("qaic: qaic_telemetry_register failed %d\n", ret);
+
return 0;
free_bootlog:
@@ -889,6 +910,7 @@ static void __exit qaic_exit(void)
* reinitializing the link_up state after the cleanup is done.
*/
link_up = true;
+ qaic_telemetry_unregister();
qaic_ssr_unregister();
qaic_ras_unregister();
qaic_bootlog_unregister();
diff --git a/drivers/accel/qaic/qaic_telemetry.c b/drivers/accel/qaic/qaic_telemetry.c
new file mode 100644
index 000000000000..fdce7a71d3eb
--- /dev/null
+++ b/drivers/accel/qaic/qaic_telemetry.c
@@ -0,0 +1,513 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/* Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. */
+/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
+
+#include <drm/drm_file.h>
+#include <drm/drm_ras.h>
+#include <asm/byteorder.h>
+#include <linux/completion.h>
+#include <linux/fs.h>
+#include <linux/fs_struct.h>
+#include <linux/kernel.h>
+#include <linux/kref.h>
+#include <linux/list.h>
+#include <linux/mhi.h>
+#include <linux/mutex.h>
+#include <linux/namei.h>
+#include <linux/path.h>
+#include <linux/sprintf.h>
+#include <linux/srcu.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+#include <uapi/drm/qaic_accel.h>
+
+#include "qaic.h"
+#include "qaic_telemetry.h"
+
+#define MAGIC 0x55AA
+#define VERSION 0x1
+
+/*
+ * Telemetry fields defined by the uAPI.
+ * This table will be used by drm_ras's dumpit to display
+ * each field to userspace.
+ */
+const struct drm_ras_telemetry_field qaic_telemetry_fields_gettable[] = {
+ {"QAIC_DEVINFO_THERMAL_SOC_MAX_TEMP", 1},
+ {"QAIC_DEVINFO_THERMAL_BOARD_TEMP", 2},
+ {"QAIC_DEVINFO_THERMAL_BOARD_MAX_TEMP", 3},
+ {"QAIC_DEVINFO_THERMAL_DDR_TEMP", 4},
+ {"QAIC_DEVINFO_THERMAL_WARNING_TEMP", 5},
+ {"QAIC_DEVINFO_THERMAL_SHUTDOWN_TEMP", 6},
+ {"QAIC_DEVINFO_BOARD_TDP", 7},
+ {"QAIC_DEVINFO_BOARD_POWER", 8},
+ {"QAIC_DEVINFO_POWER_STATE", 9},
+ {"QAIC_DEVINFO_POWER_MAX", 10},
+ {"QAIC_DEVINFO_THROTTLE_PERCENT", 11},
+ {"QAIC_DEVINFO_THROTTLE_TIME", 12},
+ {"QAIC_DEVINFO_UPTIME", 13},
+ {"QAIC_DEVINFO_THERMAL_SOC_FLOOR_TEMP", 14},
+ {"QAIC_DEVINFO_THERMAL_SOC_CEILING_TEMP", 15},
+ {"QAIC_DEVINFO_VOLTAGE_CX", 16},
+ {"QAIC_DEVINFO_VOLTAGE_MX", 17},
+ {"QAIC_DEVINFO_VOLTAGE_NSP_CX", 18},
+ {"QAIC_DEVINFO_VOLTAGE_NSP_MX", 19},
+ {"QAIC_DEVINFO_CFG_TOPS", 20},
+ {"QAIC_DEVINFO_THERMAL_PMIC_TEMP_A", 21},
+ {"QAIC_DEVINFO_THERMAL_PMIC_TEMP_C", 22},
+ {"QAIC_DEVINFO_THERMAL_PMIC_TEMP_E", 23},
+ {"QAIC_DEVINFO_POWER_ACTION", 24},
+ {"QAIC_DEVINFO_SOC_TDP", 25},
+ {"QAIC_DEVINFO_SOC_POWER", 26},
+ {},
+};
+
+const struct drm_ras_telemetry_field qaic_telemetry_fields_settable[] = {
+ {"QAIC_DEVINFO_THERMAL_WARNING_TEMP", 5},
+ {"QAIC_DEVINFO_THERMAL_SHUTDOWN_TEMP", 6},
+ {"QAIC_DEVINFO_BOARD_TDP", 7},
+ {"QAIC_DEVINFO_POWER_STATE", 9},
+ {"QAIC_DEVINFO_CFG_TOPS", 20},
+ {"QAIC_DEVINFO_POWER_ACTION", 24},
+ {"QAIC_DEVINFO_SOC_TDP", 25},
+ {},
+};
+
+enum fw_errno {
+ ERR_READ_ONLY = -1,
+ ERR_NOT_SUPPORTED = -2,
+ ERR_WHILE_PROCESSING = -3,
+ ERR_INVALID_VALUE = -4,
+ ERR_INVALID_HEADER = -5,
+};
+
+enum cmd_type {
+ TYPE_READ, /* read value from device */
+ TYPE_WRITE, /* write value to device */
+};
+
+enum msg_type {
+ MSG_PUSH, /* async push from device */
+ MSG_REQ, /* sync request to device */
+ MSG_RESP, /* sync response from device */
+};
+
+struct telemetry_data {
+ u8 cmd;
+ u8 cmd_type;
+ u8 status;
+ __le64 val; /* signed */
+} __packed;
+
+struct telemetry_header {
+ __le16 magic;
+ __le16 ver;
+ __le32 seq_num;
+ u8 type;
+ u8 id;
+ __le16 len;
+} __packed;
+
+struct telemetry_msg { /* little endian encoded */
+ struct telemetry_header hdr;
+ struct telemetry_data data;
+} __packed;
+
+struct wrapper_msg {
+ struct kref ref_count;
+ struct telemetry_msg msg;
+};
+
+struct xfer_queue_elem {
+ /*
+ * Node in list of ongoing transfer request on telemetry channel.
+ * Maintained by root device struct
+ */
+ struct list_head list;
+ /* Sequence number of this transfer request */
+ u32 seq_num;
+ /* This is used to wait on until completion of transfer request */
+ struct completion xfer_done;
+ /* Received data from device */
+ void *buf;
+};
+
+struct resp_work {
+ /* Work struct to schedule work coming on QAIC_TELEMETRY channel */
+ struct work_struct work;
+ /* Root struct of device, used to access device resources */
+ struct qaic_device *qdev;
+ /* Buffer used by MHI for transfer requests */
+ void *buf;
+};
+
+static void free_wrapper(struct kref *ref)
+{
+ struct wrapper_msg *wrapper = container_of(ref, struct wrapper_msg, ref_count);
+
+ kfree(wrapper);
+}
+
+int telemetry_request(struct qaic_device *qdev, u8 cmd, u8 cmd_type, s64 *val)
+{
+ struct wrapper_msg *wrapper;
+ struct xfer_queue_elem elem;
+ struct telemetry_msg *resp;
+ struct telemetry_msg *req;
+ long ret = 0;
+
+ wrapper = kzalloc_obj(*wrapper);
+ if (!wrapper)
+ return -ENOMEM;
+
+ kref_init(&wrapper->ref_count);
+ req = &wrapper->msg;
+
+ ret = mutex_lock_interruptible(&qdev->tele_mutex);
+ if (ret)
+ goto free_req;
+
+ req->hdr.magic = cpu_to_le16(MAGIC);
+ req->hdr.ver = cpu_to_le16(VERSION);
+ req->hdr.seq_num = cpu_to_le32(qdev->tele_next_seq_num++);
+ req->hdr.type = MSG_REQ;
+ req->hdr.id = 0;
+ req->hdr.len = cpu_to_le16(sizeof(req->data));
+
+ req->data.cmd = cmd;
+ req->data.cmd_type = cmd_type;
+ req->data.status = 0;
+ if (cmd_type == TYPE_READ)
+ req->data.val = cpu_to_le64(0);
+ else
+ req->data.val = cpu_to_le64(*val);
+
+ elem.seq_num = qdev->tele_next_seq_num - 1;
+ elem.buf = NULL;
+ init_completion(&elem.xfer_done);
+ if (likely(!qdev->tele_lost_buf)) {
+ resp = kzalloc_obj(*resp);
+ if (!resp) {
+ mutex_unlock(&qdev->tele_mutex);
+ ret = -ENOMEM;
+ goto free_req;
+ }
+
+ ret = mhi_queue_buf(qdev->tele_ch, DMA_FROM_DEVICE, resp, sizeof(*resp), MHI_EOT);
+ if (ret) {
+ mutex_unlock(&qdev->tele_mutex);
+ goto free_resp;
+ }
+ } else {
+ /*
+ * we lost a buffer because we queued a recv buf, but then
+ * queuing the corresponding tx buf failed. To try to avoid
+ * a memory leak, lets reclaim it and use it for this
+ * transaction.
+ */
+ qdev->tele_lost_buf = false;
+ }
+
+ kref_get(&wrapper->ref_count);
+ ret = mhi_queue_buf(qdev->tele_ch, DMA_TO_DEVICE, req, sizeof(*req), MHI_EOT);
+ if (ret) {
+ qdev->tele_lost_buf = true;
+ kref_put(&wrapper->ref_count, free_wrapper);
+ mutex_unlock(&qdev->tele_mutex);
+ goto free_req;
+ }
+
+ list_add_tail(&elem.list, &qdev->tele_xfer_list);
+ mutex_unlock(&qdev->tele_mutex);
+
+ ret = wait_for_completion_interruptible_timeout(&elem.xfer_done, HZ);
+ /*
+ * not using _interruptable because we have to cleanup or we'll
+ * likely cause memory corruption
+ */
+ mutex_lock(&qdev->tele_mutex);
+ if (!list_empty(&elem.list))
+ list_del(&elem.list);
+ if (!ret && !elem.buf)
+ ret = -ETIMEDOUT;
+ else if (ret > 0 && !elem.buf)
+ ret = -EIO;
+ mutex_unlock(&qdev->tele_mutex);
+
+ resp = elem.buf;
+
+ if (ret < 0)
+ goto free_resp;
+
+ if (le16_to_cpu(resp->hdr.magic) != MAGIC || le16_to_cpu(resp->hdr.ver) != VERSION ||
+ resp->hdr.type != MSG_RESP || resp->hdr.id != 0 ||
+ le16_to_cpu(resp->hdr.len) != sizeof(resp->data) || resp->data.cmd != cmd ||
+ resp->data.cmd_type != cmd_type) {
+ ret = -EINVAL;
+ goto free_resp;
+ }
+
+ if (resp->data.status) {
+ switch ((int8_t) resp->data.status) {
+ case ERR_READ_ONLY:
+ ret = -EROFS;
+ break;
+ case ERR_NOT_SUPPORTED:
+ ret = -EOPNOTSUPP;
+ break;
+ case ERR_WHILE_PROCESSING:
+ ret = -EREMOTEIO;
+ break;
+ case ERR_INVALID_VALUE:
+ ret = -EINVAL;
+ break;
+ case ERR_INVALID_HEADER:
+ ret = -EBADR;
+ break;
+ default:
+ ret = -EPROTO;
+ }
+ goto free_resp;
+ }
+
+ if (cmd_type == TYPE_READ)
+ *val = le64_to_cpu(resp->data.val);
+
+ ret = 0;
+
+free_resp:
+ kfree(resp);
+free_req:
+ kref_put(&wrapper->ref_count, free_wrapper);
+
+ return ret;
+}
+
+static int qaic_telemetry_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_id *id)
+{
+ struct qaic_device *qdev = pci_get_drvdata(to_pci_dev(mhi_dev->mhi_cntrl->cntrl_dev));
+ int ret;
+
+ ret = mhi_prepare_for_transfer(mhi_dev);
+ if (ret)
+ return ret;
+
+ dev_set_drvdata(&mhi_dev->dev, qdev);
+ qdev->tele_ch = mhi_dev;
+ qdev->tele_lost_buf = false;
+
+ return 0;
+}
+
+static void qaic_telemetry_mhi_remove(struct mhi_device *mhi_dev)
+{
+ struct qaic_device *qdev = dev_get_drvdata(&mhi_dev->dev);
+
+ mhi_unprepare_from_transfer(qdev->tele_ch);
+ qdev->tele_ch = NULL;
+}
+
+static void resp_worker(struct work_struct *work)
+{
+ struct resp_work *resp = container_of(work, struct resp_work, work);
+ struct qaic_device *qdev = resp->qdev;
+ struct telemetry_msg *msg = resp->buf;
+ struct xfer_queue_elem *elem;
+ struct xfer_queue_elem *i;
+ bool found = false;
+
+ if (le16_to_cpu(msg->hdr.magic) != MAGIC) {
+ kfree(msg);
+ kfree(resp);
+ return;
+ }
+
+ mutex_lock(&qdev->tele_mutex);
+ list_for_each_entry_safe(elem, i, &qdev->tele_xfer_list, list) {
+ if (elem->seq_num == le32_to_cpu(msg->hdr.seq_num)) {
+ found = true;
+ list_del_init(&elem->list);
+ elem->buf = msg;
+ complete_all(&elem->xfer_done);
+ break;
+ }
+ }
+ mutex_unlock(&qdev->tele_mutex);
+
+ if (!found)
+ /* request must have timed out, drop packet */
+ kfree(msg);
+
+ kfree(resp);
+}
+
+static void qaic_telemetry_mhi_ul_xfer_cb(struct mhi_device *mhi_dev,
+ struct mhi_result *mhi_result)
+{
+ struct telemetry_msg *msg = mhi_result->buf_addr;
+ struct wrapper_msg *wrapper;
+
+ wrapper = container_of(msg, struct wrapper_msg, msg);
+ kref_put(&wrapper->ref_count, free_wrapper);
+}
+
+static void qaic_telemetry_mhi_dl_xfer_cb(struct mhi_device *mhi_dev,
+ struct mhi_result *mhi_result)
+{
+ struct qaic_device *qdev = dev_get_drvdata(&mhi_dev->dev);
+ struct telemetry_msg *msg = mhi_result->buf_addr;
+ struct resp_work *resp;
+
+ if (mhi_result->transaction_status) {
+ kfree(msg);
+ return;
+ }
+
+ resp = kmalloc(sizeof(*resp), GFP_ATOMIC);
+ if (!resp) {
+ pci_err(qdev->pdev, "dl_xfer_cb alloc fail, dropping message\n");
+ kfree(msg);
+ return;
+ }
+
+ INIT_WORK(&resp->work, resp_worker);
+ resp->qdev = qdev;
+ resp->buf = msg;
+ queue_work(qdev->tele_wq, &resp->work);
+}
+
+static const struct mhi_device_id qaic_telemetry_mhi_match_table[] = {
+ { .chan = "QAIC_TELEMETRY", },
+ {},
+};
+
+static struct mhi_driver qaic_telemetry_mhi_driver = {
+ .id_table = qaic_telemetry_mhi_match_table,
+ .remove = qaic_telemetry_mhi_remove,
+ .probe = qaic_telemetry_mhi_probe,
+ .ul_xfer_cb = qaic_telemetry_mhi_ul_xfer_cb,
+ .dl_xfer_cb = qaic_telemetry_mhi_dl_xfer_cb,
+ .driver = {
+ .name = "qaic_telemetry",
+ },
+};
+
+int qaic_telemetry_register(void)
+{
+ return mhi_driver_register(&qaic_telemetry_mhi_driver);
+}
+
+void qaic_telemetry_unregister(void)
+{
+ mhi_driver_unregister(&qaic_telemetry_mhi_driver);
+}
+
+void wake_all_telemetry(struct qaic_device *qdev)
+{
+ struct xfer_queue_elem *elem;
+ struct xfer_queue_elem *i;
+
+ mutex_lock(&qdev->tele_mutex);
+ list_for_each_entry_safe(elem, i, &qdev->tele_xfer_list, list) {
+ list_del_init(&elem->list);
+ complete_all(&elem->xfer_done);
+ }
+ qdev->tele_lost_buf = false;
+ mutex_unlock(&qdev->tele_mutex);
+}
+
+static int qaic_telemetry_pre(struct drm_ras_node *ep)
+{
+ struct qaic_device *qdev = ep->priv;
+ struct inode *dev_inode;
+ struct path path;
+ char devpath[24];
+ int ret;
+
+ snprintf(devpath, sizeof(devpath), "/dev/accel/%s", to_accel_kdev(qdev->qddev)->kobj.name);
+ scoped_with_init_fs()
+ ret = kern_path(devpath, LOOKUP_FOLLOW, &path);
+ if (ret)
+ return -EINVAL;
+
+ dev_inode = d_inode(path.dentry);
+ ret = inode_permission(&nop_mnt_idmap, dev_inode, MAY_READ | MAY_WRITE);
+ path_put(&path);
+
+ return ret;
+}
+
+static int qaic_telemetry_get(struct drm_ras_node *ep, u32 *field, s64 *value, s32 *status)
+{
+ struct qaic_device *qdev = ep->priv;
+
+ *status = (s32)telemetry_request(qdev, *field, TYPE_READ, value);
+
+ return *status;
+}
+
+static int qaic_telemetry_set(struct drm_ras_node *ep, u32 *field, s64 *value, s32 *status)
+{
+ struct qaic_device *qdev = ep->priv;
+
+ *status = (s32)telemetry_request(qdev, *field, TYPE_WRITE, value);
+
+ return *status;
+}
+
+int qaic_drm_ras_register_node(struct qaic_device *qdev)
+{
+ struct drm_ras_node *node;
+ struct pci_dev *pdev = qdev->pdev;
+ const char *device_name, *node_name;
+ int ret;
+
+ node = kzalloc_obj(struct drm_ras_node);
+ if (!node)
+ return -ENOMEM;
+
+ device_name = kasprintf(GFP_KERNEL, "%04x:%02x:%02x.%d",
+ pci_domain_nr(pdev->bus), pdev->bus->number,
+ PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+ if (!device_name) {
+ kfree(node);
+ return -ENOMEM;
+ }
+
+ node_name = kasprintf(GFP_KERNEL, "qaic_telemetry");
+ if (!node_name) {
+ kfree(device_name);
+ kfree(node);
+ return -ENOMEM;
+ }
+
+ node->device_name = device_name;
+ node->node_name = node_name;
+ node->type = DRM_RAS_NODE_TYPE_TELEMETRY;
+ node->priv = qdev;
+ node->telemetry_get = qaic_telemetry_get;
+ node->telemetry_set = qaic_telemetry_set;
+ node->telemetry_pre = qaic_telemetry_pre;
+ node->telemetry_fields_gettable = qaic_telemetry_fields_gettable;
+ node->telemetry_fields_settable = qaic_telemetry_fields_settable;
+
+ ret = drm_ras_node_register(node);
+ if (ret) {
+ kfree(device_name);
+ kfree(node_name);
+ kfree(node);
+ return ret;
+ }
+
+ qdev->ras_node = node;
+ return 0;
+}
+
+void qaic_drm_ras_unregister_node(struct qaic_device *qdev)
+{
+ drm_ras_node_unregister(qdev->ras_node);
+ kfree(qdev->ras_node->node_name);
+ kfree(qdev->ras_node->device_name);
+ kfree(qdev->ras_node);
+}
diff --git a/drivers/accel/qaic/qaic_telemetry.h b/drivers/accel/qaic/qaic_telemetry.h
new file mode 100644
index 000000000000..a80922591e37
--- /dev/null
+++ b/drivers/accel/qaic/qaic_telemetry.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/* Copyright (c) 2020, The Linux Foundation. All rights reserved. */
+
+#ifndef __QAIC_TELEMETRY_H__
+#define __QAIC_TELEMETRY_H__
+
+#include "qaic.h"
+
+int telemetry_request(struct qaic_device *qdev, u8 cmd, u8 cmd_type, s64 *val);
+int qaic_drm_ras_register_node(struct qaic_device *qdev);
+void qaic_drm_ras_unregister_node(struct qaic_device *qdev);
+
+int qaic_telemetry_register(void);
+void qaic_telemetry_unregister(void);
+void wake_all_telemetry(struct qaic_device *qdev);
+
+#endif /* __QAIC_TELEMETRY_H__ */
diff --git a/include/uapi/drm/qaic_accel.h b/include/uapi/drm/qaic_accel.h
index c92d0309d583..5aac05101426 100644
--- a/include/uapi/drm/qaic_accel.h
+++ b/include/uapi/drm/qaic_accel.h
@@ -51,6 +51,34 @@ extern "C" {
#define QAIC_TRANS_VALIDATE_PARTITION_FROM_DEV 19
#define QAIC_TRANS_VALIDATE_PARTITION_TO_DEV 20
+/* Telemetry fields */
+#define QAIC_DEVINFO_THERMAL_SOC_MAX_TEMP 1
+#define QAIC_DEVINFO_THERMAL_BOARD_TEMP 2
+#define QAIC_DEVINFO_THERMAL_BOARD_MAX_TEMP 3
+#define QAIC_DEVINFO_THERMAL_DDR_TEMP 4
+#define QAIC_DEVINFO_THERMAL_WARNING_TEMP 5
+#define QAIC_DEVINFO_THERMAL_SHUTDOWN_TEMP 6
+#define QAIC_DEVINFO_BOARD_TDP 7
+#define QAIC_DEVINFO_BOARD_POWER 8
+#define QAIC_DEVINFO_POWER_STATE 9
+#define QAIC_DEVINFO_POWER_MAX 10
+#define QAIC_DEVINFO_THROTTLE_PERCENT 11
+#define QAIC_DEVINFO_THROTTLE_TIME 12
+#define QAIC_DEVINFO_UPTIME 13
+#define QAIC_DEVINFO_THERMAL_SOC_FLOOR_TEMP 14
+#define QAIC_DEVINFO_THERMAL_SOC_CEILING_TEMP 15
+#define QAIC_DEVINFO_VOLTAGE_CX 16
+#define QAIC_DEVINFO_VOLTAGE_MX 17
+#define QAIC_DEVINFO_VOLTAGE_NSP_CX 18
+#define QAIC_DEVINFO_VOLTAGE_NSP_MX 19
+#define QAIC_DEVINFO_CFG_TOPS 20
+#define QAIC_DEVINFO_THERMAL_PMIC_TEMP_A 21
+#define QAIC_DEVINFO_THERMAL_PMIC_TEMP_C 22
+#define QAIC_DEVINFO_THERMAL_PMIC_TEMP_E 23
+#define QAIC_DEVINFO_POWER_ACTION 24
+#define QAIC_DEVINFO_SOC_TDP 25
+#define QAIC_DEVINFO_SOC_POWER 26
+
/**
* struct qaic_manage_trans_hdr - Header for a transaction in a manage message.
* @type: In. Identifies this transaction. See QAIC_TRANS_* defines.
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 3/3] accel/qaic: Add callbacks for DRM RAS netlink telemetry
2026-10-29 20:30 ` [PATCH v2 3/3] accel/qaic: Add callbacks for DRM RAS netlink telemetry Zack McKevitt
@ 2026-10-30 17:06 ` Jeff Hugo
2026-10-30 20:30 ` sashiko-bot
1 sibling, 0 replies; 10+ messages in thread
From: Jeff Hugo @ 2026-10-30 17:06 UTC (permalink / raw)
To: Zack McKevitt, dri-devel, netdev, riana.tauro, aravind.iddamsetty,
rodrigo.vivi, raag.jadav, simona.vetter, airlied, kuba, ogabbay,
lizhi.hou, karol.wachowski
Cc: carl.vanderlip, troy.hanson, youssef.abdulrahman
On 10/29/2026 2:30 PM, Zack McKevitt wrote:
> @@ -611,6 +620,13 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> return ret;
> }
>
> + ret = qaic_drm_ras_register_node(qdev);
> + if (ret) {
> + qaic_mhi_free_controller(qdev->mhi_cntrl, link_up);
> + qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
> + return ret;
> + }
> +
> return 0;
> }
Registering the mhi_controller will kickoff a whole init and boot
sequence for the hardware. Feels like we should attempt this before that
as if this fails, we don't leave the hardware in a weird state.
> diff --git a/drivers/accel/qaic/qaic_telemetry.c b/drivers/accel/qaic/qaic_telemetry.c
> new file mode 100644
> index 000000000000..fdce7a71d3eb
> --- /dev/null
> +++ b/drivers/accel/qaic/qaic_telemetry.c
> @@ -0,0 +1,513 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/* Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. */
> +/* Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. */
> +
> +#include <drm/drm_file.h>
> +#include <drm/drm_ras.h>
> +#include <asm/byteorder.h>
A before D in the alphabet?
> +#include <linux/completion.h>
> +#include <linux/fs.h>
> +#include <linux/fs_struct.h>
> +#include <linux/kernel.h>
> +#include <linux/kref.h>
> +#include <linux/list.h>
> +#include <linux/mhi.h>
> +#include <linux/mutex.h>
> +#include <linux/namei.h>
> +#include <linux/path.h>
> +#include <linux/sprintf.h>
> +#include <linux/srcu.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +#include <uapi/drm/qaic_accel.h>
> +
> +#include "qaic.h"
> +#include "qaic_telemetry.h"
> +
> +#define MAGIC 0x55AA
> +#define VERSION 0x1
> +
> +/*
> + * Telemetry fields defined by the uAPI.
> + * This table will be used by drm_ras's dumpit to display
> + * each field to userspace.
> + */
> +const struct drm_ras_telemetry_field qaic_telemetry_fields_gettable[] = {
> + {"QAIC_DEVINFO_THERMAL_SOC_MAX_TEMP", 1},
> + {"QAIC_DEVINFO_THERMAL_BOARD_TEMP", 2},
> + {"QAIC_DEVINFO_THERMAL_BOARD_MAX_TEMP", 3},
> + {"QAIC_DEVINFO_THERMAL_DDR_TEMP", 4},
> + {"QAIC_DEVINFO_THERMAL_WARNING_TEMP", 5},
> + {"QAIC_DEVINFO_THERMAL_SHUTDOWN_TEMP", 6},
> + {"QAIC_DEVINFO_BOARD_TDP", 7},
> + {"QAIC_DEVINFO_BOARD_POWER", 8},
> + {"QAIC_DEVINFO_POWER_STATE", 9},
> + {"QAIC_DEVINFO_POWER_MAX", 10},
> + {"QAIC_DEVINFO_THROTTLE_PERCENT", 11},
> + {"QAIC_DEVINFO_THROTTLE_TIME", 12},
> + {"QAIC_DEVINFO_UPTIME", 13},
> + {"QAIC_DEVINFO_THERMAL_SOC_FLOOR_TEMP", 14},
> + {"QAIC_DEVINFO_THERMAL_SOC_CEILING_TEMP", 15},
> + {"QAIC_DEVINFO_VOLTAGE_CX", 16},
> + {"QAIC_DEVINFO_VOLTAGE_MX", 17},
> + {"QAIC_DEVINFO_VOLTAGE_NSP_CX", 18},
> + {"QAIC_DEVINFO_VOLTAGE_NSP_MX", 19},
> + {"QAIC_DEVINFO_CFG_TOPS", 20},
> + {"QAIC_DEVINFO_THERMAL_PMIC_TEMP_A", 21},
> + {"QAIC_DEVINFO_THERMAL_PMIC_TEMP_C", 22},
> + {"QAIC_DEVINFO_THERMAL_PMIC_TEMP_E", 23},
> + {"QAIC_DEVINFO_POWER_ACTION", 24},
> + {"QAIC_DEVINFO_SOC_TDP", 25},
> + {"QAIC_DEVINFO_SOC_POWER", 26},
This should be in sync with the defines in qaic_accel.h right? Can we
just use those defines here?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 3/3] accel/qaic: Add callbacks for DRM RAS netlink telemetry
2026-10-29 20:30 ` [PATCH v2 3/3] accel/qaic: Add callbacks for DRM RAS netlink telemetry Zack McKevitt
2026-10-30 17:06 ` Jeff Hugo
@ 2026-10-30 20:30 ` sashiko-bot
1 sibling, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-30 20:30 UTC (permalink / raw)
To: Zack McKevitt, dri-devel
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] accel/qaic: NULL pointer dereference and race on qdev->tele_ch
- [High] accel/qaic: missing dev_lock SRCU sync and tele_ch state check in telemetry
- [High] accel/qaic: missing tele_wq flush and drmm/devm mismatch in create_qdev()
- [Medium] accel/qaic: qaic_telemetry_get() and qaic_telemetry_set() drop status replies on error
- [Medium] accel/qaic: silent u8 truncation of u32 telemetry field in telemetry_request()
- [Medium] accel/qaic: ignored qaic_telemetry_register() failure in qaic_init()
- [Medium] accel/qaic: telemetry_request() drops completed response and leaks -ERESTARTSYS to netlink
--
commit 089cd42088c5c813480c90b14aeb8fbd79e5ff35
Author: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>
accel/qaic: Add callbacks for DRM RAS netlink telemetry
This commit adds MHI telemetry channel support and registers a DRM RAS
telemetry node during PCI probe so userspace can read and write device
telemetry fields over Generic Netlink.
> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 9e1e184efe0d1..e24a4f271a585 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -32,6 +32,7 @@
> #include "qaic_debugfs.h"
> #include "qaic_ras.h"
> #include "qaic_ssr.h"
> +#include "qaic_telemetry.h"
> #include "qaic_timesync.h"
> #include "sahara.h"
>
> @@ -389,6 +390,7 @@ static void qaic_notify_reset(struct qaic_device *qdev)
> qdev->dev_state = QAIC_OFFLINE;
> /* wake up any waiters to avoid waiting for timeouts at sync */
> wake_all_cntl(qdev);
> + wake_all_telemetry(qdev);
> for (i = 0; i < qdev->num_dbc; ++i)
> wakeup_dbc(qdev, i);
> synchronize_srcu(&qdev->dev_lock);
> @@ -439,6 +441,9 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev,
> if (ret)
> return NULL;
> ret = drmm_mutex_init(drm, &qdev->bootlog_mutex);
> + if (ret)
> + return NULL;
> + ret = drmm_mutex_init(drm, &qdev->tele_mutex);
> if (ret)
> return NULL;
>
> @@ -451,6 +456,9 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev,
> qdev->ssr_wq = qaicm_wq_init(drm, "qaic_ssr");
> if (IS_ERR(qdev->ssr_wq))
> return NULL;
> + qdev->tele_wq = qaicm_wq_init(drm, "qaic_tele");
> + if (IS_ERR(qdev->tele_wq))
> + return NULL;
[Severity: High]
Can this cause a use-after-free of qdev if the PCI device is removed while a
userspace process holds an open file descriptor to /dev/accel/accelN?
In create_qdev(), qdev is allocated with devm_kzalloc(&pdev->dev, ...),
whereas qdev->tele_mutex and qdev->tele_wq are managed by drmm on struct
drm_device. If /dev/accel/accelN is open when qaic_pci_remove() runs:
1. qaic_telemetry_mhi_remove() calls mhi_unprepare_from_transfer(qdev->tele_ch)
without flushing qdev->tele_wq.
2. When qaic_pci_remove() returns, devres_release_all(&pdev->dev) drops the
initial drm_device reference via devm_drm_dev_init_release(), but
drm_managed_release() does not run yet because the open file descriptor
still holds a reference on drm_device.
3. devres_release_all() immediately frees the devm-allocated qdev.
At that point, any pending resp_worker() work item on qdev->tele_wq will
dereference freed qdev memory (qdev->tele_mutex and qdev->tele_xfer_list),
and when the userspace file descriptor is finally closed,
__drmm_mutex_release() will call mutex_destroy(&qdev->tele_mutex) on the
already-freed qdev.
Should qaic_telemetry_mhi_remove() flush qdev->tele_wq, and should the
lifetime of qdev->tele_mutex and qdev->tele_wq match the devm lifetime of
qdev?
[ ... ]
> @@ -611,6 +620,13 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> return ret;
> }
>
> + ret = qaic_drm_ras_register_node(qdev);
> + if (ret) {
> + qaic_mhi_free_controller(qdev->mhi_cntrl, link_up);
> + qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -624,6 +640,7 @@ static void qaic_pci_remove(struct pci_dev *pdev)
> return;
>
> qaic_dev_reset_clean_local_state(qdev);
> + qaic_drm_ras_unregister_node(qdev);
> qaic_mhi_free_controller(qdev->mhi_cntrl, link_up);
> qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
> }
[Severity: High]
Does qaic_pci_remove() risk a use-after-free on qdev and qdev->ras_node if a
netlink telemetry request is in flight during device removal or reset?
In qaic_notify_reset() (called by qaic_dev_reset_clean_local_state()), the
driver sets qdev->dev_state = QAIC_OFFLINE, calls wake_all_telemetry(qdev),
and then calls synchronize_srcu(&qdev->dev_lock) to wait for in-flight
readers.
However, unlike the control path in qaic_control.c, qaic_telemetry_get(),
qaic_telemetry_set(), and telemetry_request() never acquire
srcu_read_lock(&qdev->dev_lock) and never check whether qdev->dev_state is
QAIC_ONLINE. In addition, qaic_pci_remove() runs
qaic_dev_reset_clean_local_state(qdev) before qaic_drm_ras_unregister_node(),
and qaic_drm_ras_unregister_node() frees qdev->ras_node immediately after
drm_ras_node_unregister().
If a netlink caller is sleeping in
wait_for_completion_interruptible_timeout(&elem.xfer_done, HZ) inside
telemetry_request(), or enters telemetry_request() after wake_all_telemetry()
runs while qdev->ras_node is still registered, synchronize_srcu() will not
wait for it. When the waiter wakes up after qaic_pci_remove() completes,
telemetry_request() will call mutex_lock(&qdev->tele_mutex) and
list_del(&elem.list) on freed qdev memory.
Should qaic_drm_ras_unregister_node() be called before
qaic_dev_reset_clean_local_state(), and should the telemetry callbacks guard
access with srcu_read_lock(&qdev->dev_lock) and check qdev->dev_state?
> @@ -860,6 +877,10 @@ static int __init qaic_init(void)
> goto free_bootlog;
> }
>
> + ret = qaic_telemetry_register();
> + if (ret)
> + pr_debug("qaic: qaic_telemetry_register failed %d\n", ret);
> +
> return 0;
>
> free_bootlog:
> @@ -889,6 +910,7 @@ static void __exit qaic_exit(void)
> * reinitializing the link_up state after the cleanup is done.
> */
> link_up = true;
> + qaic_telemetry_unregister();
> qaic_ssr_unregister();
> qaic_ras_unregister();
> qaic_bootlog_unregister();
[Severity: Medium]
If qaic_telemetry_register() fails in qaic_init(), qaic_init() logs a debug
message and returns 0, leaving qaic_telemetry_mhi_driver unregistered
(drv->p == NULL). When the module is later unloaded, qaic_exit()
unconditionally calls qaic_telemetry_unregister() -> mhi_driver_unregister()
-> driver_unregister(), which triggers:
WARN(1, "Unexpected driver unregister!\n");
in drivers/base/driver.c:driver_unregister(). Should qaic_init() unwind and
propagate the error when qaic_telemetry_register() fails, matching
qaic_ssr_register()?
> diff --git a/drivers/accel/qaic/qaic_telemetry.c b/drivers/accel/qaic/qaic_telemetry.c
> new file mode 100644
> index 0000000000000..fdce7a71d3eba
> --- /dev/null
> +++ b/drivers/accel/qaic/qaic_telemetry.c
[ ... ]
> +int telemetry_request(struct qaic_device *qdev, u8 cmd, u8 cmd_type, s64 *val)
> +{
> + struct wrapper_msg *wrapper;
> + struct xfer_queue_elem elem;
> + struct telemetry_msg *resp;
> + struct telemetry_msg *req;
> + long ret = 0;
> +
> + wrapper = kzalloc_obj(*wrapper);
> + if (!wrapper)
> + return -ENOMEM;
> +
> + kref_init(&wrapper->ref_count);
> + req = &wrapper->msg;
> +
> + ret = mutex_lock_interruptible(&qdev->tele_mutex);
> + if (ret)
> + goto free_req;
> +
> + req->hdr.magic = cpu_to_le16(MAGIC);
> + req->hdr.ver = cpu_to_le16(VERSION);
> + req->hdr.seq_num = cpu_to_le32(qdev->tele_next_seq_num++);
> + req->hdr.type = MSG_REQ;
> + req->hdr.id = 0;
> + req->hdr.len = cpu_to_le16(sizeof(req->data));
> +
> + req->data.cmd = cmd;
> + req->data.cmd_type = cmd_type;
> + req->data.status = 0;
> + if (cmd_type == TYPE_READ)
> + req->data.val = cpu_to_le64(0);
> + else
> + req->data.val = cpu_to_le64(*val);
> +
> + elem.seq_num = qdev->tele_next_seq_num - 1;
> + elem.buf = NULL;
> + init_completion(&elem.xfer_done);
> + if (likely(!qdev->tele_lost_buf)) {
> + resp = kzalloc_obj(*resp);
> + if (!resp) {
> + mutex_unlock(&qdev->tele_mutex);
> + ret = -ENOMEM;
> + goto free_req;
> + }
> +
> + ret = mhi_queue_buf(qdev->tele_ch, DMA_FROM_DEVICE, resp, sizeof(*resp), MHI_EOT);
> + if (ret) {
> + mutex_unlock(&qdev->tele_mutex);
> + goto free_resp;
> + }
> + } else {
> + /*
> + * we lost a buffer because we queued a recv buf, but then
> + * queuing the corresponding tx buf failed. To try to avoid
> + * a memory leak, lets reclaim it and use it for this
> + * transaction.
> + */
> + qdev->tele_lost_buf = false;
> + }
> +
> + kref_get(&wrapper->ref_count);
> + ret = mhi_queue_buf(qdev->tele_ch, DMA_TO_DEVICE, req, sizeof(*req), MHI_EOT);
[Severity: High]
What happens if telemetry_request() is called when qdev->tele_ch is NULL, or
while qaic_telemetry_mhi_remove() is running concurrently?
In qaic_pci_probe(), qaic_drm_ras_register_node(qdev) exposes the telemetry
netlink callbacks immediately after qaic_create_drm_device() creates
/dev/accel/accelN. However, qdev->tele_ch is only set asynchronously in
qaic_telemetry_mhi_probe() once the device enters AMSS, and is reset to NULL
in qaic_telemetry_mhi_remove().
If userspace sends a DRM_RAS_CMD_TELEMETRY_GET or DRM_RAS_CMD_TELEMETRY_SET
request before qaic_telemetry_mhi_probe() runs (or after
qaic_telemetry_mhi_remove() clears qdev->tele_ch), telemetry_request() passes
qdev->tele_ch == NULL to mhi_queue_buf() -> mhi_queue(), which immediately
dereferences mhi_dev->mhi_cntrl and crashes.
Furthermore, qaic_telemetry_mhi_probe() and qaic_telemetry_mhi_remove()
modify qdev->tele_ch and qdev->tele_lost_buf and call
mhi_unprepare_from_transfer(qdev->tele_ch) without holding qdev->tele_mutex,
and qaic_telemetry_mhi_remove() does not wake waiters on
qdev->tele_xfer_list. Should telemetry_request() check if (!qdev->tele_ch)
under qdev->tele_mutex (returning -ENODEV), and should
qaic_telemetry_mhi_probe() and qaic_telemetry_mhi_remove() serialize against
qdev->tele_mutex and wake pending waiters?
> + if (ret) {
> + qdev->tele_lost_buf = true;
> + kref_put(&wrapper->ref_count, free_wrapper);
> + mutex_unlock(&qdev->tele_mutex);
> + goto free_req;
> + }
> +
> + list_add_tail(&elem.list, &qdev->tele_xfer_list);
> + mutex_unlock(&qdev->tele_mutex);
> +
> + ret = wait_for_completion_interruptible_timeout(&elem.xfer_done, HZ);
> + /*
> + * not using _interruptable because we have to cleanup or we'll
> + * likely cause memory corruption
> + */
> + mutex_lock(&qdev->tele_mutex);
> + if (!list_empty(&elem.list))
> + list_del(&elem.list);
> + if (!ret && !elem.buf)
> + ret = -ETIMEDOUT;
> + else if (ret > 0 && !elem.buf)
> + ret = -EIO;
> + mutex_unlock(&qdev->tele_mutex);
> +
> + resp = elem.buf;
> +
> + if (ret < 0)
> + goto free_resp;
[Severity: Medium]
What happens here when wait_for_completion_interruptible_timeout() (or the
earlier mutex_lock_interruptible()) is interrupted by a signal and returns
-ERESTARTSYS?
First, if resp_worker() already completed the transfer and set elem.buf
before telemetry_request() acquired qdev->tele_mutex, ret is still
-ERESTARTSYS, so if (ret < 0) branches to free_resp and discards the valid
response (unlike msg_xfer() in qaic_control.c, which resets ret = 0 when
elem.buf is present).
Second, because telemetry_request() is called from Generic Netlink .doit
callbacks (drm_ras_nl_telemetry_get_doit() and
drm_ras_nl_telemetry_set_doit()), returning -ERESTARTSYS causes
netlink_rcv_skb() -> netlink_ack() to serialize the kernel-internal
-ERESTARTSYS (-512) value directly into the NLMSG_ERROR payload sent to
userspace. Should this check for a completed elem.buf and translate any
remaining -ERESTARTSYS to -EINTR?
[ ... ]
> +static int qaic_telemetry_get(struct drm_ras_node *ep, u32 *field, s64 *value, s32 *status)
> +{
> + struct qaic_device *qdev = ep->priv;
> +
> + *status = (s32)telemetry_request(qdev, *field, TYPE_READ, value);
> +
> + return *status;
> +}
> +
> +static int qaic_telemetry_set(struct drm_ras_node *ep, u32 *field, s64 *value, s32 *status)
> +{
> + struct qaic_device *qdev = ep->priv;
> +
> + *status = (s32)telemetry_request(qdev, *field, TYPE_WRITE, value);
> +
> + return *status;
> +}
[Severity: Medium]
In qaic_telemetry_get() and qaic_telemetry_set(), *field is a u32 coming from
the netlink DRM_RAS_A_TELEMETRY_ATTRS_FIELD attribute (which drm_ras does not
validate against telemetry_fields_gettable or telemetry_fields_settable in
the doit path), whereas telemetry_request() takes u8 cmd.
Doesn't passing *field directly to telemetry_request() silently truncate
32-bit field IDs modulo 256 (for example, turning 0x101 into 1,
QAIC_DEVINFO_THERMAL_SOC_MAX_TEMP) instead of rejecting out-of-range or
unsupported field IDs?
[Severity: Medium]
Also in qaic_telemetry_get() and qaic_telemetry_set(), both functions assign
the return value of telemetry_request() to *status and then return *status.
Looking at drm_ras_nl_telemetry_get_doit() and
drm_ras_nl_telemetry_set_doit() in drivers/gpu/drm/drm_ras.c:
ret = node->telemetry_get(node, &field, &value, &status);
if (ret) {
nlmsg_free(msg);
return ret;
}
ret = nla_put_s32(msg, DRM_RAS_A_TELEMETRY_ATTRS_STATUS, status);
When telemetry_request() returns a non-zero error code (such as when the
device returns a firmware status error in resp->data.status), returning
*status from the callback causes drm_ras to free the reply message and abort
before encoding DRM_RAS_A_TELEMETRY_ATTRS_STATUS. Is *status intended to be
reported in the netlink reply when the device returns a status error (with
the callback returning 0)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261029203000.1763447-1-zachary.mckevitt@oss.qualcomm.com?part=3
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/3] Extend drm_ras for qaic telemetry
2026-10-29 20:29 [PATCH v2 0/3] Extend drm_ras for qaic telemetry Zack McKevitt
` (2 preceding siblings ...)
2026-10-29 20:30 ` [PATCH v2 3/3] accel/qaic: Add callbacks for DRM RAS netlink telemetry Zack McKevitt
@ 2026-10-30 16:36 ` Jeff Hugo
3 siblings, 0 replies; 10+ messages in thread
From: Jeff Hugo @ 2026-10-30 16:36 UTC (permalink / raw)
To: Zack McKevitt, dri-devel, netdev, riana.tauro, aravind.iddamsetty,
rodrigo.vivi, raag.jadav, simona.vetter, airlied, kuba, ogabbay,
lizhi.hou, karol.wachowski
Cc: carl.vanderlip, troy.hanson, youssef.abdulrahman
On 10/29/2026 2:29 PM, Zack McKevitt wrote:
> This patch series extends the drm_ras netlink interface to include a new
> node type for telemetry nodes. Telemetry nodes support two operations,
> getting and setting, and drivers may specify an optional predoit function
> to validate each operation.
>
> Telemetry get operations take the drm_ras node id and telemetry field to
> be read and return the field's value and any status reported by the driver.
> Telemetry set operations take the node id, telemetry field, and value,
> and return only the status reported by the driver.
>
> This extension removes the admin requirement on the list-nodes operation.
> This allows drm_ras nodes to be discovered by userspace programs for
> drivers that do not necessarily require admin privileges, like qaic.
>
> The new drm_ras telemetry interface is implemented by the qaic driver,
> which exposes the telemetry fields to userspace and enforces filesystem-
> esque permission checks on telemetry operations in its telemetry_pre
> callback.
Rodrigo,
We are not quite to merging this yet I think, but thinking forward since
things seem to be moving in a positive direction I don't see a drm_ras
entry in maintainers. I seem to recall that initially going through an
Intel tree, but I'm guessing that it should be ok for me to take this
series via drm-misc-next. Hopefully that is ok with you.
^ permalink raw reply [flat|nested] 10+ messages in thread