svn commit: r253708 - head/sys/dev/ipmi

[John Baldwin]

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