From owner-freebsd-current@FreeBSD.ORG Thu Nov 12 15:17:16 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 60935106566B; Thu, 12 Nov 2009 15:17:16 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-fx0-f227.google.com (mail-fx0-f227.google.com [209.85.220.227]) by mx1.freebsd.org (Postfix) with ESMTP id 9BFFB8FC0C; Thu, 12 Nov 2009 15:17:15 +0000 (UTC) Received: by fxm27 with SMTP id 27so2468469fxm.3 for ; Thu, 12 Nov 2009 07:17:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:received:in-reply-to :references:date:x-google-sender-auth:message-id:subject:from:to:cc :content-type; bh=zBo+Fe2DtBbXY5H6b7Al2GedX+QfGx0YxSPvAe8X7Vs=; b=UQfuplnb7jLn9H3g3f+Fgm4lejn5EAqB+5j70FC/kkrMcWzjzIhv7DM+JN/lgtbrId m+R37kJZcKkDIZTtuaXW3aq27L2D+KJrLO4Nk4yMrQX4+nEUO5gyNl5yjgch+G0AWSL2 oI7ytCWKryvK+9t98Rqu1DrkpCTyyqW1Z1NZc= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; b=eVwr3/BIB7Ael4IH6ZoGVOMIqVW2V71pj0V30NiYtFKip3r1xPRSuZQLZ0+bIvIS7O LxXRwXNdHsnvMvugZUOoGeZ2r2auZePpHDKAiwaeyghz2ljRQA/1kRfYKqemyy+GggiR Qb5eDi1vqCUN9/qS/auaKG2FQ40+Qp60de89g= MIME-Version: 1.0 Sender: asmrookie@gmail.com Received: by 10.223.2.69 with SMTP id 5mr429941fai.88.1258039034562; Thu, 12 Nov 2009 07:17:14 -0800 (PST) In-Reply-To: <4AFB801F.5020806@scsiguy.com> References: <3bbf2fe10911060643g60079e31y7679b32dac670fd8@mail.gmail.com> <4AFAEE89.3020009@scsiguy.com> <3bbf2fe10911111746g48c8fb82r74744dffedf49758@mail.gmail.com> <4AFB801F.5020806@scsiguy.com> Date: Thu, 12 Nov 2009 16:17:14 +0100 X-Google-Sender-Auth: 3381a5387e76ae37 Message-ID: <3bbf2fe10911120717u79b2f057vceb41bc021c9ee0d@mail.gmail.com> From: Attilio Rao To: "Justin T. Gibbs" Content-Type: text/plain; charset=UTF-8 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: Thu, 12 Nov 2009 15:17:16 -0000 2009/11/12 Justin T. Gibbs : > On 11/11/2009 6:46 PM, Attilio Rao wrote: >> 2009/11/11 Justin T. Gibbs: >>> 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. >> >> Sorry, I cannot find where these existing style breakage happens, >> could you be a bit more precise? >> (I just found an un-sorted header introduction in aic79xx.h that I will fix). > > > For example this code: > > + ahd->sysctl_tree[AHD_SYSCTL_ROOT] = SYSCTL_ADD_NODE( > + &ahd->sysctl_ctx[AHD_SYSCTL_ROOT], SYSCTL_STATIC_CHILDREN(_hw), > + OID_AUTO, device_get_nameunit(ahd->dev_softc), CTLFLAG_RD, > + 0, ahd_sysctl_node_descriptions[AHD_SYSCTL_ROOT]); > + SYSCTL_ADD_PROC(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT], > + SYSCTL_CHILDREN(ahd->sysctl_tree[AHD_SYSCTL_ROOT]), OID_AUTO, > + "clear", CTLTYPE_UINT | CTLFLAG_RW, ahd, 0, ahd_clear_allcounters, > + "IU", "Clear all counters"); > > should look like this, not style(9), to match the existing style of the > driver: > > ahd->sysctl_tree[AHD_SYSCTL_ROOT] = > SYSCTL_ADD_NODE(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT], > SYSCTL_STATIC_CHILDREN(_hw), OID_AUTO, > device_get_nameunit(ahd->dev_softc), CTLFLAG_RD, > 0, ahd_sysctl_node_descriptions[AHD_SYSCTL_ROOT]); > > SYSCTL_ADD_PROC(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT], > SYSCTL_CHILDREN(ahd->sysctl_tree[AHD_SYSCTL_ROOT]), > OID_AUTO, "clear", CTLTYPE_UINT|CTLFLAG_RW, ahd, 0, > ahd_clear_allcounters, "IU", "Clear all counters"); > Could you check if this revision of the patch is adeguate?: http://www.freebsd.org/~attilio/Sandvine/STABLE_8/ahd/ahd-current3.diff Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein