From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753277AbcBPDM1 (ORCPT ); Mon, 15 Feb 2016 22:12:27 -0500 Received: from linuxhacker.ru ([217.76.32.60]:60906 "EHLO fiona.linuxhacker.ru" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1752744AbcBPDM0 convert rfc822-to-8bit (ORCPT ); Mon, 15 Feb 2016 22:12:26 -0500 Subject: Re: checkpatch falsepositives in Lustre code Mime-Version: 1.0 (Apple Message framework v1283) Content-Type: text/plain; charset=windows-1252 From: Oleg Drokin In-Reply-To: <1455591925.4046.40.camel@perches.com> Date: Mon, 15 Feb 2016 22:12:16 -0500 Cc: Andy Whitcroft , LKML , Andrew Morton Content-Transfer-Encoding: 8BIT Message-Id: <77C64895-85BC-4465-9415-3333A2E6440B@linuxhacker.ru> References: <1E5E2198-2E5C-4B6F-AAA5-C28E0A776714@linuxhacker.ru> <1455584189.4046.28.camel@perches.com> <073E66B1-D39F-4D03-BDC7-68B18172BA5D@linuxhacker.ru> <1455589627.4046.35.camel@perches.com> <1455591925.4046.40.camel@perches.com> To: Joe Perches X-Mailer: Apple Mail (2.1283) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Feb 15, 2016, at 10:05 PM, Joe Perches wrote: > On Mon, 2016-02-15 at 21:45 -0500, Oleg Drokin wrote: >> On Feb 15, 2016, at 9:27 PM, Joe Perches wrote: >> >>> On Mon, 2016-02-15 at 20:57 -0500, Oleg Drokin wrote: >>>> On Feb 15, 2016, at 7:56 PM, Joe Perches wrote: >>>>> [etc...] >>>>> >>>>> Yeah, that's a defect of some type. >>>> >>>> Also while I have your attention, here's another one: >>>> >>>> struct cfs_percpt_lock * >>>> cfs_percpt_lock_alloc(struct cfs_cpt_table *cptab) >>>> { >>>> struct cfs_percpt_lock *pcl; >>>> spinlock_t *lock; >>>> int i; >>>> … >>>> cfs_percpt_for_each(lock, i, pcl->pcl_locks) >>>> spin_lock_init(lock); >>>> >>>> The declaration of the spinlock pointer produces: >>>> CHECK: spinlock_t definition without comment >>>> >>>> Should spinlock pointers really be included in the check, it's obvious that >>>> they themselves are not really protecting anything, esp. considering it's a >>>> local function variable here. >>> >>> I don't have an opinion here. >>> >>> spinlock_t pointers are relatively rare. >> >> I guess they are. And I understand why you would want a comment for the actual >> spinlock, but pointexr - much less so. >> >> Anyway, I have some more questions: >> >> ERROR: Macros with complex values should be enclosed in parentheses >> #8720: FILE: drivers/staging/lustre/lustre/libcfs/tracefile.h:189: >> +#define cfs_tcd_for_each(tcd, i, j) \ >> + for (i = 0; cfs_trace_data[i]; i++) \ >> + for (j = 0, ((tcd) = &(*cfs_trace_data[i])[j].tcd); \ >> + j < num_possible_cpus(); \ >> + j++, (tcd) = &(*cfs_trace_data[i])[j].tcd) >> >> This is a macros with complex value alright, but the whole idea of this one >> is to not be enclosed. Any ideas about this one and similar? > > checkpatch is brainless script and a not a real parser. > Ignoring its stupid and incorrect messages is a good idea. It also asks to notify the authors ;) I guess this could be ignored too, but since Lustre lives in staging on the condition of improving its code style, I wanted to at least give it a good go and clean up as much stuff as makes sense. > fyi: There are many of these messages that exist like below. "define.*for_each" seems to be a recurring theme? > > I can't think of a reasonable way to automatically identify > and not show the defective error messages for these. Andy? > > --- > > ERROR: Macros with complex values should be enclosed in parentheses > #86: FILE: include/linux/dmar.h:86: > +#define for_each_active_drhd_unit(drhd) \ > + list_for_each_entry_rcu(drhd, &dmar_drhd_units, list) \ > + if (drhd->ignored) {} else > > ERROR: Macros with complex values should be enclosed in parentheses > #90: FILE: include/linux/dmar.h:90: > +#define for_each_active_iommu(i, drhd) \ > + list_for_each_entry_rcu(drhd, &dmar_drhd_units, list) \ > + if (i=drhd->iommu, drhd->ignored) {} else > > ERROR: Macros with complex values should be enclosed in parentheses > #94: FILE: include/linux/dmar.h:94: > +#define for_each_iommu(i, drhd) \ > + list_for_each_entry_rcu(drhd, &dmar_drhd_units, list) \ > + if (i=drhd->iommu, 0) {} else > > ERROR: Macros with complex values should be enclosed in parentheses > #110: FILE: include/linux/dmar.h:110: > +#define for_each_active_dev_scope(a, c, p, d) \ > + for_each_dev_scope((a), (c), (p), (d)) if (!(d)) { continue; } else > > total: 4 errors, 0 warnings, 285 lines checked