Date: Tue, 30 Jul 2013 11:57:00 -0700 From: Sean Bruno <sean_bruno@yahoo.com> To: John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, sbruno@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org Subject: Re: svn commit: r253708 - head/sys/dev/ipmi Message-ID: <1375210620.1496.30.camel@localhost> In-Reply-To: <201307301202.19954.jhb@freebsd.org> References: <201307271632.r6RGWYF8046749@svn.freebsd.org> <201307291617.39898.jhb@freebsd.org> <1375138259.1479.69.camel@localhost> <201307301202.19954.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-nLcfD2EZQaVp/sCqIGTt Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > >=20 > > 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? >=20 > 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. >=20 Ok, understood. Why, in this edge case, is ipmi attaching twice then? Shouldn't device_find_child() return non-NULL because the ACPI interface is attached and /dev/ipmi0 has been started? Or is it too early in the probe/attach device enumeration process for this check to succeed? Because of this, doesn't the BUS_ADD_CHILD later on "contaminate" our device tree with a device that doesn't exist? I'm being kind of a PITA about this, I know. I just want to understand what I'm missing here. I'm "arguing" that the attempted creation (even though it fails) of /dev/ipmi1 in any way is really a bug. =20 > > > 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. > > >=20 > > It definitely does *not* have the effect that I advertised in my commit > > message. > >=20 > > 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). >=20 > For these, the better fix would be to check ipmi_attached in ipmi_isa_pro= be(). > This is what happens in all the other bus front ends. >=20 For clarity, I'm reverting 253708 completely. =20 > > -- 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). >=20 > This is valid, and I don't think that should be reverted. >=20 I will apply this *separately* r253812 > > 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 thin= k > > 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. >=20 > 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. :) >=20 Sounds good. Done. Tests look good. commited r253813. Sean --=-nLcfD2EZQaVp/sCqIGTt Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (FreeBSD) iQEcBAABAgAGBQJR+Ax8AAoJEBkJRdwI6BaH1v0IAJRxjGZyOWB0Yo15UHYMwGJe JXgAi0ZeQNAeFEasXcViRKgAP0kPKMGs3v33BddDBF7+l/1jVrfB3+1bE5HYQuue 4lOkocCujd/GgWGVcZoxvDcQLHIBvn/LURaFS28im1LcIBnn2APbn4KSOtQ9CZha f6QBTEX4ZiW1ZGQGa6sRWwAcldNZjC7b/RjGLzk3F+c9ldkfU0b0vnPDymfbvLTu F1wE9l72Rk3clgqPLYOSnL/etXxpCoJJQWcwvSkWFK0+95IM2mpYrCN+R/+DtQ6U yt+Z3AVjGxzLhyQ+r7FCViV/UFu4RNiKyZyxlxFlgPS7CgTIfjpaSiFuUdqL7V4= =D7tZ -----END PGP SIGNATURE----- --=-nLcfD2EZQaVp/sCqIGTt--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1375210620.1496.30.camel>