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

[Sean Bruno]

> > 
> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20130730/6047d5d9/attachment.sig>