On Fri, May 13, 2011 at 01:46:09AM -0400, R, Durgadoss wrote:
> Hi Guenter,
>
> Thanks for the comments.
> Some clarifications..
>
> +static struct platform_device *coretemp_get_pdev(unsigned int cpu)
> > > +{
> > > + return NULL;
> >
> > Unless I am missing something, there should still be one pdev entry.
> > But because you never return it, the driver will never instantiate
> > the one supported core. You should return the one and only list entry here.
> >
> > It might be worthwhile testing the driver in a non-SMP configuration
> > to make sure that it works.
>
> Yes You are right...I have to return the one pdev entry here..
> Added that code for my next version of patch and tested in a non-smp
> Configuration. Works for me without crash.
>
> But, My machine had only one core, in non-smp config(which cannot be offlined)
> So could not test HOTPLU_CPU support with non-smp.
>
> > > +static void coretemp_remove_core(struct platform_data *pdata,
> > > + struct device *dev, int indx)
> > > +{
> > > + int i;
> > > + struct temp_data *tdata = pdata->core_data[indx];
> > > +
> > > + mutex_lock(&tdata->update_lock);
> > > +
> > I don't think the lock helps anything here. _If_ another caller tries
> > to acquire the lock while you hold it here, it will get it right after
> > the unlock below, just before you remove the data. Oops.
> > If that ever happens, the code has a severe problem somewhere.
> >
>
> I agree that the scenario you mentioned can happen.
> But to protect it, we need a global lock, that is
> alive even if we delete the temp_data.
> Should I add a lock in platform_data structure ?
>
My reasoning was that _if_ the scenario protected by the lock can happen,
ie you have a file access while a core is removed, you are in big trouble
anyway. A lock anywhere would not help you, since the access to the per-core
data you are trying to remove is already underway.
> > > + /* Remove the sysfs attributes */
> > > + for (i = 0; i < MAX_ATTRS; i++)
> > > + device_remove_file(dev, &tdata->sd_attrs[i].dev_attr);
> > > +
My understanding is that once you removed the sysfs files associated with a core,
it is safe to remove its tdata entry since sysfs access to the data associated
with the core is no longer possible. So a lock should not be needed here.
I am curious - did you experience a scenario which made you believe
that you need the lock ?
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
