lkml.org 
[lkml]   [2012]   [May]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH, v4] hwmon: coretemp: use list instead of fixed size array for temp data
    On Wed, May 09, 2012 at 10:09:06AM +0300, Kirill A. Shutemov wrote:
    > 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()

    I haven't got how list_for_each_entry_safe() will be really safe without
    the lock.

    > > 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?

    It won't. Stupid me.

    But the check + kref seems will work...

    --
    Kirill A. Shutemov
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2012-05-09 10:02    [W:0.039 / U:0.048 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site