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@xxxxxxxxxxx> > --- > 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 >
