svn commit: r253708 - head/sys/dev/ipmi
sean_bruno at yahoo.com
Tue Jul 30 18:57:09 UTC 2013
> > 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.
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.
> > > 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.
For clarity, I'm reverting 253708 completely.
> > -- 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 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 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. :)
Sounds good. Done. Tests look good. commited r253813.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 488 bytes
Desc: This is a digitally signed message part
More information about the svn-src-head