On Wed, Apr 17, 2013 at 11:51 AM, John Baldwin <j...@freebsd.org> wrote: > On Wednesday, April 17, 2013 2:36:48 pm hiren panchasara wrote: >> On Wed, Apr 17, 2013 at 10:19 AM, John Baldwin <j...@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 <j...@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.
Committed in r249595. Thanks for all the help. Hiren > > -- > John Baldwin _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
