Page MenuHomeFreeBSD

Replace all resource occurrences of '0UL/~0UL' with '0/~0'.
ClosedPublic

Authored by jhibbits on Feb 11 2016, 2:29 PM.

Details

Summary

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'.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhibbits retitled this revision from to Replace all resource occurrences of '0UL/~0UL' with '0/~0'..Feb 11 2016, 2:29 PM
jhibbits updated this object.
jhibbits edited the test plan for this revision. (Show Details)
jhibbits added reviewers: committers, jhb.
jhibbits updated this revision to Diff 13231.

Should these values be two macros?

Should these values be two macros?

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.

robak added a subscriber: robak.Feb 11 2016, 2:54 PM

Why am I getting notifications regarding this revision? It doesnt seem anything I'd be involved with anyhow...

mat removed a reviewer: committers.Feb 11 2016, 3:04 PM
mat added a subscriber: mat.
In D5255#111921, @robak wrote:

Why am I getting notifications regarding this revision? It doesnt seem anything I'd be involved with anyhow...

Someone added the committers group to it. I removed it.

mat removed a subscriber: mat.Feb 11 2016, 3:05 PM
andrew added inline comments.Feb 11 2016, 3:11 PM
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.

robak removed a subscriber: robak.Feb 11 2016, 3:19 PM

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.

jhibbits added inline comments.Feb 11 2016, 3:24 PM
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.

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.

dvl added a subscriber: dvl.Feb 11 2016, 6:12 PM

I am also getting updates.

dvl removed a subscriber: dvl.Feb 11 2016, 6:12 PM
jilles added a subscriber: jilles.Feb 11 2016, 11:23 PM
jilles added inline comments.
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.

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....

jhibbits added inline comments.Feb 15 2016, 3:42 AM
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.

jhb edited edge metadata.Feb 16 2016, 4:57 PM

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).

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.

jhibbits edited edge metadata.Mar 3 2016, 1:29 AM
jhibbits updated this revision to Diff 14003.

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.

cem edited edge metadata.Mar 3 2016, 1:50 AM
cem accepted this revision.
This revision is now accepted and ready to land.Mar 3 2016, 1:50 AM
This revision was automatically updated to reflect the committed changes.