From owner-freebsd-scsi@FreeBSD.ORG Tue Jul 23 21:07:34 2013 Return-Path: Delivered-To: scsi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 27113E5B; Tue, 23 Jul 2013 21:07:34 +0000 (UTC) (envelope-from uqs@FreeBSD.org) Received: from acme.spoerlein.net (acme.spoerlein.net [IPv6:2a01:4f8:131:23c2::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id C40CC254D; Tue, 23 Jul 2013 21:07:33 +0000 (UTC) Received: from localhost (acme.spoerlein.net [IPv6:2a01:4f8:131:23c2::1]) by acme.spoerlein.net (8.14.7/8.14.7) with ESMTP id r6NL7UNn019377 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 23 Jul 2013 23:07:30 +0200 (CEST) (envelope-from uqs@FreeBSD.org) Date: Tue, 23 Jul 2013 23:07:30 +0200 From: Ulrich =?utf-8?B?U3DDtnJsZWlu?= To: "Justin T. Gibbs" Subject: Re: Please review patch for aic7xxx_pci.c Message-ID: <20130723210730.GF9030@acme.spoerlein.net> References: <20130716094927.GA9030@acme.spoerlein.net> <20130723071129.GD9030@acme.spoerlein.net> <20130723204530.GE9030@acme.spoerlein.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20130723204530.GE9030@acme.spoerlein.net> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: scsi@freebsd.org X-BeenThere: freebsd-scsi@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SCSI subsystem List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Jul 2013 21:07:34 -0000 [adding back scsi@freebsd.org] On Tue, 2013-07-23 at 22:45:30 +0200, Ulrich Spörlein wrote: > Thanks, but maybe it's better to have the discussion here. > > You wrote: > The difference in the argument order between the declaration and definition is > likely what led to this programming error. Your patch should also correct the > argument reversal in the declaration of ahc_9005_subdevinfo_valid(). > > The argument order you've used seems the best choice since it matches what is > used in ahc_compose_id(). > > > > I'm not sure that reversing the arguments in the function definition is > the right way. This would make the patch a no-op. > > Consider the flow pre r95378 > > if (ahc_get_pci_function(pci) > 0 > && subvendor == 0x9005 > && subdevice != device > && SUBID_9005_TYPE_KNOWN(subdevice) != 0 > && SUBID_9005_MFUNCENB(subdevice) == 0) > return (NULL); > > and after r95378 > > if (ahc_get_pci_function(pci) > 0 > && ahc_9005_subdevinfo_valid(vendor, device, subvendor, subdevice) > -> which translates to > if (device == 0x9005 > && subdevice == 0x9005 > && subvendor != vendor > && SUBID_9005_TYPE_KNOWN(subvendor) != 0 > && SUBID_9005_TYPE(subvendor) > && DEVID_9005_TYPE(vendor) == DEVID_9005_TYPE_HBA > && SUBID_9005_MFUNCENB(subdevice) == 0) > return (NULL); > > But this is nonsensical. 0x9005 is the vendor ID for adaptec, > DEVID_9005_TYPE() should test the device id, etc. > > > The strangest part is that, although this function tests for "validity", > when it returns 1 or true, then ahc_find_pci_device() is more likely to > return NULL. This all seems horribly backwards. > > It's unlikely that someone will want to run -CURRENT on a machine with > that hardware in it, so maybe we should axe it? > > Cheers, > Uli > > On Tue, 2013-07-23 at 13:34:02 -0600, Justin T. Gibbs wrote: > > Done. > > > > -- > > Justin > > > > On Jul 23, 2013, at 1:11 AM, Ulrich Spörlein wrote: > > > > > [adding scsi, it would be nice if I could get another set of eyes on > > > this as I don't have the hardware to test this] > > > > > > On Tue, 2013-07-16 at 11:49:27 +0200, Ulrich Spörlein wrote: > > >> > > >> Hey Justin, Ken, > > >> > > >> Coverity found one more instance of swapped parameters in the kernel, > > >> introduced quite some time ago. It's probably hard to get the hardware > > >> to test this change these days ... > > >> > > >> Please see > > >> https://github.com/uqs/freebsd-head/commit/2f8f438a380c2a52a2e9f266cd716f56c8a4bb75 > > >> and leave comments, or reply to this mail. > > >> > > >> You can see the diff that introduced the problem here: > > >> http://git.freebsd.your.org/gitweb/?p=freebsd.git;a=blobdiff;f=sys/dev/aic7xxx/aic7xxx_pci.c;h=42dcdcc57f09d11199aabd6b224f2dad730f0733;hp=a4c0f4672cab3eb965d7acf1a580162168ca9716;hb=27ca4db2579f6e74861db16299eeb52e158fa7a7;hpb=06842004edacee7beec6cb72239ded59709506a2 > > >> > > >> or here: > > >> > > >> https://github.com/uqs/freebsd-head/commit/9696016b0572574bb656fb109ff2916127fa8cb8 > > >> > > >> Thanks! > > >> Uli > > > _______________________________________________ > > > freebsd-scsi@freebsd.org mailing list > > > http://lists.freebsd.org/mailman/listinfo/freebsd-scsi > > > To unsubscribe, send any mail to "freebsd-scsi-unsubscribe@freebsd.org" > >