Re: [PATCH v5 2/3] firmware: qcom_scm: Add support for Qualcomm Secure Execution Environment SCM interface
From: Maximilian Luz
Date: Fri Aug 04 2023 - 16:11:52 EST
On 8/4/23 18:48, Johan Hovold wrote:
On Sun, Jul 30, 2023 at 06:19:03PM +0200, Maximilian Luz wrote:
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for Qualcomm Secure Execution Environment (SEE) interface (QSEECOM).
+ * Responsible for setting up and managing QSEECOM client devices.
+ *
+ * Copyright (C) 2023 Maximilian Luz <luzmaximilian@xxxxxxxxx>
+ */
+#include <linux/auxiliary_bus.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
Looks like you're missing some includes like module.h and slab.h.
Right. I'll add both for the next version.
+
+#include <linux/firmware/qcom/qcom_qseecom.h>
+#include <linux/firmware/qcom/qcom_scm.h>
+static void qseecom_client_release(struct device *dev)
+{
+ struct qseecom_client *client = container_of(dev, struct qseecom_client, aux_dev.dev);
Nit: Perhaps you can separate declaration and initialisation here to
stay within 80 columns.
Sure, I'll do that.
+
+ kfree(client);
+}
+static int qcom_qseecom_remove(struct platform_device *qseecom_dev)
+{
+ return 0; /* Nothing to do here, all is managed via devm. */
+}
You should just drop this one (even if it serves as documentation).
Okay.
+static struct platform_driver qcom_qseecom_driver = {
+ .driver = {
+ .name = "qcom_qseecom",
+ },
+ .probe = qcom_qseecom_probe,
+ .remove = qcom_qseecom_remove,
+};
+
+static int __init qcom_qseecom_init(void)
+{
+ return platform_driver_register(&qcom_qseecom_driver);
+}
+subsys_initcall(qcom_qseecom_init);
+
+static void __exit qcom_qseecom_exit(void)
+{
+ platform_driver_unregister(&qcom_qseecom_driver);
+}
+module_exit(qcom_qseecom_exit);
No need for this one either since this driver can only be built-in now.
Right.
+MODULE_AUTHOR("Maximilian Luz <luzmaximilian@xxxxxxxxx>");
+MODULE_DESCRIPTION("Driver for the Qualcomm SEE (QSEECOM) interface");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:qcom_qseecom");
No need for MODULE_ALIAS() either.
Oh right. As long as the module and device name match this should work
automatically, correct? I forgot about that.
+static void qcom_scm_qseecom_free(void *data)
+{
+ struct platform_device *qseecom_dev = data;
+
+ platform_device_unregister(qseecom_dev);
Perhaps use platform_device_del() and platform_device_put() for symmetry
as you're not using platform_device_register() below.
Sure, I can do that.
+}
+
+static int qcom_scm_qseecom_init(struct qcom_scm *scm)
+{
+ struct platform_device *qseecom_dev;
+ u32 version;
+ int ret;
+
+ /*
+ * Note: We do two steps of validation here: First, we try to query the
+ * QSEECOM version as a check to see if the interface exists on this
+ * device. Second, we check against known good devices due to current
+ * driver limitations (see comment in qcom_scm_qseecom_allowlist).
+ *
+ * Note that we deliberately do the machine check after the version
+ * check so that we can log potentially supported devices. This should
+ * be safe as downstream sources indicate that the version query is
+ * neither blocking nor reentrant.
+ */
+ ret = qcom_scm_qseecom_get_version(&version);
+ if (ret)
+ return 0;
+
+ dev_info(scm->dev, "qseecom: found qseecom with version 0x%x\n", version);
+
+ if (!qcom_scm_qseecom_machine_is_allowed()) {
+ dev_info(scm->dev, "qseecom: untested device, skipping\n");
untested "machine"?
That would be more consistent, yes.
+ return 0;
+ }
+
+ /*
+ * Set up QSEECOM interface device. All application clients will be
+ * set up and managed by the corresponding driver for it.
+ */
+ qseecom_dev = platform_device_alloc("qcom_qseecom", -1);
+ if (!qseecom_dev)
+ return -ENOMEM;
+
+ qseecom_dev->dev.parent = scm->dev;
+
+ ret = platform_device_add(qseecom_dev);
+ if (ret) {
+ platform_device_put(qseecom_dev);
+ return ret;
+ }
+
+ return devm_add_action_or_reset(scm->dev, qcom_scm_qseecom_free, qseecom_dev);
+}
+
+#else /* CONFIG_QCOM_QSEECOM */
+
+static int qcom_scm_qseecom_init(struct qcom_scm *scm)
+{
+ return 0;
+}
+
+#endif /* CONFIG_QCOM_QSEECOM */
+
/**
* qcom_scm_is_available() - Checks if SCM is available
*/
@@ -1468,6 +1848,18 @@ static int qcom_scm_probe(struct platform_device *pdev)
if (download_mode)
qcom_scm_set_download_mode(true);
+ /*
+ * Initialize the QSEECOM interface. Note: QSEECOM is fairly
Nit: I'd add a line break and an empty line before the "Note:".
Sure, I'll do that.
+ * self-contained and this only adds the interface device (the driver
+ * of which does most of the heavy lifting). So any errors returned
+ * here should be either -ENOMEM or -EINVAL (with the latter only in
+ * case there's a bug in our code). This means that there is no need to
+ * bring down the whole SCM driver. Just log the error instead and let
+ * SCM live.
+ */
+ ret = qcom_scm_qseecom_init(scm);
+ WARN(ret < 0, "failed to initialize qseecom: %d", ret);
Missing '\n'.
Right.
+
return 0;
}
+#ifdef CONFIG_QCOM_QSEECOM
+
+int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id);
+int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
+ size_t rsp_size);
+
+#else /* CONFIG_QCOM_QSEECOM */
+
+int qcom_scm_qseecom_app_get_id(const char *app_name, u32 *app_id)
+{
+ return -EINVAL;
+}
+
+int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
+ size_t rsp_size)
+{
+ return -EINVAL;
+}
These should be static inline as you already noticed.
Already done :)
+
+#endif /* CONFIG_QCOM_QSEECOM */
+
#endif
With the above fixed you can add my
Reviewed-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
Thanks!
Regards
Max