Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755781Ab2EIKei (ORCPT ); Wed, 9 May 2012 06:34:38 -0400 Received: from imr3.ericy.com ([198.24.6.13]:47749 "EHLO imr3.ericy.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753702Ab2EIKeg (ORCPT ); Wed, 9 May 2012 06:34:36 -0400 Date: Wed, 9 May 2012 03:32:12 -0700 From: Guenter Roeck To: "Kirill A. Shutemov" CC: Fenghua Yu , Durgadoss R , Andi Kleen , Jean Delvare , "lm-sensors@lm-sensors.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH, v4] hwmon: coretemp: use list instead of fixed size array for temp data Message-ID: <20120509103212.GA16699@ericsson.com> References: <1336474175-5714-1-git-send-email-kirill.shutemov@linux.intel.com> <1336495180.22553.68.camel@groeck-laptop> <20120509070906.GA21556@otc-wbsnb-06> <20120509072339.GB21556@otc-wbsnb-06> <20120509095617.GB15630@ericsson.com> <20120509101634.GA24566@otc-wbsnb-06> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20120509101634.GA24566@otc-wbsnb-06> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4346 Lines: 108 On Wed, May 09, 2012 at 06:16:34AM -0400, Kirill A. Shutemov wrote: > On Wed, May 09, 2012 at 02:56:17AM -0700, Guenter Roeck wrote: > > On Wed, May 09, 2012 at 03:23:39AM -0400, Kirill A. Shutemov wrote: > > > 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" > > > > > > > > > > > > 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 > > > > > > --- > > > > > > 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. > > > > > We know that it by itself won't be called multiple times. So the only question is > > if the functions to add/remove a core can be called while coretemp_remove is called, > > or if that is mutually exclusive (not that the current code handles this case). > > > > Fortunately, there is a function to block CPU removal/insertion: get_online_cpus() > > and put_online_cpus(). I have no idea if it is necessary to protect coretemp_remove() > > with it, but it might be on the safe side anyway. > > > > > > > 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... > > > > > Yes, but would be way too complicated. > > More code, yes, but complicated? What you propose looks like a trick. It > has too many assumptions on context. > There is an even better solution: unregistering the hotplug notifier before removing the driver. And, as you will notice, that is already done. So list_for_each_entry_safe() is safe after all, since no other remove/add activity will occur at the same time. > I personally prefer kref since it's straight forward and more friendly for > future changes. > Guess we have to agree to disagree on that one. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/