linux-kernel - Re: [PATCH v2] soc: qcom: llcc: Add per slice counter and common llcc slice descriptor

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4a438b20-efaa-48d0-a885-c64e664df9e2@oss.qualcomm.com>
Date: Fri, 27 Jun 2025 16:17:42 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Unnathi Chalicheemala <unnathi.chalicheemala@....qualcomm.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>
Cc: quic_satyap@...cinc.com, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@....qualcomm.com
Subject: Re: [PATCH v2] soc: qcom: llcc: Add per slice counter and common llcc
 slice descriptor

On 6/27/25 12:03 AM, Unnathi Chalicheemala wrote:
> When a client driver calls llcc_slice_getd() multiple times or when
> multiple client drivers vote for the same slice, there can be mismatch
> in activate/deactivate count.
> 
> Add an atomic per-slice counter to track the number of
> llcc_slice_activate() and llcc_slice_deactivate() calls per slice.
> Also introduce a common LLCC slice descriptor to ensure consistent
> tracking of slice usage.
> 
> Signed-off-by: Unnathi Chalicheemala <unnathi.chalicheemala@....qualcomm.com>
> ---
> Changes in v2:
> - Dropped bitmap as refcount is replacing it.
> - Modified commit message to explain problem first.
> - Moved NULL pointer checks to the beginning in llcc_slice_getd().
> - Replace incorrect usage of atomic_inc_return() with atomic_inc().
> - Use devm_kcalloc() to allocate memory for common slice descriptor.
> - Link to v1: https://lore.kernel.org/all/20241008194636.3075093-1-quic_uchalich@quicinc.com/
> ---
>  drivers/soc/qcom/llcc-qcom.c       | 60 +++++++++++++++++++++-----------------
>  include/linux/soc/qcom/llcc-qcom.h |  6 ++--
>  2 files changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 192edc3f64dc3eee12ab5ebdb9034cd0e2010891..b0cfdd30967a1a3390a9c0ef6bc01b333244884c 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -3853,30 +3853,25 @@ static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>  struct llcc_slice_desc *llcc_slice_getd(u32 uid)
>  {
>  	const struct llcc_slice_config *cfg;
> -	struct llcc_slice_desc *desc;
>  	u32 sz, count;
>  
> -	if (IS_ERR(drv_data))
> +	if (IS_ERR(drv_data) || !drv_data)

IS_ERR_OR_NULL()

>  		return ERR_CAST(drv_data);
>  
> +	if (IS_ERR_OR_NULL(drv_data->desc) || !drv_data->cfg)
> +		return ERR_PTR(-ENODEV);
> +
>  	cfg = drv_data->cfg;
>  	sz = drv_data->cfg_size;
>  
> -	for (count = 0; cfg && count < sz; count++, cfg++)
> +	for (count = 0; count < sz; count++, cfg++)
>  		if (cfg->usecase_id == uid)
>  			break;
>  
> -	if (count == sz || !cfg)
> +	if (count == sz)
>  		return ERR_PTR(-ENODEV);
>  
> -	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> -	if (!desc)
> -		return ERR_PTR(-ENOMEM);
> -
> -	desc->slice_id = cfg->slice_id;
> -	desc->slice_size = cfg->max_cap;
> -
> -	return desc;

This hunk looks unrelated to the problem you described and could go
into a separate commit (can these ever reasonably be NULL, bar
programmer error?)

> +	return &drv_data->desc[count];

count is a poor name (especially since 'sz' holds the number of slices
we have), we can change it to just 'i' or something

>  }
>  EXPORT_SYMBOL_GPL(llcc_slice_getd);
>  
> @@ -3887,7 +3882,7 @@ EXPORT_SYMBOL_GPL(llcc_slice_getd);
>  void llcc_slice_putd(struct llcc_slice_desc *desc)
>  {
>  	if (!IS_ERR_OR_NULL(desc))
> -		kfree(desc);
> +		WARN(atomic_read(&desc->refcount), " Slice %d is still active\n", desc->slice_id);
>  }
>  EXPORT_SYMBOL_GPL(llcc_slice_putd);
>  
> @@ -3963,7 +3958,8 @@ int llcc_slice_activate(struct llcc_slice_desc *desc)
>  		return -EINVAL;
>  
>  	mutex_lock(&drv_data->lock);
> -	if (test_bit(desc->slice_id, drv_data->bitmap)) {
> +	if ((atomic_read(&desc->refcount)) >= 1) {
> +		atomic_inc(&desc->refcount);

if (atomic_inc_return(&desc->refcount) > 1)

should do the same thing, in a single call.. but then you'd need
to roll back below on failure.. but then regmap should never fail
because it's just sugar syntax for MMIO accesses.. but then we've
had issues before when regmap core broke.. so many dillemas..

>  		mutex_unlock(&drv_data->lock);

You can also switch to scoped guards in a separate commit, which would
be a very welcome change

[...]

>  	if (config->activate_on_init) {
> -		desc.slice_id = config->slice_id;
> -		ret = llcc_slice_activate(&desc);
> +		desc = llcc_slice_getd(config->usecase_id);
> +		if (PTR_ERR_OR_ZERO(desc))
> +			return -EINVAL;

I don't think it can return a nullptr

Konrad