Message-ID: <lewsbn6t5ysemyk2gt632hdgl2wuhr2qxpblmkiuu3aiivsu5g@phgwetzycwyx>
Date: Tue, 22 Oct 2024 23:30:40 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Unnathi Chalicheemala <quic_uchalich@...cinc.com>
Cc: Konrad Dybcio <konradybcio@...nel.org>, linux-arm-msm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, kernel@...cinc.com
Subject: Re: [PATCH] soc: qcom: llcc: Add per slice counter and common llcc
 slice descriptor

On Tue, Oct 08, 2024 at 12:46:36PM GMT, Unnathi Chalicheemala wrote:
> Introduce atomic per-slice counter to keep track of the

Why an atomic? If it's always managed under a lock, an integer should do
just fine (and if not...it's probably broken ;))

> activate/deactivate count per slice and a common llcc slice
> descriptor to maintain accurate count.
> 
> In case a client driver calls llcc_slice_getd more than once,
> it will get the same descriptor for given use case. And if two
> client drivers are voting for same slice, this count variable
> will help track enable/disable of slice accurately.
> 

Please flip the commit message around, to match the style described in 
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes

Also, please add some () to your function names in the description.

> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@...cinc.com>
> ---
>  drivers/soc/qcom/llcc-qcom.c       | 50 +++++++++++++++++++++---------
>  include/linux/soc/qcom/llcc-qcom.h |  4 +++
>  2 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 8fa4ffd3a9b5..0cb4fd18fd2c 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -813,7 +813,6 @@ 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))
> @@ -826,17 +825,10 @@ struct llcc_slice_desc *llcc_slice_getd(u32 uid)
>  		if (cfg->usecase_id == uid)
>  			break;
>  
> -	if (count == sz || !cfg)

cfg starts out as a pointer the first element in an array of
struct llcc_slice_config, for each iteration in the loop it moves
forward once sizeof(struct llcc_slice_config)... how can it become NULL?

Perhaps I'm reading the code wrong?

> +	if (count == sz || !cfg || IS_ERR_OR_NULL(drv_data->desc))
>  		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;
> +	return &drv_data->desc[count];
>  }
>  EXPORT_SYMBOL_GPL(llcc_slice_getd);
>  
> @@ -847,7 +839,8 @@ 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);
>  
> @@ -923,6 +916,12 @@ int llcc_slice_activate(struct llcc_slice_desc *desc)
>  		return -EINVAL;
>  
>  	mutex_lock(&drv_data->lock);
> +	if ((atomic_read(&desc->refcount)) >= 1) {
> +		atomic_inc_return(&desc->refcount);

Two separate atomic operations are no longer atomic... But as I stated
above, it doesn't matter if mutual exclusion is provided by the mutex.

> +		mutex_unlock(&drv_data->lock);
> +		return 0;
> +	}
> +
>  	if (test_bit(desc->slice_id, drv_data->bitmap)) {
>  		mutex_unlock(&drv_data->lock);
>  		return 0;
> @@ -937,6 +936,7 @@ int llcc_slice_activate(struct llcc_slice_desc *desc)
>  		return ret;
>  	}
>  
> +	atomic_inc_return(&desc->refcount);

The difference between atomic_inc() and atomic_inc_return() is the
return type... which you ignore... 

>  	__set_bit(desc->slice_id, drv_data->bitmap);

Do you need both the bitmap and the refcount?

>  	mutex_unlock(&drv_data->lock);
>  
> @@ -963,6 +963,12 @@ int llcc_slice_deactivate(struct llcc_slice_desc *desc)
>  		return -EINVAL;
>  
>  	mutex_lock(&drv_data->lock);
> +	if ((atomic_read(&desc->refcount)) > 1) {
> +		atomic_dec_return(&desc->refcount);
> +		mutex_unlock(&drv_data->lock);
> +		return 0;
> +	}
> +
>  	if (!test_bit(desc->slice_id, drv_data->bitmap)) {
>  		mutex_unlock(&drv_data->lock);
>  		return 0;
> @@ -976,6 +982,7 @@ int llcc_slice_deactivate(struct llcc_slice_desc *desc)
>  		return ret;
>  	}
>  
> +	atomic_dec_return(&desc->refcount);
>  	__clear_bit(desc->slice_id, drv_data->bitmap);
>  	mutex_unlock(&drv_data->lock);
>  
> @@ -1020,7 +1027,7 @@ static int _qcom_llcc_cfg_program(const struct llcc_slice_config *config,
>  	u32 attr1_val;
>  	u32 attr0_val;
>  	u32 max_cap_cacheline;
> -	struct llcc_slice_desc desc;
> +	struct llcc_slice_desc *desc;
>  
>  	attr1_val = config->cache_mode;
>  	attr1_val |= config->probe_target_ways << ATTR1_PROBE_TARGET_WAYS_SHIFT;
> @@ -1165,8 +1172,11 @@ static int _qcom_llcc_cfg_program(const struct llcc_slice_config *config,
>  	}
>  
>  	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;
> +
> +		ret = llcc_slice_activate(desc);
>  	}
>  
>  	return ret;
> @@ -1183,6 +1193,12 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev,
>  	sz = drv_data->cfg_size;
>  	llcc_table = drv_data->cfg;
>  
> +	for (i = 0; i < sz; i++) {
> +		drv_data->desc[i].slice_id = llcc_table[i].slice_id;
> +		drv_data->desc[i].slice_size = llcc_table[i].max_cap;
> +		atomic_set(&drv_data->desc[i].refcount, 0);
> +	}
> +
>  	for (i = 0; i < sz; i++) {
>  		ret = _qcom_llcc_cfg_program(&llcc_table[i], cfg);
>  		if (ret)
> @@ -1331,6 +1347,12 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  	llcc_cfg = cfg->sct_data;
>  	sz = cfg->size;
>  
> +	drv_data->desc = devm_kzalloc(dev, sizeof(struct llcc_slice_desc) * sz, GFP_KERNEL);

That's a devm_kcalloc()

Regards,
Bjorn

> +	if (IS_ERR_OR_NULL(drv_data->desc)) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
>  	for (i = 0; i < sz; i++)
>  		if (llcc_cfg[i].slice_id > drv_data->max_slices)
>  			drv_data->max_slices = llcc_cfg[i].slice_id;
> diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
> index 9e9f528b1370..5eca861e2837 100644
> --- a/include/linux/soc/qcom/llcc-qcom.h
> +++ b/include/linux/soc/qcom/llcc-qcom.h
> @@ -60,10 +60,12 @@
>   * struct llcc_slice_desc - Cache slice descriptor
>   * @slice_id: llcc slice id
>   * @slice_size: Size allocated for the llcc slice
> + * @refcount: Counter to track activate/deactivate slice count
>   */
>  struct llcc_slice_desc {
>  	u32 slice_id;
>  	size_t slice_size;
> +	atomic_t refcount;
>  };
>  
>  /**
> @@ -126,6 +128,7 @@ struct llcc_edac_reg_offset {
>   * @bitmap: Bit map to track the active slice ids
>   * @ecc_irq: interrupt for llcc cache error detection and reporting
>   * @version: Indicates the LLCC version
> + * @desc: Array pointer of llcc_slice_desc
>   */
>  struct llcc_drv_data {
>  	struct regmap **regmaps;
> @@ -140,6 +143,7 @@ struct llcc_drv_data {
>  	unsigned long *bitmap;
>  	int ecc_irq;
>  	u32 version;
> +	struct llcc_slice_desc *desc;
>  };
>  
>  #if IS_ENABLED(CONFIG_QCOM_LLCC)
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists