Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] accel/qda: non-zero remote_session_id sent in qda_fastrpc_invoke_pack() - [Low] accel/qda: commit message references non-existent qda_fastrpc_get_dsp_info() --
commit a3e1eaffe6784dab5dbbf7ef1443bdf4fdd1f0a5 Author: Ekansh Gupta <[email protected]> accel/qda: Add legacy shim support for DSP capability query This commit wires FASTRPC_IOCTL_GET_DSP_INFO into the legacy FastRPC compatibility shim so user space can query DSP capabilities through /dev/fastrpc-* devices using the shared capability cache. > accel/qda: Add legacy shim support for DSP capability query > > Wire FASTRPC_IOCTL_GET_DSP_INFO into the legacy FastRPC compatibility > shim on top of qda_fastrpc_get_dsp_info(), so existing user space can > query DSP capabilities through the /dev/fastrpc-* interface exactly > as it could with the driver this one replaces. > > The device-wide capability cache introduced for the native > DRM_IOCTL_QDA_QUERY path is reused as-is; the shim only has to > translate between the legacy fastrpc_ioctl_capability layout and the > shared qda_fastrpc_get_dsp_info() call. > > Signed-off-by: Ekansh Gupta <[email protected]> [Severity: Low] This isn't a bug, but the commit message states twice that FASTRPC_IOCTL_GET_DSP_INFO is wired on top of qda_fastrpc_get_dsp_info(). Should this reference qda_get_dsp_capability() instead, since legacy_get_dsp_info() in drivers/accel/qda/qda_legacy_ops.c delegates to qda_get_dsp_capability() and no function named qda_fastrpc_get_dsp_info() exists in the codebase? > diff --git a/drivers/accel/qda/qda_legacy_ops.c > b/drivers/accel/qda/qda_legacy_ops.c > index fc5e94c0c9bcd..8d9d597c76c26 100644 > --- a/drivers/accel/qda/qda_legacy_ops.c > +++ b/drivers/accel/qda/qda_legacy_ops.c > @@ -188,6 +188,11 @@ static int legacy_invoke(struct qda_session *session, > u32 handle, u32 sc, > return qda_fastrpc_invoke_legacy(session->qda_dev, session, handle, sc, > args); > } > > +static int legacy_get_dsp_info(struct qda_session *session, u32 > attribute_id, u32 *capability) > +{ > + return qda_get_dsp_capability(session->qda_dev, session, attribute_id, > capability); > +} [Severity: Medium] When user space opens /dev/fastrpc-* (which assigns a positive session->remote_session_id via qda_session_alloc()) and issues FASTRPC_IOCTL_GET_DSP_INFO before creating a DSP process, will the DSP accept the capability query with a non-zero msg->fastrpc.remote_session_id? When qdev->valid_attributes is false, the call chain: qda_shim_get_dsp_info() legacy_get_dsp_info() qda_get_dsp_capability() qda_fastrpc_invoke(QDA_FASTRPC_TYPE_GET_DSP_INFO, ...) qda_fastrpc_invoke_common() qda_fastrpc_invoke_pack() packs the request with ctx->handle set to QDA_FASTRPC_DSP_UTILITIES_HANDLE. However, in drivers/accel/qda/qda_fastrpc.c, qda_fastrpc_invoke_pack() only zeroes remote_session_id for QDA_FASTRPC_INIT_HANDLE: /* The session has no process yet to identify while one is being created */ if (ctx->handle == QDA_FASTRPC_INIT_HANDLE) msg->fastrpc.remote_session_id = 0; else msg->fastrpc.remote_session_id = ctx->session->remote_session_id; In contrast, in drivers/misc/fastrpc.c, fastrpc_get_info_from_dsp() calls fastrpc_internal_invoke() with kernel = true, and fastrpc_invoke_send() sets msg->client_id = 0 whenever kernel is true. Should qda_fastrpc_invoke_pack() also set msg->fastrpc.remote_session_id = 0 when ctx->handle == QDA_FASTRPC_DSP_UTILITIES_HANDLE so that capability queries match the legacy driver and do not pass an uninitialized DSP process's session ID? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=24
