> > > > 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. Sean
signature.asc
Description: This is a digitally signed message part
