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

[John Baldwin]

On Thursday 16 November 2006 00:03, M. Warner Losh wrote:
> In message: <455BA914.7010602 at samsco.org>
>             Scott Long <scottl at samsco.org> writes:
> : M. Warner Losh wrote:
> : > In message: <455BA0B6.1000109 at samsco.org>
> : >             Scott Long <scottl at samsco.org> writes:
> : > : 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
> : > 
> : > That's exactly what I'm thinking too.  Glad to see we're on the same
> : > page.

Yeah, I think I could go for this to.

> : > : 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.
> : > 
> : > I was thinking that if we printed things last, we could potentially
> : > eliminate them from normal use and just print them as part of the
> : > resources (optionally?):
> : > 
> : > atapci0: port 0x1f0-0x1f7,0x3f6,0x170-0x177,0x376,0xfc00-0xfc0f irq 23 
[FAST]

This works for me, too.  This also affects ISA devices added via hints.  Right 
now you just see the hint values which show a start port, but not the whole 
range.

> : > 
> : > or something similar...
> : > 
> : > Warner
> : 
> : Sounds good.  Beware that MSI makes it possible to have as many as 64
> : interrupts per device, though it's more common to have anywhere from 1
> : to 8.  I'd probably split up your above line into 2 lines, one for
> : memory resources and one for interrupt resources.
> 
> Is there some convention for MSI naming?  Or are the resources
> basically anonymous?

They are just IRQ values.  On x86 they will be >= 256, but that may not be 
true on other platforms.  Other busses might also support multiple IRQs per 
device (if you had HT peripherals I think they could do this).  In general 
IRQs are just cookies anyway.  On x86 they index into the interrupt_sources[] 
array.  On other platforms they can have other meanings.  It would almost be 
better if SYS_RES_IRQ was just SYS_RES_INTR.

-- 
John Baldwin