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

[Bartosz Golaszewski]

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

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.

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

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

Bartosz

> Best regards,
> Krzysztof
>