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

[hiren panchasara]

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;
         }
        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

> Thanks.
>
> --
> John Baldwin