From owner-cvs-all@FreeBSD.ORG Tue Sep 18 21:07:43 2007 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9052C16A4E1 for ; Tue, 18 Sep 2007 21:07:43 +0000 (UTC) (envelope-from keramida@freebsd.org) Received: from igloo.linux.gr (igloo.linux.gr [62.1.205.36]) by mx1.freebsd.org (Postfix) with ESMTP id DB67513C478 for ; Tue, 18 Sep 2007 21:07:42 +0000 (UTC) (envelope-from keramida@freebsd.org) Received: from kobe.laptop (dialup192.ach.sch.gr [81.186.70.192]) (authenticated bits=128) by igloo.linux.gr (8.14.1/8.14.1/Debian-9) with ESMTP id l8IL6fjk018395 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Wed, 19 Sep 2007 00:06:53 +0300 Received: from kobe.laptop (kobe.laptop [127.0.0.1]) by kobe.laptop (8.14.1/8.14.1) with ESMTP id l8IL6FFr001580; Wed, 19 Sep 2007 00:06:34 +0300 (EEST) (envelope-from keramida@freebsd.org) Received: (from keramida@localhost) by kobe.laptop (8.14.1/8.14.1/Submit) id l8IJnl13001871; Tue, 18 Sep 2007 22:49:47 +0300 (EEST) (envelope-from keramida@freebsd.org) Date: Tue, 18 Sep 2007 22:49:46 +0300 From: Giorgos Keramidas To: Jack F Vogel Message-ID: <20070918194946.GA1799@kobe.laptop> References: <200709102150.l8ALoeXW087953@repoman.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200709102150.l8ALoeXW087953@repoman.freebsd.org> X-Hellug-MailScanner: Found to be clean X-Hellug-MailScanner-SpamCheck: not spam, SpamAssassin (not cached, score=-4.101, required 5, autolearn=not spam, ALL_TRUSTED -1.80, AWL 0.30, BAYES_00 -2.60) X-Hellug-MailScanner-From: keramida@freebsd.org X-Spam-Status: No Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/dev/em if_em.c if_em.h X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Sep 2007 21:07:43 -0000 On 2007-09-10 21:50, Jack F Vogel wrote: > jfv 2007-09-10 21:50:40 UTC > > FreeBSD src repository > > Modified files: > sys/dev/em if_em.c if_em.h > Log: > A number of small fixes: > - duplicate #define in header, thanks to Kevin Lo for pointing out. > - incorrect BUSMASTER enable logic, thanks Patrick Oeschger > - 82543 fails due to bogus IO BAR logic > - Allow 82571 to use MSI interrupts > - Checksum Offload for UDP not working on 82575 This is probably nit-picking but the following seems a bit odd: % --- a/sys/dev/em/if_em.c Mon Sep 10 21:01:56 2007 +0000 % +++ b/sys/dev/em/if_em.c Mon Sep 10 21:50:40 2007 +0000 % @@ -2450,8 +2450,8 @@ em_identify_hardware(struct adapter *ada % % /* Make sure our PCI config space has the necessary stuff set */ % adapter->hw.bus.pci_cmd_word = pci_read_config(dev, PCIR_COMMAND, 2); % - if ((adapter->hw.bus.pci_cmd_word & PCIM_CMD_BUSMASTEREN) == 0 && % - (adapter->hw.bus.pci_cmd_word & PCIM_CMD_MEMEN)) { % + if (!((adapter->hw.bus.pci_cmd_word & PCIM_CMD_BUSMASTEREN) && % + (adapter->hw.bus.pci_cmd_word & PCIM_CMD_MEMEN))) { % device_printf(dev, "Memory Access and/or Bus Master bits " % "were not set!\n"); % adapter->hw.bus.pci_cmd_word |= It adds yet another pair of parentheses, just to use the style: if (!(condition1 && condition2)) which I sometimes find hard to read. I'm not sure if this is commonly the style used in our drivers, but isn't the following easier to parse? % /* Make sure our PCI config space has the necessary stuff set */ % adapter->hw.bus.pci_cmd_word = pci_read_config(dev, PCIR_COMMAND, 2); % - if ((adapter->hw.bus.pci_cmd_word & PCIM_CMD_BUSMASTEREN) == 0 && % - (adapter->hw.bus.pci_cmd_word & PCIM_CMD_MEMEN)) { % + if ((adapter->hw.bus.pci_cmd_word & PCIM_CMD_BUSMASTEREN) == 0 || % + (adapter->hw.bus.pci_cmd_word & PCIM_CMD_MEMEN) == 0) { % device_printf(dev, "Memory Access and/or Bus Master bits " % "were not set!\n"); % adapter->hw.bus.pci_cmd_word |= AFAICT, the logic doesn't change, but not it is more explicitly clear that any bit being unset triggers the rest of the code. We also lose an extra pair of parentheses, which makes the source code less "Lisp"y too :) Having said that, I see that the '(if|while) \(!\(' pattern appears in many other places: keramida@kobe:/bsd/src$ egrep -r -e '(if|while) \([^!]\(' sys/dev | wc -l 357 keramida@kobe:/bsd/src$ I also don't see any reference to this sort of construct in style(9), so if this is the preferred style for FreeBSD code, then I need to learn to read it without worrying too much :) - Giorgos