cvs commit: src/sys/dev/bce if_bce.c src/sys/dev/em if_em.c if_em.h src/sys/dev/mpt mpt.h mpt_pci.c

[Scott Long]

M. Warner Losh wrote:
> In message: <200611151756.31047.jhb at freebsd.org>
>             John Baldwin <jhb at freebsd.org> writes:
> : On Wednesday 15 November 2006 17:51, M. Warner Losh wrote:
> : > In message: <455B963A.4050200 at samsco.org>
> : >             Scott Long <scottl at samsco.org> writes:
> : > : John Baldwin wrote:
> : > : > On Wednesday 15 November 2006 16:51, Ruslan Ermilov wrote:
> : > : >> On Thu, Nov 16, 2006 at 12:51:19AM +0300, Ruslan Ermilov wrote:
> : > : >>> On Wed, Nov 15, 2006 at 08:04:57PM +0000, John Baldwin wrote:
> : > : >>>> jhb         2006-11-15 20:04:57 UTC
> : > : >>>>
> : > : >>>>   FreeBSD src repository
> : > : >>>>
> : > : >>>>   Modified files:
> : > : >>>>     sys/dev/bce          if_bce.c 
> : > : >>>>     sys/dev/em           if_em.c if_em.h 
> : > : >>>>     sys/dev/mpt          mpt.h mpt_pci.c 
> : > : >>>>   Log:
> : > : >>>>   Add MSI support to em(4), bce(4), and mpt(4).  For now, we only 
> : > : > support
> : > : >>>>   devices that support a maximum of 1 message, and we use that 1 
> : message
> : > : >>>>   instead of the INTx rid 0 IRQ with the same interrupt handler, etc.
> : > : >>>>   
> : > : >>>>   Revision  Changes    Path
> : > : >>>>   1.19      +11 -3     src/sys/dev/bce/if_bce.c
> : > : >>>>   1.164     +11 -2     src/sys/dev/em/if_em.c
> : > : >>>>   1.56      +1 -0      src/sys/dev/em/if_em.h
> : > : >>>>   1.31      +1 -0      src/sys/dev/mpt/mpt.h
> : > : >>>>   1.39      +14 -1     src/sys/dev/mpt/mpt_pci.c
> : > : >>>>
> : > : >>> How will the "vmstat -i" output look like for MSI-enabled devices?
> : > : >>>
> : > : >> irqXXXX, where XXXX>=1024?
> : > : > 
> : > : > s/1024/256/
> : > : > 
> : > : 
> : > : There is a problem here, though.  Newbus prints out the IRQ number after
> : > : a successful device probe phase.  It has no knowledge of MSI at that 
> : > : point, so it just prints out the traditional IRQ value.  At some point,
> : > : this needs to be fixed.  Having the driver tell newbus about its MSI
> : > : intentions in the probe routine is unrealistic, so there is no quick
> : > : fix there.  Probably need to delay printing the device message until
> : > : later in the attach routine, once the driver has set up all of the 
> : > : resources.
> : > 
> : > I've been wanting to move the printing of the attach string from
> : > post-probe, pre-attach to post-attach for some time now.  We would
> : > then report the resources assigned to the device.  I'm not sure if I'd
> : > print all the resources assigned, or only those the driver activates.
> : > Both sides of the argument have merit, imho.  On the pro side,
> : > resources are used, and printing them will help highlight conflicts.
> : > On the con side, people think it clutters things up too much and might
> : > lead to false expectations.
> : > 
> : > When I've mentioned this desire at various developer summits, I was
> : > told basically "go for it, but only at freebsd X.0 since people have
> : > dmesg parsers" by many people (maybe even including Scott).
> : 
> : Also, the way it works now, if attach fails and the routine prints out error 
> : messages, you get to see line for the device first with the initial resources 
> : and then you see the error message from the driver.  If you move the printf 
> : down, then all you get is an error message from the driver.  At the very 
> : least this would break POLA for a lot of our users.  I'm still undecided.
> : I could add a printf when a device ends up succesfully allocating MSI or MSI-X 
> : IRQs if people desired.
> 
> I'd prefer those be under bootverbose.
> 
> I think the real answer may be to get devinfo output to be real...
> 
> Warner

Yeah, the resource info really is superfluous during boot.  It looks 
cool in the old-school unix way, but that's about it.  Maybe it's time
to change newbus to only print the driver name and driver-supplied 
description after the probe phase, like you hint at.  newbus could then
print resource info as the driver activates it, if bootverbose is set.
I'd support something like this for 7.0, even if devinfo doesn't get
fully fixed.  If MSI gets merged to RELENG_6, then we'll just have to
live with the incorrect newbus prints.

So for example, I would change this:

atapci0: <Intel ICH5 SATA150 controller> port 
0x1f0-0x1f7,0x3f6,0x170-0x177,0x376,0xfc00-0xfc0f at device 31.2 on pci0

into:

atapci0: <Intel ICH5 SATA150 controller> at device 31.2 on pci0

Calls to bus_activate_resouce/bus_alloc_resource(RF_ACTIVE) could then
print the various resources, contingent on bootverbose being set.  This
would make the ugly [GIANT_LOCKED] and [FAST] messages that get printed
right now have a chance of being more consistent.

Scott