Date: Sat, 22 Feb 2003 00:57:01 +0100 From: Maxime Henrion <mux@freebsd.org> To: Nate Lawson <nate@root.org> Cc: src-committers@FreeBSD.org, cvs-src@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/pci if_xl.c Message-ID: <20030221235701.GS60813@elvis.mu.org> In-Reply-To: <Pine.BSF.4.21.0302211523570.54211-100000@root.org> References: <20030221222315.GR60813@elvis.mu.org> <Pine.BSF.4.21.0302211523570.54211-100000@root.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Nate Lawson wrote: > On Fri, 21 Feb 2003, Maxime Henrion wrote: > > Maxime Henrion wrote: > > These patches look mostly fine (I only looked at the busdma drivers). I > > saw two minor nits though. You're using "if (error != 0)", I personally > > prefer "if (error)" (I don't know if style(9) says something for this), > > but what's more important is that it makes style inconsistent in those > > drivers where the latter is used everywhere else. > > I did it because of this example and the warning against "!error". > error = function(a1, a2); > if (error != 0) > exit(error); > > But I don't care either way -- I try to stick to the original style when > possible (i.e. the xl changes do not remove the goto tree even though I > don't like that style). Then let's follow the existing style in those drivers. > > Also, there's an unreachable bus_teardown_intr() call in the xl(4) diff. > > It should be just removed since with your patches, the interrupt is the > > last thing allocated in the xl_init() function and thus we never need to > > deallocate it there. > > I actually did that on purpose to handle future reordering if it > happens. I think it's an advantage to have the resource cleanup be > (mostly) independent of the order it was allocated in. In fact, with > a little more work, the attach error case and the detach case could be > unified. Fair enough. It would be nice to add a comment explaining this then so that people don't wonder about it. Also, telling it to lint with a /* UNREACH */ or something might make sense. Why aren't you doing it in every driver? Cheers, Maxime To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030221235701.GS60813>