From owner-freebsd-current@FreeBSD.ORG Wed Nov 11 17:36:18 2009 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6EBC61065694; Wed, 11 Nov 2009 17:36:18 +0000 (UTC) (envelope-from gibbs@scsiguy.com) Received: from aslan.scsiguy.com (mail.scsiguy.com [70.89.174.89]) by mx1.freebsd.org (Postfix) with ESMTP id CAAC58FC12; Wed, 11 Nov 2009 17:36:17 +0000 (UTC) Received: from [192.168.3.59] (mail4.spectralogic.com [207.225.98.253]) (authenticated bits=0) by aslan.scsiguy.com (8.14.3/8.14.3) with ESMTP id nABH476l069558 (version=TLSv1/SSLv3 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Wed, 11 Nov 2009 10:04:09 -0700 (MST) (envelope-from gibbs@scsiguy.com) Message-ID: <4AFAEE89.3020009@scsiguy.com> Date: Wed, 11 Nov 2009 10:04:09 -0700 From: "Justin T. Gibbs" User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.4pre) Gecko/20090915 Thunderbird/3.0b4 ThunderBrowse/3.2.6.5 MIME-Version: 1.0 To: Attilio Rao References: <3bbf2fe10911060643g60079e31y7679b32dac670fd8@mail.gmail.com> In-Reply-To: <3bbf2fe10911060643g60079e31y7679b32dac670fd8@mail.gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.5 Cc: gibbs@freebsd.org, freebsd-current@freebsd.org, Scott Long , Ed Maste Subject: Re: [PATCH] Adding sysctl for errors statistics to ahd(4) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Nov 2009 17:36:18 -0000 On 11/6/2009 7:43 AM, Attilio Rao wrote: > This patch introduces some mechanisms for collecting informations on > errors frequency and debugging (and relative sysctls for printing them > out) for the ahd(4) driver: > http://www.freebsd.org/~attilio/Sandvine/STABLE_8/ahd/ahd-current2.diff > > The usage of array for sysctls is a bit too paranoid but it allows for > further extendibility of the code and doesn't loose of cleaness. > This code has been contributed back by Sandvine Incorporated with some cleanups. > Please review. > > Thanks, > Attilio In general, I think the patch is fine. It violates the existing style of the driver in some places (e.g. the aic7xxx drivers wrap function arguments to the opening '(' not to a 4 space indent), which should probably be addressed so the code remains consistent. My only real complaint is that there is not some generic reporting mechanism for these types of statistics. The ahd chips are legacy. Will Sandvine or vendor X add similar but slightly different sysctls to the next driver they need to integrate into their product? Higher level monitoring software can't be written to be agnostic of the storage controller and thus survive largely untouched as the HW under the software changes. I don't expect Sandvine to address this, but the project should. In the mean time, I'm okay with the patch going in since it is obviously useful to at least one company. -- Justin