Date: Tue, 30 Jul 2013 12:02:19 -0400 From: John Baldwin <jhb@freebsd.org> To: sbruno@freebsd.org Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r253708 - head/sys/dev/ipmi Message-ID: <201307301202.19954.jhb@freebsd.org> In-Reply-To: <1375138259.1479.69.camel@localhost> References: <201307271632.r6RGWYF8046749@svn.freebsd.org> <201307291617.39898.jhb@freebsd.org> <1375138259.1479.69.camel@localhost>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201307301202.19954.jhb>