hwmon: coretemp: use dynamically allocated array to store per core data

hwmon: coretemp: use dynamically allocated array to store per core data

[Lukasz Odzioba]

This patch is continuation of my previous work. After Guenter Roeck's and Jean Delware's comments I changed list to an array. Initial array size is 4 and it is extended when core id exeeds size of the array, which in practice can mean every time a new core is initialized. I am not sure whether it is reasonable since it implies a lot of krealloc calls with small size increments. Please let me know if you think that more greedy approach would be better here. 

Comments are welcome.

[PATCH 1/1] hwmon: coretemp: use dynamically allocated array to store per core data

[Lukasz Odzioba]

Removes arbitrary limit of supported CPU cores and max core ID.
Replaces fixed size array storing per core information with dynamically allocated one.

Currently coretemp is not able to handle cores with core ID greater than 32.
Such attempt ends up with the following error in dmesg:
coretemp coretemp.0: Adding Core XXX failed

Signed-off-by: Lukasz Odzioba <lukasz.odzioba@intel.com>
---
 drivers/hwmon/coretemp.c |   94 +++++++++++++++++++++++++++++++++++++---------
 1 files changed, 76 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 3e03379..1e60039 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -52,11 +52,9 @@ module_param_named(tjmax, force_tjmax, int, 0444);
 MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
 
 #define BASE_SYSFS_ATTR_NO	2	/* Sysfs Base attr no for coretemp */
-#define NUM_REAL_CORES		32	/* Number of Real cores per cpu */
 #define CORETEMP_NAME_LENGTH	19	/* String Length of attrs */
 #define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
 #define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
-#define MAX_CORE_DATA		(NUM_REAL_CORES + BASE_SYSFS_ATTR_NO)
 
 #define TO_PHYS_ID(cpu)		(cpu_data(cpu).phys_proc_id)
 #define TO_CORE_ID(cpu)		(cpu_data(cpu).cpu_core_id)
