From owner-svn-src-all@FreeBSD.ORG Wed Apr 17 19:02:21 2013 Return-Path: Delivered-To: svn-src-all@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 E57E1B1B; Wed, 17 Apr 2013 19:02:21 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) by mx1.freebsd.org (Postfix) with ESMTP id BA9B2F57; Wed, 17 Apr 2013 19:02:21 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 3AAE3B918; Wed, 17 Apr 2013 15:02:21 -0400 (EDT) From: John Baldwin To: hiren panchasara Subject: Re: svn commit: r249461 - head/sys/dev/ips Date: Wed, 17 Apr 2013 14:51:45 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p25; KDE/4.5.5; amd64; ; ) References: <201304140242.r3E2geSq094403@svn.freebsd.org> <201304171319.26560.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201304171451.45400.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Wed, 17 Apr 2013 15:02:21 -0400 (EDT) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Apr 2013 19:02:22 -0000 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. -- John Baldwin