svn commit: r249461 - head/sys/dev/ips
hiren panchasara
hiren at FreeBSD.org
Wed Apr 17 21:26:46 UTC 2013
On Wed, Apr 17, 2013 at 11:51 AM, John Baldwin <jhb at 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 <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.
Committed in r249595.
Thanks for all the help.
Hiren
>
> --
> John Baldwin
More information about the svn-src-all
mailing list