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.
Details
Details
tinderboxed
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/sys/bus.h | ||
|---|---|---|
| 490 ↗ | (On Diff #13555) | 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 ↗ | (On Diff #13555) | 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 ↗ | (On Diff #13555) | 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 ↗ | (On Diff #13555) | 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 ↗ | (On Diff #13555) | 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 ↗ | (On Diff #13555) | Same as aacraid, this is just bizarre code that is probably wrong and/or never exercised. |
| sys/dev/aacraid/aacraid.c | ||
| 1666 ↗ | (On Diff #13555) | 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 | ||
| 398 ↗ | (On Diff #13555) | This should just use _any() since the driver just called bus_set_resource() above. |
| sys/dev/arcmsr/arcmsr.c | ||
| 4146 ↗ | (On Diff #13555) | These are all PCI BARs in this driver and should be using _any() instead. |
| sys/dev/hpt27xx/hpt27xx_os_bsd.c | ||
| 123 ↗ | (On Diff #13555) | These should be _any(). |
| sys/dev/hptmv/entry.c | ||
| 1307 ↗ | (On Diff #13555) | This should just be using _any(). |
| sys/dev/hptnr/hptnr_os_bsd.c | ||
| 109 ↗ | (On Diff #13555) | These should just be using _any(). (Same as hptrr) |
| sys/dev/hptrr/hptrr_os_bsd.c | ||
| 101 ↗ | (On Diff #13555) | 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 ↗ | (On Diff #13555) | 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 ↗ | (On Diff #13555) | 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 ↗ | (On Diff #13555) | Typo ("resource(anywheredev" instead of "resource_anywhere(dev") |
| sys/dev/sdhci/sdhci_pci.c | ||
| 333 ↗ | (On Diff #13555) | This is a BAR and should use _any(). |
| sys/dev/sound/pci/als4000.c | ||
| 763 ↗ | (On Diff #13555) | This is a BAR and should use _any(). |
| sys/dev/sound/pci/cs4281.c | ||
| 781 ↗ | (On Diff #13555) | These are BARs and should be using _any() (all of the ones changed in this driver) |
| sys/dev/sound/pci/vibes.c | ||
| 742 ↗ | (On Diff #13555) | This is a BAR and should be using _any(). |
| 835 ↗ | (On Diff #13555) | 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 ↗ | (On Diff #13555) | 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 ↗ | (On Diff #13555) | 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). |
Comment Actions
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).