@@ -104,7 +102,9 @@ struct temp_data {
 struct platform_data {
 	struct device *hwmon_dev;
 	u16 phys_proc_id;
-	struct temp_data *core_data[MAX_CORE_DATA];
+	u32 core_data_size;
+	struct temp_data **core_data;
+	struct mutex core_data_lock;
 	struct device_attribute name_attr;
 };
 
@@ -117,12 +117,25 @@ struct pdev_entry {
 static LIST_HEAD(pdev_list);
 static DEFINE_MUTEX(pdev_list_mutex);
 
+static struct temp_data *get_core_data(struct platform_data *pdata, int index)
+{
+	struct temp_data *tdata;
+
+	mutex_lock(&pdata->core_data_lock);
+	if (index >= pdata->core_data_size)
+		tdata = NULL;
+	else
+		tdata = pdata->core_data[index];
+	mutex_unlock(&pdata->core_data_lock);
+	return tdata;
+}
+
 static ssize_t show_label(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = get_core_data(pdata, attr->index);
 
 	if (tdata->is_pkg_data)
 		return sprintf(buf, "Physical id %u\n", pdata->phys_proc_id);
@@ -136,7 +149,7 @@ static ssize_t show_crit_alarm(struct device *dev,
 	u32 eax, edx;
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = get_core_data(pdata, attr->index);
 
 	rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
 
@@ -148,8 +161,9 @@ static ssize_t show_tjmax(struct device *dev,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct platform_data *pdata = dev_get_drvdata(dev);
+	struct temp_data *tdata = get_core_data(pdata, attr->index);
 
-	return sprintf(buf, "%d\n", pdata->core_data[attr->index]->tjmax);
+	return sprintf(buf, "%d\n", tdata->tjmax);
 }
 
 static ssize_t show_ttarget(struct device *dev,
@@ -157,8 +171,9 @@ static ssize_t show_ttarget(struct device *dev,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct platform_data *pdata = dev_get_drvdata(dev);
+	struct temp_data *tdata = get_core_data(pdata, attr->index);
 
-	return sprintf(buf, "%d\n", pdata->core_data[attr->index]->ttarget);
+	return sprintf(buf, "%d\n", tdata->ttarget);
 }
 
 static ssize_t show_temp(struct device *dev,
@@ -167,7 +182,8 @@ static ssize_t show_temp(struct device *dev,
 	u32 eax, edx;
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct platform_data *pdata = dev_get_drvdata(dev);
-	struct temp_data *tdata = pdata->core_data[attr->index];
+	struct temp_data *tdata = get_core_data(pdata, attr->index);
+
 
 	mutex_lock(&tdata->update_lock);
 
@@ -485,8 +501,23 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 	 */
 	attr_no = pkg_flag ? 1 : TO_ATTR_NO(cpu);
 
-	if (attr_no > MAX_CORE_DATA - 1)
-		return -ERANGE;
+	if (attr_no >= pdata->core_data_size) {
+		struct temp_data **tmp;
+		u32 new_size = attr_no + 1;
+
+		get_online_cpus();
+		mutex_lock(&pdata->core_data_lock);
+		tmp = krealloc(pdata->core_data, new_size * sizeof(struct temp_data*), GFP_ATOMIC | __GFP_ZERO);
+		if (tmp == NULL) {
+			mutex_unlock(&pdata->core_data_lock);
+			put_online_cpus();
+			return -ENOMEM;
+		}
+		pdata->core_data = tmp;
+		pdata->core_data_size = new_size;
+		mutex_unlock(&pdata->core_data_lock);
+		put_online_cpus();
+	}
 
 	/*
 	 * Provide a single set of attributes for all HT siblings of a core
@@ -495,7 +526,8 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 	 * Skip if a HT sibling of this core is already registered.
 	 * This is not an error.
 	 */
-	if (pdata->core_data[attr_no] != NULL)
+	tdata = get_core_data(pdata, attr_no);
+	if (tdata)
 		return 0;
 
 	tdata = init_temp_data(cpu, pkg_flag);
@@ -525,7 +557,9 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 		}
 	}
 
+	mutex_lock(&pdata->core_data_lock);
 	pdata->core_data[attr_no] = tdata;
+	mutex_unlock(&pdata->core_data_lock);
 
 	/* Create sysfs interfaces */
 	err = create_core_attrs(tdata, pdata->hwmon_dev, attr_no);
@@ -534,7 +568,9 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 
 	return 0;
 exit_free:
+	mutex_lock(&pdata->core_data_lock);
 	pdata->core_data[attr_no] = NULL;
+	mutex_unlock(&pdata->core_data_lock);
 	kfree(tdata);
 	return err;
 }
@@ -555,26 +591,35 @@ static void coretemp_add_core(unsigned int cpu, int pkg_flag)
 static void coretemp_remove_core(struct platform_data *pdata,
 				 int indx)
 {
-	struct temp_data *tdata = pdata->core_data[indx];
+	struct temp_data *tdata = get_core_data(pdata, indx);
 
 	/* Remove the sysfs attributes */
 	sysfs_remove_group(&pdata->hwmon_dev->kobj, &tdata->attr_group);
 
-	kfree(pdata->core_data[indx]);
+	mutex_lock(&pdata->core_data_lock);
+	kfree(tdata);
 	pdata->core_data[indx] = NULL;
+	mutex_unlock(&pdata->core_data_lock);
 }
 
 static int coretemp_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct platform_data *pdata;
+	const u32 init_size = 4;
 
 	/* Initialize the per-package data structures */
 	pdata = devm_kzalloc(dev, sizeof(struct platform_data), GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
 
+	pdata->core_data = kcalloc(init_size, sizeof(struct temp_data*), GFP_KERNEL);
+	if (!pdata->core_data)
+		return -ENOMEM;
+
 	pdata->phys_proc_id = pdev->id;
+	mutex_init(&pdata->core_data_lock);
+	pdata->core_data_size = init_size;
 	platform_set_drvdata(pdev, pdata);
 
 	pdata->hwmon_dev = devm_hwmon_device_register_with_groups(dev, DRVNAME,
@@ -587,9 +632,12 @@ static int coretemp_remove(struct platform_device *pdev)
 	struct platform_data *pdata = platform_get_drvdata(pdev);
 	int i;
 
-	for (i = MAX_CORE_DATA - 1; i >= 0; --i)
+	get_online_cpus();
+	for (i = pdata->core_data_size - 1; i >= 0; --i)
 		if (pdata->core_data[i])
 			coretemp_remove_core(pdata, i);
+	put_online_cpus();
+	kfree(pdata->core_data);
 
 	return 0;
 }
@@ -667,12 +715,15 @@ static bool is_any_core_online(struct platform_data *pdata)
 	int i;
 
 	/* Find online cores, except pkgtemp data */
-	for (i = MAX_CORE_DATA - 1; i >= 0; --i) {
+	mutex_lock(&pdata->core_data_lock);
+	for (i = pdata->core_data_size - 1; i >= 0; --i) {
 		if (pdata->core_data[i] &&
 			!pdata->core_data[i]->is_pkg_data) {
+			mutex_unlock(&pdata->core_data_lock);
 			return true;
 		}
 	}
+	mutex_unlock(&pdata->core_data_lock);
 	return false;
 }
 
@@ -723,6 +774,7 @@ static void put_core_offline(unsigned int cpu)
 	int i, indx;
 	struct platform_data *pdata;
 	struct platform_device *pdev = coretemp_get_pdev(cpu);
+	struct temp_data *tdata;
 
 	/* If the physical CPU device does not exist, just return */
 	if (!pdev)
@@ -732,12 +784,18 @@ static void put_core_offline(unsigned int cpu)
 
 	indx = TO_ATTR_NO(cpu);
 
-	/* The core id is too big, just return */
-	if (indx > MAX_CORE_DATA - 1)
+	if (indx >= pdata->core_data_size)
 		return;
 
-	if (pdata->core_data[indx] && pdata->core_data[indx]->cpu == cpu)
+	get_online_cpus();
+	tdata = get_core_data(pdata, indx);
+	if (!tdata) {
+		put_online_cpus();
+		return;
+	}
+	if (tdata->cpu == cpu)
 		coretemp_remove_core(pdata, indx);
+	put_online_cpus();
 
 	/*
 	 * If a HT sibling of a core is taken offline, but another HT sibling
-- 
1.7.1

Re: [PATCH 1/1] hwmon: coretemp: use dynamically allocated array to store per core data

[Guenter Roeck]

On 09/11/2015 06:56 AM, Lukasz Odzioba wrote:
> Removes arbitrary limit of supported CPU cores and max core ID.
> Replaces fixed size array storing per core information with dynamically allocated one.
>
> Currently coretemp is not able to handle cores with core ID greater than 32.
> Such attempt ends up with the following error in dmesg:
> coretemp coretemp.0: Adding Core XXX failed
>
> Signed-off-by: Lukasz Odzioba <lukasz.odzioba@intel.com>
> ---
>   drivers/hwmon/coretemp.c |   94 +++++++++++++++++++++++++++++++++++++---------
>   1 files changed, 76 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index 3e03379..1e60039 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -52,11 +52,9 @@ module_param_named(tjmax, force_tjmax, int, 0444);
>   MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
>
>   #define BASE_SYSFS_ATTR_NO	2	/* Sysfs Base attr no for coretemp */
> -#define NUM_REAL_CORES		32	/* Number of Real cores per cpu */
>   #define CORETEMP_NAME_LENGTH	19	/* String Length of attrs */
>   #define MAX_CORE_ATTRS		4	/* Maximum no of basic attrs */
>   #define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
> -#define MAX_CORE_DATA		(NUM_REAL_CORES + BASE_SYSFS_ATTR_NO)
>
>   #define TO_PHYS_ID(cpu)		(cpu_data(cpu).phys_proc_id)
>   #define TO_CORE_ID(cpu)		(cpu_data(cpu).cpu_core_id)
> @@ -104,7 +102,9 @@ struct temp_data {
>   struct platform_data {
>   	struct device *hwmon_dev;
>   	u16 phys_proc_id;
> -	struct temp_data *core_data[MAX_CORE_DATA];
> +	u32 core_data_size;
> +	struct temp_data **core_data;
> +	struct mutex core_data_lock;
>   	struct device_attribute name_attr;
>   };
>
> @@ -117,12 +117,25 @@ struct pdev_entry {
>   static LIST_HEAD(pdev_list);
>   static DEFINE_MUTEX(pdev_list_mutex);
>
> +static struct temp_data *get_core_data(struct platform_data *pdata, int index)
> +{
> +	struct temp_data *tdata;
> +
> +	mutex_lock(&pdata->core_data_lock);
> +	if (index >= pdata->core_data_size)
> +		tdata = NULL;

You can return NULL but never check for this condition in the calling code.
The only time you check in the calling code is when you want to know
if pdata->core_data[index] is NULL, which is distinctly different.
As such, this check does not really make sense. It would indicate
a severe driver error if it is encountered, meaning it would be better
to just let the driver crash if that happens.

> +	else
> +		tdata = pdata->core_data[index];
> +	mutex_unlock(&pdata->core_data_lock);

This lock doesn't help you anything. If create_core_data can be called _now_,
after the lock is released, tdata is no longer valid.

Really, I don't understand the point of this function. Using pdata->core_data[index]
directly without lock would be just as (un-)safe. For the lock to have any value,
you would have to keep it while accessing tdata (which is presumably what it is
supposed to protect).

At the same time, the lock is too aggressive. You don't need to protect
a read from another read. All you need to protect is a read from a write.

It might make more sense to have both get_core_data() to acquire the
(read ?) lock, and put_core_data() to release it. The function would
then not get the pointer, but just acquire the lock - the calling code
can get the pointer directly, after all.

> +	return tdata;
> +}
> +
>   static ssize_t show_label(struct device *dev,
>   				struct device_attribute *devattr, char *buf)
>   {
>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>   	struct platform_data *pdata = dev_get_drvdata(dev);
> -	struct temp_data *tdata = pdata->core_data[attr->index];
> +	struct temp_data *tdata = get_core_data(pdata, attr->index);
>
>   	if (tdata->is_pkg_data)
>   		return sprintf(buf, "Physical id %u\n", pdata->phys_proc_id);
> @@ -136,7 +149,7 @@ static ssize_t show_crit_alarm(struct device *dev,
>   	u32 eax, edx;
>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>   	struct platform_data *pdata = dev_get_drvdata(dev);
> -	struct temp_data *tdata = pdata->core_data[attr->index];
> +	struct temp_data *tdata = get_core_data(pdata, attr->index);
>
>   	rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx);
>
> @@ -148,8 +161,9 @@ static ssize_t show_tjmax(struct device *dev,
>   {
>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>   	struct platform_data *pdata = dev_get_drvdata(dev);
> +	struct temp_data *tdata = get_core_data(pdata, attr->index);
>
> -	return sprintf(buf, "%d\n", pdata->core_data[attr->index]->tjmax);
> +	return sprintf(buf, "%d\n", tdata->tjmax);
>   }
>
>   static ssize_t show_ttarget(struct device *dev,
> @@ -157,8 +171,9 @@ static ssize_t show_ttarget(struct device *dev,
>   {
>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>   	struct platform_data *pdata = dev_get_drvdata(dev);
> +	struct temp_data *tdata = get_core_data(pdata, attr->index);
>
> -	return sprintf(buf, "%d\n", pdata->core_data[attr->index]->ttarget);
> +	return sprintf(buf, "%d\n", tdata->ttarget);
>   }
>
>   static ssize_t show_temp(struct device *dev,
> @@ -167,7 +182,8 @@ static ssize_t show_temp(struct device *dev,
>   	u32 eax, edx;
>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>   	struct platform_data *pdata = dev_get_drvdata(dev);
> -	struct temp_data *tdata = pdata->core_data[attr->index];
> +	struct temp_data *tdata = get_core_data(pdata, attr->index);
> +
Unnecessary added empty line.
>
>   	mutex_lock(&tdata->update_lock);
>
> @@ -485,8 +501,23 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>   	 */
>   	attr_no = pkg_flag ? 1 : TO_ATTR_NO(cpu);
>
> -	if (attr_no > MAX_CORE_DATA - 1)
> -		return -ERANGE;
> +	if (attr_no >= pdata->core_data_size) {
> +		struct temp_data **tmp;
> +		u32 new_size = attr_no + 1;
> +
> +		get_online_cpus();
> +		mutex_lock(&pdata->core_data_lock);
> +		tmp = krealloc(pdata->core_data, new_size * sizeof(struct temp_data*), GFP_ATOMIC | __GFP_ZERO);
> +		if (tmp == NULL) {
> +			mutex_unlock(&pdata->core_data_lock);
> +			put_online_cpus();
> +			return -ENOMEM;
> +		}
> +		pdata->core_data = tmp;
> +		pdata->core_data_size = new_size;
> +		mutex_unlock(&pdata->core_data_lock);
> +		put_online_cpus();

I do not understand what the get_online_cpus() and put_online_cpus() while allocating
the memory is supposed to accomplish. Please elaborate.

Also, it might be more efficient to allocate memory in chunks, not one at a time.

> +	}
>
>   	/*
>   	 * Provide a single set of attributes for all HT siblings of a core
> @@ -495,7 +526,8 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>   	 * Skip if a HT sibling of this core is already registered.
>   	 * This is not an error.
>   	 */
> -	if (pdata->core_data[attr_no] != NULL)
> +	tdata = get_core_data(pdata, attr_no);
> +	if (tdata)
>   		return 0;
>
>   	tdata = init_temp_data(cpu, pkg_flag);
> @@ -525,7 +557,9 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>   		}
>   	}
>
> +	mutex_lock(&pdata->core_data_lock);
>   	pdata->core_data[attr_no] = tdata;
> +	mutex_unlock(&pdata->core_data_lock);
>
>   	/* Create sysfs interfaces */
>   	err = create_core_attrs(tdata, pdata->hwmon_dev, attr_no);
> @@ -534,7 +568,9 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>
>   	return 0;
>   exit_free:
> +	mutex_lock(&pdata->core_data_lock);
>   	pdata->core_data[attr_no] = NULL;
> +	mutex_unlock(&pdata->core_data_lock);
>   	kfree(tdata);
>   	return err;
>   }
> @@ -555,26 +591,35 @@ static void coretemp_add_core(unsigned int cpu, int pkg_flag)
>   static void coretemp_remove_core(struct platform_data *pdata,
>   				 int indx)
>   {
> -	struct temp_data *tdata = pdata->core_data[indx];
> +	struct temp_data *tdata = get_core_data(pdata, indx);
>
>   	/* Remove the sysfs attributes */
>   	sysfs_remove_group(&pdata->hwmon_dev->kobj, &tdata->attr_group);
>
> -	kfree(pdata->core_data[indx]);
> +	mutex_lock(&pdata->core_data_lock);
> +	kfree(tdata);
>   	pdata->core_data[indx] = NULL;
> +	mutex_unlock(&pdata->core_data_lock);
>   }
>
>   static int coretemp_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct platform_data *pdata;
> +	const u32 init_size = 4;
>
What is the purpose of this variable ? You might as well use a constant.

>   	/* Initialize the per-package data structures */
>   	pdata = devm_kzalloc(dev, sizeof(struct platform_data), GFP_KERNEL);
>   	if (!pdata)
>   		return -ENOMEM;
>
> +	pdata->core_data = kcalloc(init_size, sizeof(struct temp_data*), GFP_KERNEL);
> +	if (!pdata->core_data)
> +		return -ENOMEM;
> +
>   	pdata->phys_proc_id = pdev->id;
> +	mutex_init(&pdata->core_data_lock);
> +	pdata->core_data_size = init_size;
>   	platform_set_drvdata(pdev, pdata);
>
>   	pdata->hwmon_dev = devm_hwmon_device_register_with_groups(dev, DRVNAME,
> @@ -587,9 +632,12 @@ static int coretemp_remove(struct platform_device *pdev)
>   	struct platform_data *pdata = platform_get_drvdata(pdev);
>   	int i;
>
> -	for (i = MAX_CORE_DATA - 1; i >= 0; --i)
> +	get_online_cpus();
> +	for (i = pdata->core_data_size - 1; i >= 0; --i)
>   		if (pdata->core_data[i])
>   			coretemp_remove_core(pdata, i);
> +	put_online_cpus();
> +	kfree(pdata->core_data);
>
>   	return 0;
>   }
> @@ -667,12 +715,15 @@ static bool is_any_core_online(struct platform_data *pdata)
>   	int i;
>
>   	/* Find online cores, except pkgtemp data */
> -	for (i = MAX_CORE_DATA - 1; i >= 0; --i) {
> +	mutex_lock(&pdata->core_data_lock);
> +	for (i = pdata->core_data_size - 1; i >= 0; --i) {
>   		if (pdata->core_data[i] &&
>   			!pdata->core_data[i]->is_pkg_data) {
> +			mutex_unlock(&pdata->core_data_lock);

Here it would be betetr to keep the code flow, use a local variable
for the return value, and something like
			rv = true;
			break;
		...
	return rv;

>   			return true;
>   		}
>   	}
> +	mutex_unlock(&pdata->core_data_lock);
>   	return false;
>   }
>
> @@ -723,6 +774,7 @@ static void put_core_offline(unsigned int cpu)
>   	int i, indx;
>   	struct platform_data *pdata;
>   	struct platform_device *pdev = coretemp_get_pdev(cpu);
> +	struct temp_data *tdata;
>
>   	/* If the physical CPU device does not exist, just return */
>   	if (!pdev)
> @@ -732,12 +784,18 @@ static void put_core_offline(unsigned int cpu)
>
>   	indx = TO_ATTR_NO(cpu);
>
> -	/* The core id is too big, just return */
> -	if (indx > MAX_CORE_DATA - 1)
> +	if (indx >= pdata->core_data_size)

This condition would now indicate a severe driver error
and should be reported, for example with pr_err().

>   		return;
>
> -	if (pdata->core_data[indx] && pdata->core_data[indx]->cpu == cpu)
> +	get_online_cpus();
> +	tdata = get_core_data(pdata, indx);
> +	if (!tdata) {
> +		put_online_cpus();
> +		return;
> +	}
> +	if (tdata->cpu == cpu)
>   		coretemp_remove_core(pdata, indx);
> +	put_online_cpus();
>
>   	/*
>   	 * If a HT sibling of a core is taken offline, but another HT sibling
>

RE: [PATCH 1/1] hwmon: coretemp: use dynamically allocated array to store per core data

[Odzioba, Lukasz]

On 09/11/2015 04:28 AM, Guenter Roeck wrote:
> You can return NULL but never check for this condition in the calling code.
> The only time you check in the calling code is when you want to know
> if pdata->core_data[index] is NULL, which is distinctly different.
> As such, this check does not really make sense. It would indicate
> a severe driver error if it is encountered, meaning it would be better
> to just let the driver crash if that happens.

Good catch, I'll remove (index >= pdata->core_data_size) condition.

> This lock doesn't help you anything. If create_core_data can be called _now_,
> after the lock is released, tdata is no longer valid.

I don't get it, could you explain it?
core_data contains pointers and address and "length" of it can change after
krealloc, but the data in it should still point to the valid memory which is
not changing during reallocation, so tdata should be ok. Am I missing something?

> Really, I don't understand the point of this function. Using pdata->core_data[index]
> directly without lock would be just as (un-)safe. For the lock to have any value,
> you would have to keep it while accessing tdata (which is presumably what it is
> supposed to protect).

This lock is to protect core_data from beeing reallocated while accessing core_data[index].
>From my understanding without it we would have to assume/know that operations like
tdata *x=core_data[index] and core_data substitution in krealloc are atomic.
Once we have an address of temp_data struct krealloc cannot hurt us.

> At the same time, the lock is too aggressive. You don't need to protect
> a read from another read. All you need to protect is a read from a write.

> It might make more sense to have both get_core_data() to acquire the
> (read ?) lock, and put_core_data() to release it. The function would
> then not get the pointer, but just acquire the lock - the calling code
> can get the pointer directly, after all.

This has even wider scope than my lock, but I can implement it if you wish.

> I do not understand what the get_online_cpus() and put_online_cpus() while allocating
> the memory is supposed to accomplish. Please elaborate.

I couldn't find a way to avoid deadlock and race condition on during cpu removal
using just one lock - when sysfs is accessed while cpu is beeing removed.
As a background you could read history of Kirill A. Shutemov's patches from 2012:
http://permalink.gmane.org/gmane.linux.drivers.sensors/29589

Instead of get/put_online_cpus I could use another lock.
I am using it during memory allocation because put_core_offline could be executed
simoultaneously with create_core_data, can't it?
Maybe I am just trying to reinvent a wheel and you have a better idea how to ensure that.

> Also, it might be more efficient to allocate memory in chunks, not one at a time.

I'll change step from 1 to 16, unless you have a better idea.

> What is the purpose of this variable ? You might as well use a constant.

I wanted to keep it for readability, but after all I agree that it would be better to remove it.

Regarding the rest of the comments I'll apply them.

Thank you for your time and sorry for my delayed response.
Lukas