From owner-svn-src-head@FreeBSD.ORG Tue Jul 30 16:18:42 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 28135F57; Tue, 30 Jul 2013 16:18:42 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id E9CA32D53; Tue, 30 Jul 2013 16:18:41 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 64755B97A; Tue, 30 Jul 2013 12:18:40 -0400 (EDT) From: John Baldwin To: sbruno@freebsd.org Subject: Re: svn commit: r253708 - head/sys/dev/ipmi Date: Tue, 30 Jul 2013 12:02:19 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p28; KDE/4.5.5; amd64; ; ) References: <201307271632.r6RGWYF8046749@svn.freebsd.org> <201307291617.39898.jhb@freebsd.org> <1375138259.1479.69.camel@localhost> In-Reply-To: <1375138259.1479.69.camel@localhost> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201307301202.19954.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Tue, 30 Jul 2013 12:18:40 -0400 (EDT) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Jul 2013 16:18:42 -0000 On Monday, July 29, 2013 6:50:59 pm Sean Bruno wrote: > [sbruno_comment_blocks == 4] > > > > > The identify function in 7.x has no such check: > > > > static void > > ipmi_isa_identify(driver_t *driver, device_t parent) > > { > > struct ipmi_get_info info; > > uint32_t devid; > > > > if (ipmi_smbios_identify(&info) && info.iface_type != SSIF_MODE && > > device_find_child(parent, "ipmi", -1) == NULL) { > > Ok then what is this ^^^^^^^^^ ? Doesn't this mean that if > device_find_child() returns a child node that we should abort? Is that > not the same as what I'm going on about? This makes it only add at most one child device. It is a common idiom in identify routines so that if you kldunload and re-kldload you don't end up with two "ipmi" devices added by the identify routine. > > I'd rather be sure this is the right fix, and if it isn't I'd prefer to > > revert this as I don't think it is actually fixing anything. > > > It definitely does *not* have the effect that I advertised in my commit > message. > > the commit DOES: > -- remove any attempt to do anything in ipmi_isa_* functions. > -- does not emit any errors on attach failure (which are noisy and > distracting). For these, the better fix would be to check ipmi_attached in ipmi_isa_probe(). This is what happens in all the other bus front ends. > -- make attaching to ipmi0 more "reliable" by blindly raising the > timeout value to 6 seconds. (6 seconds is the totally empirical > value I came up with in testing that never failed to attach across > 100+ reboots). This is valid, and I don't think that should be reverted. > I disagree that it should be reverted. We can argue about it if you > wish and I'm open to modifying back to the original code. I don't think > I'd agree with removing the error messages on attachment failure though. > I view the attachment failures as "sysadmin noise" but they should be > there *if* there is a real attach failure. How about just moving the ipmi_attached check into the probe routine to match all the other uses (grep for ipmi_attached in the dir to see what I mean). Also, when you MFC, don't claim it fixes NMIs from bce(4), just that it removes noise and expands the timeout. :) -- John Baldwin