[PATCH] soc: qcom: llcc: Add per slice counter and common llcc slice descriptor

[PATCH] soc: qcom: llcc: Add per slice counter and common llcc slice descriptor

[Unnathi Chalicheemala]

Introduce atomic per-slice counter to keep track of the
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.

Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.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)
+	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);
+		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);
 	__set_bit(desc->slice_id, drv_data->bitmap);
 	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);
+	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

Re: [PATCH] soc: qcom: llcc: Add per slice counter and common llcc slice descriptor

[Bjorn Andersson]

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@quicinc.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
>