Message-ID: <55F2E503.6000706@roeck-us.net>
Date:	Fri, 11 Sep 2015 07:28:19 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Lukasz Odzioba <lukasz.odzioba@...el.com>, fenghua.yu@...el.com
CC:	jdelvare@...e.de, lm-sensors@...sensors.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] hwmon: coretemp: use dynamically allocated array
 to store per core data

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists