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;
MIME-Version: 1.0
References: <20230828192507.117334-1-bartosz.golaszewski@linaro.org>
 <20230828192507.117334-7-bartosz.golaszewski@linaro.org> <8b7bada9-3898-1b60-3dea-766a760412f7@linaro.org>
 <CACMJSetObp0k312DmqhTCkw7jsf05OHX1yxbyYj+sVfbtwRcVQ@mail.gmail.com> <86bb50fd-72f3-7c76-c4fe-f8e4765e33d5@linaro.org>
In-Reply-To: <86bb50fd-72f3-7c76-c4fe-f8e4765e33d5@linaro.org>
From:   Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Date:   Wed, 30 Aug 2023 15:09:10 +0200
Message-ID: <CACMJSes9Lwgh+xSDQo4KQhOHdcLigYgi=scFTeCtiqKFt1QNnw@mail.gmail.com>
Subject: Re: [PATCH 06/11] firmware: qcom-shm-bridge: new driver
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Alex Elder <elder@linaro.org>,
        Srini Kandagatla <srinivas.kandagatla@linaro.org>,
        kernel@quicinc.com, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 29 Aug 2023 at 18:47, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> 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.
>

Shouldn't we cross that bridge when we get there? SCM has no support
for runtime PM. Probe ordering is quite well handled with a simple
probe deferral. This is also not a parent-child relationship. SHM
Bridge calls into the trustzone using SCM, but SCM is also a user of
SHM Bridge.

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

I really prefer probe deferral over one platform driver creating
platform devices for another. The device is on the DT, let's let OF
populate it as it should.

Bart