From owner-freebsd-hackers@FreeBSD.ORG Wed Oct 13 21:30:21 2010 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4186B106564A for ; Wed, 13 Oct 2010 21:30:21 +0000 (UTC) (envelope-from yanegomi@gmail.com) Received: from mail-iw0-f182.google.com (mail-iw0-f182.google.com [209.85.214.182]) by mx1.freebsd.org (Postfix) with ESMTP id E8A8A8FC1C for ; Wed, 13 Oct 2010 21:30:20 +0000 (UTC) Received: by iwn8 with SMTP id 8so8418646iwn.13 for ; Wed, 13 Oct 2010 14:30:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:sender:received:date :x-google-sender-auth:message-id:subject:from:to:content-type; bh=wkO7ifP9PIcdjkJHD91Ps31tQ3iQdbgcOE09ahp5XNw=; b=hu1E50Mukm07nQeErHxwxX5Yge7ixCaohQQ1dUx8GuQvLMucFFpg5abbFDVKKOzMy7 IE7aT6Acy95xHdcpA6K1o+L4gf5AgbG2hKrP1ccH47f5eFSj97K4ihsFttAa79Yvgslq ktI5O50+rYdq2FK8oomjBwnpT7BQ7PsCZ5U7g= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:date:x-google-sender-auth:message-id:subject :from:to:content-type; b=pbGj8jaZTRnVt7yIAid0SVzOI1E1ShXTcM44zxCTXALEh36DTreZOSM7dHdmB3nlco lRUne168t7ENeTqsISY7FhrHLT01pGlGYmCjOFlFtsmx6ZAlm+4fM8qy/9i66xlDMdMH UMu8Us+FQ6/UUWNgPQWHkDAsqQcw64XE+/IQ8= MIME-Version: 1.0 Received: by 10.231.190.75 with SMTP id dh11mr7511261ibb.189.1287005420235; Wed, 13 Oct 2010 14:30:20 -0700 (PDT) Sender: yanegomi@gmail.com Received: by 10.231.184.3 with HTTP; Wed, 13 Oct 2010 14:30:20 -0700 (PDT) Date: Wed, 13 Oct 2010 14:30:20 -0700 X-Google-Sender-Auth: OqgcuDoOxdnBe780JIPBdJ120Ls Message-ID: From: Garrett Cooper To: freebsd-hackers@freebsd.org Content-Type: multipart/mixed; boundary=0016367b6ecce5ae790492864c55 Subject: [PATCH] Bug with powerof2 macro in sys/param.h X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Oct 2010 21:30:21 -0000 --0016367b6ecce5ae790492864c55 Content-Type: text/plain; charset=ISO-8859-1 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 --0016367b6ecce5ae790492864c55 Content-Type: text/x-patch; charset=US-ASCII; name="fix-sys-param-powerof2-macro-when-x-equals-zero.patch" Content-Disposition: attachment; filename="fix-sys-param-powerof2-macro-when-x-equals-zero.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gf8nmnqk0 SW5kZXg6IHN5cy9zeXMvcGFyYW0uaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBzeXMvc3lzL3BhcmFtLmgJKHJl dmlzaW9uIDIxMTc2NykKKysrIHN5cy9zeXMvcGFyYW0uaAkod29ya2luZyBjb3B5KQpAQCAtMjU0 LDcgKzI1NCw3IEBACiAjZGVmaW5lCXJvdW5kZG93bih4LCB5KQkoKCh4KS8oeSkpKih5KSkKICNk ZWZpbmUJcm91bmR1cCh4LCB5KQkoKCgoeCkrKCh5KS0xKSkvKHkpKSooeSkpICAvKiB0byBhbnkg eSAqLwogI2RlZmluZQlyb3VuZHVwMih4LCB5KQkoKCh4KSsoKHkpLTEpKSYofigoeSktMSkpKSAv KiBpZiB5IGlzIHBvd2VycyBvZiB0d28gKi8KLSNkZWZpbmUgcG93ZXJvZjIoeCkJKCgoKHgpLTEp Jih4KSk9PTApCisjZGVmaW5lIHBvd2Vyb2YyKHgpCSgoeCkgIT0gMCAmJiAoKCgoeCktMSkmKHgp KT09MCkpCiAKIC8qIE1hY3JvcyBmb3IgbWluL21heC4gKi8KICNkZWZpbmUJTUlOKGEsYikgKCgo YSk8KGIpKT8oYSk6KGIpKQo= --0016367b6ecce5ae790492864c55 Content-Type: application/octet-stream; name="test_powerof2.c" Content-Disposition: attachment; filename="test_powerof2.c" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gf8npk301 I2luY2x1ZGUgPHN5cy9wYXJhbS5oPgojaW5jbHVkZSA8YXNzZXJ0Lmg+CgppbnQKbWFpbih2b2lk KQp7Cglhc3NlcnQgKCFwb3dlcm9mMigwKSk7Cglhc3NlcnQgKHBvd2Vyb2YyKDEpKTsKCWFzc2Vy dCAocG93ZXJvZjIoMikpOwoJYXNzZXJ0ICghcG93ZXJvZjIoMykpOwoJYXNzZXJ0ICghcG93ZXJv ZjIoNSkpOwoJYXNzZXJ0IChwb3dlcm9mMig4KSk7CglyZXR1cm4gKDApOwp9Cg== --0016367b6ecce5ae790492864c55 Content-Type: text/x-patch; charset=US-ASCII; name="mfiutil+mptutil-use-sys-param-powerof2.patch" Content-Disposition: attachment; filename="mfiutil+mptutil-use-sys-param-powerof2.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gf8nx1wt2 SW5kZXg6IHVzci5zYmluL21maXV0aWwvbWZpX2NvbmZpZy5jCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIHVzci5z YmluL21maXV0aWwvbWZpX2NvbmZpZy5jCShyZXZpc2lvbiAyMTE3NjcpCisrKyB1c3Iuc2Jpbi9t Zml1dGlsL21maV9jb25maWcuYwkod29ya2luZyBjb3B5KQpAQCAtMjksNiArMjksNyBAQAogICog JEZyZWVCU0QkCiAgKi8KIAorI2luY2x1ZGUgPHN5cy9wYXJhbS5oPgogI2luY2x1ZGUgPHN5cy90 eXBlcy5oPgogI2lmZGVmIERFQlVHCiAjaW5jbHVkZSA8c3lzL3N5c2N0bC5oPgpAQCAtNTIsOCAr NTMsNiBAQAogc3RhdGljIGludAlhZGRfc3BhcmUoaW50IGFjLCBjaGFyICoqYXYpOwogc3RhdGlj IGludAlyZW1vdmVfc3BhcmUoaW50IGFjLCBjaGFyICoqYXYpOwogCi0jZGVmaW5lIHBvd2Vyb2Yy KHgpICAgICgoKCh4KS0xKSYoeCkpPT0wKQotCiBzdGF0aWMgbG9uZwogZGVodW1hbml6ZShjb25z dCBjaGFyICp2YWx1ZSkKIHsKSW5kZXg6IHVzci5zYmluL21wdHV0aWwvbXB0X2NvbmZpZy5jCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIHVzci5zYmluL21wdHV0aWwvbXB0X2NvbmZpZy5jCShyZXZpc2lvbiAyMTE3 NjcpCisrKyB1c3Iuc2Jpbi9tcHR1dGlsL21wdF9jb25maWcuYwkod29ya2luZyBjb3B5KQpAQCAt NTAsOCArNTAsNiBAQAogc3RhdGljIHZvaWQJZHVtcF9jb25maWcoQ09ORklHX1BBR0VfUkFJRF9W T0xfMCAqdm9sKTsKICNlbmRpZgogCi0jZGVmaW5lIHBvd2Vyb2YyKHgpICAgICgoKCh4KS0xKSYo eCkpPT0wKQotCiBzdGF0aWMgbG9uZwogZGVodW1hbml6ZShjb25zdCBjaGFyICp2YWx1ZSkKIHsK --0016367b6ecce5ae790492864c55 Content-Type: text/x-patch; charset=US-ASCII; name="sys-md-md-fix-powerof2-invocation.patch" Content-Disposition: attachment; filename="sys-md-md-fix-powerof2-invocation.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gf8o0v5h3 SW5kZXg6IHN5cy9kZXYvbWQvbWQuYwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBzeXMvZGV2L21kL21kLmMJKHJl dmlzaW9uIDIxMTc2NykKKysrIHN5cy9kZXYvbWQvbWQuYwkod29ya2luZyBjb3B5KQpAQCAtODMy LDcgKzgzMiw3IEBACiAJZXJyb3IgPSAwOwogCWlmIChtZGlvLT5tZF9vcHRpb25zICYgfihNRF9B VVRPVU5JVCB8IE1EX0NPTVBSRVNTIHwgTURfUkVTRVJWRSkpCiAJCXJldHVybiAoRUlOVkFMKTsK LQlpZiAobWRpby0+bWRfc2VjdG9yc2l6ZSAhPSAwICYmICFwb3dlcm9mMihtZGlvLT5tZF9zZWN0 b3JzaXplKSkKKwlpZiAoIXBvd2Vyb2YyKG1kaW8tPm1kX3NlY3RvcnNpemUpKQogCQlyZXR1cm4g KEVJTlZBTCk7CiAJLyogQ29tcHJlc3Npb24gZG9lc24ndCBtYWtlIHNlbnNlIGlmIHdlIGhhdmUg cmVzZXJ2ZWQgc3BhY2UgKi8KIAlpZiAobWRpby0+bWRfb3B0aW9ucyAmIE1EX1JFU0VSVkUpCg== --0016367b6ecce5ae790492864c55 Content-Type: text/x-patch; charset=US-ASCII; name="sys-i386-pci-pci_pir-remove-unnecessary-powerof2-clause.patch" Content-Disposition: attachment; filename="sys-i386-pci-pci_pir-remove-unnecessary-powerof2-clause.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_gf8pce9p4 SW5kZXg6IHN5cy9pMzg2L3BjaS9wY2lfcGlyLmMKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gc3lzL2kzODYvcGNp L3BjaV9waXIuYwkocmV2aXNpb24gMjEzODAyKQorKysgc3lzL2kzODYvcGNpL3BjaV9waXIuYwko d29ya2luZyBjb3B5KQpAQCAtNTI2LDcgKzUyNiw3IEBACiAJICogSVJRcyIgc2V0LgogCSAqLwog CWlmICghUENJX0lOVEVSUlVQVF9WQUxJRChwY2lfbGluay0+cGxfaXJxKSkgewotCQlpZiAocGNp X2xpbmstPnBsX2lycW1hc2sgIT0gMCAmJiBwb3dlcm9mMihwY2lfbGluay0+cGxfaXJxbWFzaykp CisJCWlmIChwb3dlcm9mMihwY2lfbGluay0+cGxfaXJxbWFzaykpCiAJCQlpcnEgPSBmZnMocGNp X2xpbmstPnBsX2lycW1hc2spIC0gMTsKIAkJZWxzZQogCQkJaXJxID0gcGNpX3Bpcl9jaG9vc2Vf aXJxKHBjaV9saW5rLAo= --0016367b6ecce5ae790492864c55--