Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 4 Aug 2023 18:48:57 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Maximilian Luz <luzmaximilian@gmail.com>
Cc:     Bjorn Andersson <andersson@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Ard Biesheuvel <ardb@kernel.org>,
        Ilias Apalodimas <ilias.apalodimas@linaro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Steev Klimaszewski <steev@kali.org>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 2/3] firmware: qcom_scm: Add support for Qualcomm
 Secure Execution Environment SCM interface
Message-ID: <ZM0r-ZrkWXBtNZJZ@hovoldconsulting.com>
References: <20230730161906.606163-1-luzmaximilian@gmail.com>
 <20230730161906.606163-3-luzmaximilian@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230730161906.606163-3-luzmaximilian@gmail.com>
Precedence: bulk

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@gmail.com>
> + */
> +#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.

> +
> +#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.

> +
> +	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).

> +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.

> +MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>");
> +MODULE_DESCRIPTION("Driver for the Qualcomm SEE (QSEECOM) interface");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:qcom_qseecom");

No need for MODULE_ALIAS() either.

> +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.

> +}
> +
> +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"?

> +		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:".

> +	 * 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'.

> +
>  	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.

> +
> +#endif /* CONFIG_QCOM_QSEECOM */
> +
>  #endif

With the above fixed you can add my

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

Johan