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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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:
signature.asc
Description: Digital signature
_______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
