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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 28/08/2023 21:25, Bartosz Golaszewski wrote:
> +/**
> + * qcom_shm_bridge_pool_new - Create a new SHM Bridge memory pool.
> + *
> + * @size: Size of the pool.
> + *
> + * Creates a new Shared Memory Bridge pool from which users can allocate memory
> + * chunks. Must be called from process context.
> + *
> + * Return:
> + * Pointer to the newly created SHM Bridge pool with reference count set to 1
> + * or ERR_PTR().
> + */
> +struct qcom_shm_bridge_pool *qcom_shm_bridge_pool_new(size_t size)
> +{
> +	struct device *dev;
> +
> +	dev = bus_find_device_by_name(&platform_bus_type, NULL,
> +				      "qcom-shm-bridge");
> +	if (!dev)
> +		return ERR_PTR(-ENODEV);
> +
> +	return qcom_shm_bridge_pool_new_for_dev(dev, size);
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_pool_new);

I do not see an user of this. Do not add exported symbols without users.
Drop export. I would even suggest drop the function entirely, why do we
need dead code?

> +
> +/**
> + * qcom_shm_bridge_pool_ref - Increate the refcount of an SHM Bridge pool.
> + *
> + * @pool: SHM Bridge pool of which the reference count to increase.
> + *
> + * Return:
> + * Pointer to the same pool object.
> + */
> +struct qcom_shm_bridge_pool *
> +qcom_shm_bridge_pool_ref(struct qcom_shm_bridge_pool *pool)
> +{
> +	kref_get(&pool->refcount);
> +
> +	return pool;
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_pool_ref);

Ditto

> +
> +/**
> + * qcom_shm_bridge_pool_unref - Decrease the refcount of an SHM Bridge pool.
> + *
> + * @pool: SHM Bridge pool of which the reference count to decrease.
> + *
> + * Once the reference count reaches 0, the pool is released.
> + */
> +void qcom_shm_bridge_pool_unref(struct qcom_shm_bridge_pool *pool)
> +{
> +	kref_put(&pool->refcount, qcom_shm_bridge_pool_release);
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_pool_unref);

Ditto

> +
> +static void devm_qcom_shm_bridge_pool_unref(void *data)
> +{
> +	struct qcom_shm_bridge_pool *pool = data;
> +
> +	qcom_shm_bridge_pool_unref(pool);
> +}
> +
> +/**
> + * devm_qcom_shm_bridge_pool_new - Managed variant of qcom_shm_bridge_pool_new.
> + *
> + * @dev: Device for which to map memory and which will manage this pool.
> + * @size: Size of the pool.
> + *
> + * Return:
> + * Pointer to the newly created SHM Bridge pool with reference count set to 1
> + * or ERR_PTR().
> + */
> +struct qcom_shm_bridge_pool *
> +devm_qcom_shm_bridge_pool_new(struct device *dev, size_t size)
> +{
> +	struct qcom_shm_bridge_pool *pool;
> +	int ret;
> +
> +	pool = qcom_shm_bridge_pool_new(size);
> +	if (IS_ERR(pool))
> +		return pool;
> +
> +	ret = devm_add_action_or_reset(dev, devm_qcom_shm_bridge_pool_unref,
> +				       pool);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return pool;
> +}
> +EXPORT_SYMBOL_GPL(devm_qcom_shm_bridge_pool_new);

Ditto

> +
> +/**
> + * qcom_shm_bridge_alloc - Allocate a chunk of memory from an SHM Bridge pool.
> + *
> + * @pool: Pool to allocate memory from. May be NULL.
> + * @size: Number of bytes to allocate.
> + * @gfp: Allocation flags.
> + *
> + * If pool is NULL then the global fall-back pool is used.
> + *
> + * Return:
> + * Virtual address of the allocated memory or ERR_PTR(). Must be freed using
> + * qcom_shm_bridge_free().
> + */
> +void *qcom_shm_bridge_alloc(struct qcom_shm_bridge_pool *pool,
> +			    size_t size, gfp_t gfp)
> +{
> +	struct qcom_shm_bridge_chunk *chunk __free(kfree) = NULL;
> +	unsigned long vaddr;
> +	int ret;

...

> +
> +	return no_free_ptr(chunk)->vaddr;
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_alloc);
> +
> +/**
> + * qcom_shm_bridge_free - Free SHM Bridge memory allocated from the pool.
> + *
> + * @vaddr: Virtual address of the allocated memory to free.
> + */
> +void qcom_shm_bridge_free(void *vaddr)
> +{
> +	struct qcom_shm_bridge_chunk *chunk;
> +	struct qcom_shm_bridge_pool *pool;
> +
> +	scoped_guard(spinlock_irqsave, &qcom_shm_bridge_chunks_lock)
> +		chunk = radix_tree_delete_item(&qcom_shm_bridge_chunks,
> +					       (unsigned long)vaddr, NULL);
> +	if (!chunk)
> +		goto out_warn;
> +
> +	pool = chunk->parent;
> +
> +	guard(spinlock_irqsave)(&pool->lock);
> +
> +	list_for_each_entry(chunk, &pool->chunks, siblings) {
> +		if (vaddr != chunk->vaddr)
> +			continue;
> +
> +		gen_pool_free(pool->genpool, (unsigned long)chunk->vaddr,
> +			      chunk->size);
> +		list_del(&chunk->siblings);
> +		qcom_shm_bridge_pool_unref(pool);
> +		kfree(chunk);
> +		return;
> +	}
> +
> +out_warn:
> +	WARN(1, "Virtual address %p not allocated for SHM bridge", vaddr);
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_free);
> +

...

> +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);
> +
> +	return gen_pool_virt_to_phys(pool->genpool, (unsigned long)vaddr);
> +}
> +EXPORT_SYMBOL_GPL(qcom_shm_bridge_to_phys_addr);

Ditto

Best regards,
Krzysztof