Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id ; Tue, 11 Dec 2001 02:00:16 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id ; Tue, 11 Dec 2001 01:59:57 -0500 Received: from hal.astr.lu.lv ([195.13.134.67]:59784 "EHLO hal.astr.lu.lv") by vger.kernel.org with ESMTP id ; Tue, 11 Dec 2001 01:59:44 -0500 Message-Id: <200112110659.fBB6xLt24936@hal.astr.lu.lv> Content-Type: text/plain; charset=US-ASCII From: Andris Pavenis To: Doug Ledford Subject: Re: [PATCH] i810_audio fix for version 0.11 Date: Tue, 11 Dec 2001 08:59:20 +0200 X-Mailer: KMail [version 1.3.2] Cc: Nathan Bryant , linux-kernel@vger.kernel.org In-Reply-To: <200112080945.fB89jAC00998@hal.astr.lu.lv> <3C15566B.7010803@redhat.com> In-Reply-To: <3C15566B.7010803@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org On Tuesday 11 December 2001 02:42, Doug Ledford wrote: > Andris Pavenis wrote: > > Why returning non zero from __start_dac() and similar procedures when > > something real has been done there is so bad. > > Personal preference. Thought about slightly different idea but didn't test it (I'm now about 300 km away from a box where I did the tests and it will be so up to Cristmas): move both port operations used before and after call to __start_dac() and __start_adc() inside these inline procedures. In this case we would have waiting for results of __start_dac only when it really needed, so no possibility of deadlock there > > > Using such return code would > > ensure we never try to wait for results of __start_dac() if nothing is > > done by this procedure. > > That's part of the point. In this driver, I try to control when things are > done and keep track of them in a deterministic way. Using a return code to > tell us a function we called did nothing when we shouldn't have called it > in the first place if it wasn't going to do anything is backwards from the > way I prefer to handle things. Namely, find out why the function was > called when it shouldn't have been and solve the problem. Note: I don't If we moved outb((inb(port+OFF_CIV)+1)&31, port+OFF_LVI); and while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ; from inside __i810_update_lvi to __start_adc and __start_dac (inside if block) we would avoid deadlocks. If You like to have noticed that __start_dac ot __start_adc is called when they should not, then add printk with message there (or even disable sound support totally with additional error message in log, so user will complain). I think leaving out possiblility of deadlocks is too dangerous. > follow that philosophy on all functions, only on very simple ones like > this, there are a lot of complex functions where you want the function to > make those decisions. So, like I said, personal preference on how to > handle these things. > > > I think such way is also more safe against possible future > > modifications as real conditions are only in a single place. Keeping them > > in 2 places is possible source of bitrot if driver will be updated in > > future. > > It's intended to do exactly that. A lot of what makes this driver work > properly right now is the LVI handling. That was severly busted when I > first got hold of the driver. I *want* things to break if the LVI handling > is changed by someone else because that will alert me to the fact that the > LVI handling is then busted (at least, if they change it incorrectly, if > they do things right then they will catch problems like this and fix them > properly and I won't have to do anything). I would suggest to disable sound support and give reasonable error message in this case. So user will able to complain if this happens. It's more difficult to get usefull report when deadlock happens (and many users may not want to debug and provide additional info it in this case any more) Andris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/