Date: Wed, 17 Apr 2013 14:51:45 -0400 From: John Baldwin <jhb@freebsd.org> To: hiren panchasara <hiren@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r249461 - head/sys/dev/ips Message-ID: <201304171451.45400.jhb@freebsd.org> In-Reply-To: <CALCpEUGF9gMb=9nVJAg1EvS1UFF1QpPFrJzbPAHPAVGueDT_-Q@mail.gmail.com> References: <201304140242.r3E2geSq094403@svn.freebsd.org> <201304171319.26560.jhb@freebsd.org> <CALCpEUGF9gMb=9nVJAg1EvS1UFF1QpPFrJzbPAHPAVGueDT_-Q@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday, April 17, 2013 2:36:48 pm hiren panchasara wrote: > On Wed, Apr 17, 2013 at 10:19 AM, John Baldwin <jhb@freebsd.org> wrote: > > On Wednesday, April 17, 2013 1:15:11 pm hiren panchasara wrote: > >> On Tue, Apr 16, 2013 at 9:10 AM, John Baldwin <jhb@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201304171451.45400.jhb>