Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <be27a31c-6c63-43cd-9857-f4b2a9568cf6@quicinc.com>
Date:   Thu, 28 Sep 2023 11:19:34 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 02/11] firmware: qcom: scm: add a dedicated SCM memory
 allocator
Content-Language: en-US
To:     Bartosz Golaszewski <brgl@bgdev.pl>,
        Andy Gross <agross@kernel.org>,
        "Bjorn Andersson" <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Maximilian Luz <luzmaximilian@gmail.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
CC:     <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <kernel@quicinc.com>,
        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
References: <20230928092040.9420-1-brgl@bgdev.pl>
 <20230928092040.9420-3-brgl@bgdev.pl>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20230928092040.9420-3-brgl@bgdev.pl>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/28/2023 2:20 AM, Bartosz Golaszewski wrote:
> +void *qcom_scm_mem_alloc(size_t size, gfp_t gfp)
> +{
> +	struct qcom_scm_mem_chunk *chunk;
> +	unsigned long vaddr;

there are places below where you unnecessarily typecast this to its 
given type

> +	int ret;
> +
> +	if (!size)
> +		return ZERO_SIZE_PTR;
> +
> +	size = roundup(size, 1 << PAGE_SHIFT);
> +
> +	chunk = kzalloc(sizeof(*chunk), gfp);
> +	if (!chunk)
> +		return NULL;
> +
> +	vaddr = gen_pool_alloc(qcom_scm_mem.pool, size);
> +	if (!vaddr) {
> +		kfree(chunk);
> +		return NULL;
> +	}
> +
> +	chunk->paddr = gen_pool_virt_to_phys(qcom_scm_mem.pool,
> +					     (unsigned long)vaddr);

unnecessary typecast?

> +	chunk->size = size;
> +
> +	scoped_guard(spinlock_irqsave, &qcom_scm_mem.lock) {

my first exposure to this infrastructure..very cool now that I've 
wrapped my head around it! This helped for those also new to this:
https://lwn.net/Articles/934679/

> +		ret = radix_tree_insert(&qcom_scm_mem.chunks, vaddr, chunk);
> +		if (ret) {
> +			gen_pool_free(qcom_scm_mem.pool, (unsigned long)vaddr,

unnecessary typecast?

> +				      chunk->size);
> +			kfree(chunk);
> +			return NULL;
> +		}
> +	}
> +
> +	return (void *)vaddr;
> +}