svn commit: r249461 - head/sys/dev/ips

[John Baldwin]

On Wednesday, April 17, 2013 2:36:48 pm hiren panchasara wrote:
> On Wed, Apr 17, 2013 at 10:19 AM, John Baldwin <jhb at 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 at 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