From owner-svn-src-head@FreeBSD.ORG Wed Apr 17 21:26:46 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 0C6FC35F; Wed, 17 Apr 2013 21:26:46 +0000 (UTC) (envelope-from hiren.panchasara@gmail.com) Received: from mail-ee0-f43.google.com (mail-ee0-f43.google.com [74.125.83.43]) by mx1.freebsd.org (Postfix) with ESMTP id 32802889; Wed, 17 Apr 2013 21:26:44 +0000 (UTC) Received: by mail-ee0-f43.google.com with SMTP id e50so992100eek.16 for ; Wed, 17 Apr 2013 14:26:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=loM0lOWaTXndQ7PWxK+vHBavbGNqnA9ceDE8qaWzjKU=; b=ZDofhuBi0d6goWBqYHc/aR4SO8XP3/3BlE9ValX7ePK8ujoAkSnU+RM6/R0pPNpZMs YT6y2sgCZjtOr90fpMyBeKlvk0yWbPjUc8+O7nzwjrOkqY+z8oOZnKw7d1wLy4qxbFKk yEFZ7Vy9jiMSalxRWDeZNOC/YbLGvDpAytozp6GdQDQ+sDXSw80hnpHqVwSS2BxbN2xz VKu7713dL5OV4GbeX4pfo6yoceGDhRscfWOF0KIP1DiN4YV9eHNe2NBlh3Bp7v0WTM4o X6rzkLcFXns3LuY/I6iDHnn4WtHtrdGr+BEgafToo4eJBV/ijUoF8MqUimadx/iF57Ga VSkw== MIME-Version: 1.0 X-Received: by 10.14.5.137 with SMTP id 9mr22327293eel.30.1366233997155; Wed, 17 Apr 2013 14:26:37 -0700 (PDT) Sender: hiren.panchasara@gmail.com Received: by 10.15.91.72 with HTTP; Wed, 17 Apr 2013 14:26:37 -0700 (PDT) In-Reply-To: <201304171451.45400.jhb@freebsd.org> References: <201304140242.r3E2geSq094403@svn.freebsd.org> <201304171319.26560.jhb@freebsd.org> <201304171451.45400.jhb@freebsd.org> Date: Wed, 17 Apr 2013 14:26:37 -0700 X-Google-Sender-Auth: 7cmsF--Rx3H7IQK1lglWMFWsm8g Message-ID: Subject: Re: svn commit: r249461 - head/sys/dev/ips From: hiren panchasara To: John Baldwin Content-Type: text/plain; charset=UTF-8 Cc: svn-src-head , svn-src-all , src-committers X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Apr 2013 21:26:46 -0000 On Wed, Apr 17, 2013 at 11:51 AM, John Baldwin wrote: > On Wednesday, April 17, 2013 2:36:48 pm hiren panchasara wrote: >> On Wed, Apr 17, 2013 at 10:19 AM, John Baldwin wrote: >> > On Wednesday, April 17, 2013 1:15:11 pm hiren panchasara wrote: >> >> On Tue, Apr 16, 2013 at 9:10 AM, John Baldwin wrote: >> >> > On Saturday, April 13, 2013 10:42:40 pm Hiren Panchasara wrote: >> >> >> Author: hiren >> >> >> Date: Sun Apr 14 02:42:40 2013 >> >> >> New Revision: 249461 >> >> >> URL: http://svnweb.freebsd.org/changeset/base/249461 >> >> >> >> >> >> Log: >> >> >> Fixing a clang warning indicating uninitialized variable usage. >> >> >> >> >> >> PR: kern/177164 >> >> >> Approved by: sbruno (mentor) >> >> > >> >> > Hmm, I always thought that dmatags and maps were opaque types and not >> >> > necessarily "known" in consumers to be pointers. (Some drivers do > check tehm >> >> > against NULL implicitly via 'if (map)' or 'if (tag)', but well-behaved > drivers >> >> > use other means (flags, etc.) to know if they are valid.) >> >> >> >> Hi John, >> >> >> >> Would this be a better fix? We do not need to do any clean up if we >> >> fail to create the tag: >> >> >> >> Index: ips.c >> >> =================================================================== >> >> --- ips.c (revision 249588) >> >> +++ ips.c (working copy) >> >> @@ -578,7 +578,7 @@ >> >> { >> >> int error; >> >> bus_dma_tag_t dmatag; >> >> - bus_dmamap_t dmamap = NULL; >> >> + bus_dmamap_t dmamap; >> >> if (bus_dma_tag_create( /* parent */ sc- >>adapter_dmatag, >> >> /* alignemnt */ 1, >> >> /* boundary */ 0, >> >> @@ -595,7 +595,7 @@ >> >> &dmatag) != 0) { >> >> device_printf(sc->dev, "can't alloc dma tag for >> >> statue queue\n"); >> >> error = ENOMEM; >> >> - goto exit; >> >> + return error; >> >> } >> >> if(bus_dmamem_alloc(dmatag, (void *)&(sc->copper_queue), >> >> BUS_DMA_NOWAIT, &dmamap)){ >> > >> > That would be fine. I would actually prefer though that this only >> > call bus_dmamem_free() if the alloc succeeds, so in addition I would >> > make the call to bus_dmammem_free() conditional on sc->copper_queue != > NULL. >> > >> >> Makes sense, final patch looks like this: >> >> Index: ips.c >> =================================================================== >> --- ips.c (revision 249588) >> +++ ips.c (working copy) >> @@ -578,7 +578,7 @@ >> { >> int error; >> bus_dma_tag_t dmatag; >> - bus_dmamap_t dmamap = NULL; >> + bus_dmamap_t dmamap; >> if (bus_dma_tag_create( /* parent */ sc->adapter_dmatag, >> /* alignemnt */ 1, >> /* boundary */ 0, >> @@ -595,7 +595,7 @@ >> &dmatag) != 0) { >> device_printf(sc->dev, "can't alloc dma tag for >> statue queue\n"); >> error = ENOMEM; >> - goto exit; >> + return error; > > One more suggestion: just use 'return (ENOMEM)' directly perhaps. > >> } >> if(bus_dmamem_alloc(dmatag, (void *)&(sc->copper_queue), >> BUS_DMA_NOWAIT, &dmamap)){ >> @@ -623,7 +623,8 @@ >> >> return 0; >> exit: >> - bus_dmamem_free(dmatag, sc->copper_queue, dmamap); >> + if (sc->copper_queue != NULL) >> + bus_dmamem_free(dmatag, sc->copper_queue, dmamap); >> bus_dma_tag_destroy(dmatag); >> return error; >> } >> >> Thanks, >> Hiren > > Looks ok to me with or without the suggestion above. Committed in r249595. Thanks for all the help. Hiren > > -- > John Baldwin