The idea behind this is '~0ul' is well-defined, and casting to uintmax_t, on a
32-bit platform, will leave the upper 32 bits as 0. The maximum range of a
resource is 0xFFF.... (all bits of the full type set). By dropping the 'ul'
suffix, C type promotion rules apply, and the sign extension of ~0 on 32 bit
platforms gets it to a type-independent 'unsigned max'.
Details
Details
- Reviewers
cem jhb - Commits
- rS296336: Replace all resource occurrences of '0UL/~0UL' with '0/~0'.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I considered this, but, after discussing with jhb (and others), 0/~0 are sufficient, and standard C casting rules apply. The only real reason to use macros in this case would be if you would want a non-~0 upper limit, but this is only about setting defaults. I have another diff in review (D5134) to set physmem rman bounds appropriately, if that's your concern.
Comment Actions
Why am I getting notifications regarding this revision? It doesnt seem anything I'd be involved with anyhow...
| sys/x86/x86/nexus.c | ||
|---|---|---|
| 749 ↗ | (On Diff #13231) | This looks wrong given the comment, although I would imagine it could be removed when rman_res_t becomes uintmax_t. |
Comment Actions
I had added the committers group to get a wider audience, under the assumption that if you don't care you would just ignore it.
| sys/x86/x86/nexus.c | ||
|---|---|---|
| 749 ↗ | (On Diff #13231) | You're right, for the ulong rman_res_t case, it is incorrect because vm_paddr_t is uint64_t size for PAE, not u_long. I'll revert it for now, and it will go away completely when rman_res_t becomes uintmax_t. |
Comment Actions
Sorry guys, last time I looked at committers there were only about 50 people included. I'll refrain from adding in the future, since it's exploded since then.
| sys/arm/at91/at91.c | ||
|---|---|---|
| 167 ↗ | (On Diff #13231) | Do comparisons like these actually still compile on 32-bit systems? I know that comparing an unsigned long to an int converts the int to unsigned long, but this kind of comparison likely causes a -Wsign-compare warning since it clearly violates mathematical equality. |
Comment Actions
I agree that this gives correct results as is, but (as the original perpetrator) I'd rather see an explicit unsigned type called out here -- e.g., ~(some_type_t)0 -- rather than depending on two's-complement sign extension behavior, if you're not going to use a macro. Using bitwise operators on signed values makes me uncomfortable, and I wouldn't want to rely on WG14's good graces....
| sys/arm/at91/at91.c | ||
|---|---|---|
| 167 ↗ | (On Diff #13231) | It passes a tinderbox build. Maybe if it was an inequality (<, >) it would throw an error, but because it's an equality check, it can just use type conversion rules and be happy. |
Comment Actions
In general I'm a bit torn on having very verbose macros (RMAN_MIN/MAX, etc.). 0 and ~0 are concise. However, we live with macros like that for bus_dma tags.
One thing that might make using the macros more palatable (and I will help) is to use these magic values less often. I know Marcel has suggested having some sort of IS_DEFAULT_RANGE(start, end) macro. (It's quite interesting to see which drivers include 'count' in their 'is_default' check and which do not btw :( ). That macro would help isolate the need for the verbose macros in some cases. Another one that would help quite a bit is another alloc_resource wrapper that is like _any() but takes the size(). bus_alloc_resource_anywhere() or some such (I can't think of a better name but am open to suggestions). Having those two things would wipe out most of the bare 0 and ~0. At that point it would probably be tolerable to use the more verbose RMAN macros I had previously not been a fan of (and it might even improve readability).
| sys/dev/ata/chipsets/ata-promise.c | ||
|---|---|---|
| 194 ↗ | (On Diff #13231) | This should have been part of the previous patch to introduce rman_res_t? Perhaps commit this as a separate change for now apart from the 0/~0 diff? |
| sys/dev/cy/cy_isa.c | ||
| 83 ↗ | (On Diff #13231) | A size of zero here seems really odd. I wonder if this means to be bus_alloc_resource_any(). The nearby error message and variables (iobase) that imply this is an I/O port allocation don't inspire confidence either. *sigh* |
| sys/dev/cy/cy_pci.c | ||
| 118 ↗ | (On Diff #13231) | Ok, the '0' error is repeated here as well. It seems that '0' works but all of these calls in cy(4) should be using bus_alloc_resource_any() instead. I would break that out as a separate change from this. |
| sys/dev/hpt27xx/hpt27xx_osm_bsd.c | ||
| 1264 ↗ | (On Diff #13231) | This should be bus_alloc_resource_any(). |
| sys/dev/hptiop/hptiop.c | ||
| 2056 ↗ | (On Diff #13231) | This should be bus_alloc_resource_any(). |
| sys/dev/hptmv/entry.c | ||
| 1993 ↗ | (On Diff #13231) | This should be bus_alloc_resource_any(). |
| sys/dev/hptnr/hptnr_osm_bsd.c | ||
| 1449 ↗ | (On Diff #13231) | This should be bus_alloc_resource_any(). |
| sys/dev/hptrr/hptrr_osm_bsd.c | ||
| 1097 ↗ | (On Diff #13231) | bus_alloc_resource_any(). |
| sys/dev/pci/pci_pci.c | ||
| 391 ↗ | (On Diff #13231) | The cast change I would pull out with the ata change. (Maybe a single "convert a few more long -> rman_res_t" commit) |
| 1392 ↗ | (On Diff #13231) | Similarly here (part of the long -> rman_res_t). |
Comment Actions
I'll do another pass for the RMAN_IS_DEFAULT_RANGE(), which will eliminate a bunch of these.
| sys/dev/ata/chipsets/ata-promise.c | ||
|---|---|---|
| 194 ↗ | (On Diff #13231) | I was literally typing the commit message for this specific change when I got your notification. |
| sys/dev/hpt27xx/hpt27xx_osm_bsd.c | ||
| 1264 ↗ | (On Diff #13231) | I'll make a pass on these bus_alloc_resource->bus_alloc_resource_any later today. |
| sys/dev/pci/pci_pci.c | ||
| 391 ↗ | (On Diff #13231) | Realistically, I think this should be going away with the migration to uintmax_t, since this condition will always be false. |
Comment Actions
Reduce the diff after all the recent changes.
Recent changes reducing the diff:
- bus_alloc_resource_anywhere()
- Rewrite of some to bus_alloc_resource_any()
- RMAN_IS_DEFAULT_RANGE()
All of these reduce the diff to only the handful of files in this diff.