[PATCH] Bug with powerof2 macro in sys/param.h

[Garrett Cooper]

    I was talking to someone today about this macro, and he noted that
the algorithm is incorrect -- it fails the base case with ((x) == 0 --
which makes sense because 2^(x) cannot equal 0 (mathematically
impossible, unless you consider the limit as x goes to negative
infinity as log (0) / log(2) is undefined). I tested out his claim and
he was right:

$ ./test_powerof2
Assertion failed: (!powerof2(0)), function main, file test_powerof2.c, line 7.
Abort trap: 6 (core dumped)

    Applying the following patch, everything's correct with the
testcase I've written:

$ ./test_powerof2
$

    There are a few different places in the sourcebase where this
macro is used, so I'm a bit wary of the implications of having this
patch be committed because this might break functionality somewhere
critical:

sys/dev/cas/if_cas.c:  CTASSERT(powerof2(n) && (n) >= (min) && (n) <= (max))

This is ok.

sys/dev/aic7xxx/aic79xx.c:     while (powerof2(sg_prefetch_align) == 0)

This is ok.

sys/dev/md/md.c:       if (mdio->md_sectorsize != 0 &&
!powerof2(mdio->md_sectorsize))

This is fixed (attached).

sys/dev/mpt/mpt_raid.c:
powerof2(vol_pg->VolumeSettings.HotSparePool)

This is ok.

sys/dev/mpt/mpt_raid.c:
powerof2(disk_pg->PhysDiskSettings.HotSparePool)

This is ok.

sys/dev/gem/if_gem.c:CTASSERT(powerof2(GEM_NRXDESC) && GEM_NRXDESC >=
32 && GEM_NRXDESC <= 8192);

This is ok.

sys/dev/gem/if_gem.c:CTASSERT(powerof2(GEM_NTXDESC) && GEM_NTXDESC >=
32 && GEM_NTXDESC <= 8192);

This is ok.

sys/dev/pci/pci.c:     if (!powerof2(actual))

This looks ok.

sys/dev/cxgb/cxgb_sge.c:       while (!powerof2(fl_q_size))
sys/dev/cxgb/cxgb_sge.c:       while (!powerof2(jumbo_q_size))

I don't feel comfortable enough to state whether or not these are problems.

sys/dev/hme/if_hme.c:CTASSERT(powerof2(HME_NRXDESC) && HME_NRXDESC >=
32 && HME_NRXDESC <= 256);

This is ok.

sys/x86/x86/local_apic.c:      KASSERT(powerof2(divisor), ("lapic:
invalid divisor %u", divisor));

This is ok.

sys/x86/x86/local_apic.c:      KASSERT(powerof2(count), ("bad count"));

This could be bad if msix is released and this was called, if I was
reading the code correctly.

sys/x86/x86/local_apic.c:      KASSERT(powerof2(align), ("bad align"));

This could be bad if msix is released and this was called, if I was
reading the code correctly.

sys/net/flowtable.c:           ft->ft_lock_count =
2*(powerof2(mp_maxid + 1) ? (mp_maxid + 1):

This could be bad.

sys/geom/gate/g_gate.c:        if (ggio->gctl_sectorsize > 0 &&
!powerof2(ggio->gctl_sectorsize)) {

This is ok.

sys/geom/stripe/g_stripe.c:    if (!powerof2(md->md_stripesize)) {

Not sure.

sys/geom/geom_redboot.c:       if (powerof2(cp->provider->mediasize))

Not sure.

sys/i386/i386/i686_mem.c:          powerof2((len)) &&          /* ...
and power of two */      \

This is ok.

sys/i386/i386/k6_mem.c:        if (desc->mr_len < 131072 ||
!powerof2(desc->mr_len))

This is ok.

sys/i386/pci/pci_pir.c:                if (pci_link->pl_irqmask != 0
&& powerof2(pci_link->pl_irqmask))

This is fixed (attached).

sys/i386/pci/pci_pir.c:                if (error &&
!powerof2(pci_link->pl_irqmask)) {

Not sure.

sys/netinet6/ip6_input.c:              if
(!powerof2(V_ip6_output_flowtable_size)) {

This is ok.

sys/amd64/amd64/amd64_mem.c:       powerof2((len)) &&          /* ...
and power of two */      \

This is ok.

usr.sbin/mptutil/mpt_config.c:#define powerof2(x)    ((((x)-1)&(x))==0)

This is fixed (attached).

usr.sbin/mptutil/mpt_config.c:                 if ((stripe_size < 512)
|| (!powerof2(stripe_size))) {

This is fixed (attached).

usr.sbin/mfiutil/mfi_config.c:#define powerof2(x)    ((((x)-1)&(x))==0)

This is fixed (attached).

    Could someone please review these patches and help me commit them?
I'm testing out the patches I can (the amd64 and msi and net ones).
Thanks,
-Garrett
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-sys-param-powerof2-macro-when-x-equals-zero.patch
Type: text/x-patch
Size: 529 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20101013/431d5e42/fix-sys-param-powerof2-macro-when-x-equals-zero.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test_powerof2.c
Type: application/octet-stream
Size: 216 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20101013/431d5e42/test_powerof2.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mfiutil+mptutil-use-sys-param-powerof2.patch
Type: text/x-patch
Size: 911 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20101013/431d5e42/mfiutilmptutil-use-sys-param-powerof2.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sys-md-md-fix-powerof2-invocation.patch
Type: text/x-patch
Size: 510 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20101013/431d5e42/sys-md-md-fix-powerof2-invocation.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sys-i386-pci-pci_pir-remove-unnecessary-powerof2-clause.patch
Type: text/x-patch
Size: 469 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20101013/431d5e42/sys-i386-pci-pci_pir-remove-unnecessary-powerof2-clause.bin