linux-kernel - Re: [PATCH 06/11] firmware: qcom-shm-bridge: new driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8b7bada9-3898-1b60-3dea-766a760412f7@linaro.org>
Date:   Tue, 29 Aug 2023 10:18:32 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        Alex Elder <elder@...aro.org>,
        Srini Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     kernel@...cinc.com, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 06/11] firmware: qcom-shm-bridge: new driver

On 28/08/2023 21:25, Bartosz Golaszewski wrote:
> This module is a platform driver that also exposes an interface for
> kernel users to allocate blocks of memory shared with the trustzone.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> ---
>  drivers/firmware/Kconfig                 |   8 +
>  drivers/firmware/Makefile                |   1 +
>  drivers/firmware/qcom-shm-bridge.c       | 452 +++++++++++++++++++++++
>  include/linux/firmware/qcom/shm-bridge.h |  32 ++
>  4 files changed, 493 insertions(+)
>  create mode 100644 drivers/firmware/qcom-shm-bridge.c
>  create mode 100644 include/linux/firmware/qcom/shm-bridge.h
> 

...

> +/**
> + * qcom_shm_bridge_to_phys_addr - Translate address from virtual to physical.
> + *
> + * @vaddr: Virtual address to translate.
> + *
> + * Return:
> + * Physical address corresponding to 'vaddr'.
> + */
> +phys_addr_t qcom_shm_bridge_to_phys_addr(void *vaddr)
> +{
> +	struct qcom_shm_bridge_chunk *chunk;
> +	struct qcom_shm_bridge_pool *pool;
> +
> +	guard(spinlock_irqsave)(&qcom_shm_bridge_chunks_lock);
> +
> +	chunk = radix_tree_lookup(&qcom_shm_bridge_chunks,
> +				  (unsigned long)vaddr);
> +	if (!chunk)
> +		return 0;
> +
> +	pool = chunk->parent;
> +
> +	guard(spinlock_irqsave)(&pool->lock);

Why both locks are spinlocks? The locks are used quite a lot.

> +
> +	return gen_pool_virt_to_phys(pool->genpool, (unsigned long)vaddr);
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_to_phys_addr);
> +
> +static int qcom_shm_bridge_probe(struct platform_device *pdev)
> +{
> +	struct qcom_shm_bridge_pool *default_pool;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	/*
> +	 * We need to wait for the SCM device to be created and bound to the
> +	 * SCM driver.
> +	 */
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;

I think we miss here (and in all other drivers) device links to qcm.

> +
> +	ret = qcom_scm_enable_shm_bridge();
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to enable the SHM bridge\n");
> +
> +	default_pool = qcom_shm_bridge_pool_new_for_dev(
> +				dev, qcom_shm_bridge_default_pool_size);
> +	if (IS_ERR(default_pool))
> +		return dev_err_probe(dev, PTR_ERR(default_pool),
> +				     "Failed to create the default SHM Bridge pool\n");
> +
> +	WRITE_ONCE(qcom_shm_bridge_default_pool, default_pool);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id qcom_shm_bridge_of_match[] = {
> +	{ .compatible = "qcom,shm-bridge", },
> +	{ }
> +};
> +
> +static struct platform_driver qcom_shm_bridge_driver = {
> +	.driver = {
> +		.name = "qcom-shm-bridge",
> +		.of_match_table = qcom_shm_bridge_of_match,
> +		/*
> +		 * Once enabled, the SHM Bridge feature cannot be disabled so
> +		 * there's no reason to ever unbind the driver.
> +		 */
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe = qcom_shm_bridge_probe,
> +};
> +
> +static int __init qcom_shm_bridge_init(void)
> +{
> +	return platform_driver_register(&qcom_shm_bridge_driver);
> +}
> +subsys_initcall(qcom_shm_bridge_init);

Why this is part of subsystem? Should be rather device_initcall... or
simply module (and a tristate).

Best regards,
Krzysztof