Page MenuHomeFreeBSD

Add the bus_alloc_resource_anywhere() convenience function, and use it.
ClosedPublic

Authored by jhibbits on Feb 20 2016, 9:22 PM.
Tags
None
Referenced Files
F103493050: D5370.diff
Mon, Nov 25, 5:31 PM
Unknown Object (File)
Sun, Nov 24, 7:23 AM
Unknown Object (File)
Fri, Nov 22, 11:58 PM
Unknown Object (File)
Fri, Nov 22, 11:34 AM
Unknown Object (File)
Wed, Nov 20, 10:49 PM
Unknown Object (File)
Wed, Nov 20, 4:46 AM
Unknown Object (File)
Mon, Nov 18, 1:00 AM
Unknown Object (File)
Mon, Nov 11, 6:13 PM
Subscribers

Details

Summary

Many instances of bus_alloc_resource() simply use 0 and ~0 for start and end to
denote 'anywhere' with a given count. To clean this up, introduce a
bus_alloc_resource_anywhere() convenience function.

Test Plan

tinderboxed

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2666
Build 2685: arc lint + arc unit

Event Timeline

jhibbits retitled this revision from to Add the bus_alloc_resource_anywhere() convenience function, and use it..
jhibbits updated this object.
jhibbits edited the test plan for this revision. (Show Details)
jhibbits added a reviewer: jhb.
skra added inline comments.
sys/sys/bus.h
490

While looking at both bus_alloc_resource_any() and bus_alloc_resource_anywhere(), it reminds me that I have always had hard time to get what bus_alloc_resource_any() really does. And now there will be two such functions.

The behaviour is encoded in some device parent. For example, in nexus for x86, all three parameters are checked - start, end, count - to be replaced by those ones saved in resource list. On the other hand, on simplebus for fdt, only two parameters are checked - start, end - to do same work. Thus in case of fdt, there will be no difference between calling these two functions. Further, in x86 case, I wonder how rman will work when bus_alloc_resource_anywhere() is called and no one modifies start and end parameter.

Maybe, the correct way is to call bus_alloc_resource_any() in all places and check that correct size is got if the driver cares?

sys/sys/bus.h
490

Well, rman cares about all 3 arguments. All bus drivers (should) end up eventually calling rman_reserve_resource(), which does check all the constraints.

sys/sys/bus.h
490

Reading nexus_alloc_resource() in sys/x86/x86/nexus.c, the default range will be replaced if and only if the range is default and count is 1. So, if bus_alloc_resource_anywhere() is called with count not 1, then the default range will be passed to rman_reserve_resource(). This is my concern as this is IMO not the way it should work for default range. But I can be wrong.

Contrary, simplebus_alloc_resource() in sys/dev/fdt/simplebus.c does check only that the range is default. That means that there is no difference if bus_alloc_resource_any() is called or bus_alloc_resource_anywhere().

At least, behaviour of bus_alloc_resource_any() and bus_alloc_resource_anywhere() should be consistent across various buses otherwise it's bad news for drivers.

sys/sys/bus.h
490

Hmm, I misinterpreted what start and end parameters mean. So, sorry for noise in x86 nexus case. Now I understand that ANY suffix means - take it from resource list - and ANYWHERE suffix means - take it really anywhere.

However, my concern in consistency on various buses still persists.

sys/sys/bus.h
490

The buses will have to be individually fixed, if there's a problem. This change changes nothing in that regard, and really isn't a functional change at all. It simply lifts the 0/~0 out of the individual bus_alloc_resource() calls, and replaces them with a wrapper. Purely a mechanical change.

sys/dev/aac/aac.c
1783–1784

Same as aacraid, this is just bizarre code that is probably wrong and/or never exercised.

sys/dev/aacraid/aacraid.c
1666

This code just seems so bizarre. It seems like it wants to "grow" the BAR if it's too small, but you can't do that. You can allocate more space, but the BAR can only decode it's actual size.

sys/dev/amdsbwd/amdsbwd.c
397–398

This should just use _any() since the driver just called bus_set_resource() above.

sys/dev/arcmsr/arcmsr.c
4146

These are all PCI BARs in this driver and should be using _any() instead.

sys/dev/hpt27xx/hpt27xx_os_bsd.c
123–124

These should be _any().

sys/dev/hptmv/entry.c
1307–1308

This should just be using _any().

sys/dev/hptnr/hptnr_os_bsd.c
109–110

These should just be using _any(). (Same as hptrr)

sys/dev/hptrr/hptrr_os_bsd.c
101–102

This should just be using _any(), it's just a BAR. They might be trying to only map a subset of it (which we could honor in the future via bus_map_resource(), but for now we can only map the whole thing).

sys/dev/ichsmb/ichsmb_pci.c
244–245

This is actually a magic BAR in extra config space so the size shouldn't be needed. However, this is fine for now. It would be worth testing to see if one could just use _any() here if one has the hardware.

sys/dev/oce/oce_hw.c
271–272

This seems very dubious. It's a BAR so it should be using _any(), however it seems to want to hard code a size for certain variants of the hardware. It might be worth digging up who wrote this driver to ask them what they are trying to do (and how it can possibly work).

sys/dev/rp/rp_isa.c
200

Typo ("resource(anywheredev" instead of "resource_anywhere(dev")

sys/dev/sdhci/sdhci_pci.c
333–334

This is a BAR and should use _any().

sys/dev/sound/pci/als4000.c
763–764

This is a BAR and should use _any().

sys/dev/sound/pci/cs4281.c
781

These are BARs and should be using _any() (all of the ones changed in this driver)

sys/dev/sound/pci/vibes.c
742

This is a BAR and should be using _any().

833–834

This one (and the next) are definitely wrong to use a fixed size (they are even using the wrong size as the driver just called bus_set_resource() above with a different size). They should be using _any().

sys/dev/tws/tws.c
260–261

This is just broken actually. If the BAR's size is smaller than this size we will actually allocate too large of a range, though the BAR can only decode it's actual size. If the BAR is >= this size we just allocate the BAR's size. This should just be using "any". Probably this size is the exact size of the BAR so it happens to work ok.

sys/sys/bus.h
490

Yes, the bus behavior problem is orthogonal. I think all busses should treat a count as '1' as the 'auto' wildcard as that is what resource_list_alloc() documents in its comments.

Unfortunately, I think bus_alloc_resource_any() is not the best name. I might have called it 'auto'. The idea is that when your bus "knows" the size and location of your resource, the driver should just use what the bus says it should use (e.g. PCI BARs, or ACPI resources that are enumerated via _CRS). A few drivers need to allocate a specific range (e.g. old ISA drivers) for which all 3 arguments are used. A few drivers need to allocate a relocatable range of resources where they know the size, but the range can be anywhere. That is what the 'anywhere()' variant is for. That is actually what the PCI bus wants to do on behalf of a driver when it allocates space for the BAR (because the PCI bus at that time knows the specific size it wants).

jhibbits marked 7 inline comments as done.
jhibbits edited edge metadata.

Address jhb's comments. Tinderboxed.

Thanks. I have one final request which is to split out the "_any()" changes into a separate commit. Aside from that though this looks good to me. Thanks again for being patient.

One other thought may be to commit the new _anywhere() function (and manpage, etc.) as a separate change from all the conversions. This would make it easier (I think) to MFC the new API back to older branches (since you may get conflicts in the various driver, etc. changes).

jhb edited edge metadata.
This revision is now accepted and ready to land.Feb 26 2016, 7:52 PM
This revision was automatically updated to reflect the committed changes.