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: <86bb50fd-72f3-7c76-c4fe-f8e4765e33d5@linaro.org>
Date:   Tue, 29 Aug 2023 18:47:40 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Cc:     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>,
        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 29/08/2023 15:24, Bartosz Golaszewski wrote:
>>> +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.
> 
> I'm not sure what to answer. The first one protects the global chunk
> mapping stored in the radix tree. The second one protects a single
> memory pool from concurrent access. Both can be modified from any
> context, hence spinlocks.

Not much PREEMPT friendly, although indeed protected code is small. At
least here, I did not check other places.

> 
>>
>>> +
>>> +     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.
>>
> 
> Well, SCM, once probed, cannot be unbound. What would device links
> guarantee above that?

Runtime PM, probe ordering (dependencies) detection.

> 
>>> +
>>> +     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).
>>
> 
> We want it to get up as soon as possible (right after SCM, because SCM
> is the first user).

Then probably should be populated/spawned by SCM.

Best regards,
Krzysztof