Re: [PATCH, v4] hwmon: coretemp: use list instead of fixed size array for temp data

["Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4510 bytes --]

On Tue, May 08, 2012 at 09:39:40AM -0700, Guenter Roeck wrote:
> On Tue, 2012-05-08 at 06:49 -0400, Kirill A. Shutemov wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > 
> > Let's rework code to allow arbitrary number of cores on a CPU, not
> > limited by hardcoded array size.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  v4:
> >    - address issues pointed by Guenter Roeck;
> >  v3:
> >    - drop redundant refcounting and checks;
> >  v2:
> >    - fix NULL pointer dereference. Thanks to R, Durgadoss;
> >    - use mutex instead of spinlock for list locking.
> > ---
> 
> Hi Kirill,
> 
> unfortunately now we have another race condition :(. See below ...

Ughh..

> > @@ -557,11 +579,22 @@ exit_free:
> >  static int __devexit coretemp_remove(struct platform_device *pdev)
> >  {
> >  	struct platform_data *pdata = platform_get_drvdata(pdev);
> > -	int i;
> > +	struct temp_data *tdata;
> >  
> > -	for (i = MAX_CORE_DATA - 1; i >= 0; --i)
> > -		if (pdata->core_data[i])
> > -			coretemp_remove_core(pdata, &pdev->dev, i);
> > +	for (;;) {
> > +		mutex_lock(&pdata->temp_data_lock);
> > +		if (!list_empty(&pdata->temp_data_list)) {
> > +			tdata = list_first_entry(&pdata->temp_data_list,
> > +					struct temp_data, list);
> > +			list_del(&tdata->list);
> > +		} else
> > +			tdata = NULL;
> > +		mutex_unlock(&pdata->temp_data_lock);
> > +
> > +		if (!tdata)
> > +			break;
> > +		coretemp_remove_core(tdata, &pdev->dev);
> > +	}
> >  
> Unfortunately, that results in a race condition, since the tdata list
> entry is gone before the attribute file is deleted.
> 
> I think you can still use list_for_each_entry_safe, only outside the
> mutex, and remove the list entry at the end of coretemp_remove_core()
> after deleting the attribute file. Just keep the code as it was, and
> remove the list entry (mutex-protected) where core_data[] was set to
> NULL.

I think

if (tdata)
	return -ENODEV;

in show methods will fix the issue. Right?

> 
> >  	device_remove_file(&pdev->dev, &pdata->name_attr);
> >  	hwmon_device_unregister(pdata->hwmon_dev);
> > @@ -641,16 +674,18 @@ static void __cpuinit coretemp_device_remove(unsigned int cpu)
> >  
> >  static bool __cpuinit is_any_core_online(struct platform_data *pdata)
> >  {
> > -	int i;
> > +	struct temp_data *tdata;
> > +	bool ret = false;
> >  
> > -	/* Find online cores, except pkgtemp data */
> > -	for (i = MAX_CORE_DATA - 1; i >= 0; --i) {
> > -		if (pdata->core_data[i] &&
> > -			!pdata->core_data[i]->is_pkg_data) {
> > -			return true;
> > +	mutex_lock(&pdata->temp_data_lock);
> > +	list_for_each_entry(tdata, &pdata->temp_data_list, list) {
> > +		if (!tdata->is_pkg_data) {
> 
> Actually, we don't need is_pkg_data anymore at all. Just test for
> "tdata->id == 1" instead. That will simplify the code a bit.

Okay, I'll update this.

> > +			ret = true;
> > +			break;
> >  		}
> >  	}
> > -	return false;
> > +	mutex_unlock(&pdata->temp_data_lock);
> > +	return ret;
> >  }
> >  
> >  static void __cpuinit get_core_online(unsigned int cpu)
> > @@ -697,9 +732,10 @@ static void __cpuinit get_core_online(unsigned int cpu)
> >  
> >  static void __cpuinit put_core_offline(unsigned int cpu)
> >  {
> > -	int i, indx;
> > +	int i, attr_no;
> >  	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)
> > @@ -707,14 +743,15 @@ static void __cpuinit put_core_offline(unsigned int cpu)
> >  
> >  	pdata = platform_get_drvdata(pdev);
> >  
> > -	indx = TO_ATTR_NO(cpu);
> > +	attr_no = TO_ATTR_NO(cpu);
> >  
> > -	/* The core id is too big, just return */
> > -	if (indx > MAX_CORE_DATA - 1)
> > -		return;
> > -
> > -	if (pdata->core_data[indx] && pdata->core_data[indx]->cpu == cpu)
> > -		coretemp_remove_core(pdata, &pdev->dev, indx);
> > +	tdata = get_temp_data(pdata, attr_no);
> > +	if (tdata && tdata->cpu == cpu) {
> > +		mutex_lock(&pdata->temp_data_lock);
> > +		list_del(&tdata->list);
> > +		mutex_unlock(&pdata->temp_data_lock);
> > +		coretemp_remove_core(tdata, &pdev->dev);
> 
> Same race condition as above here.
> 
> > +	}
> >  
> >  	/*
> >  	 * If a HT sibling of a core is taken offline, but another HT sibling
> 
> 

-- 
 Kirill A. Shutemov

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]