From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean Delvare Subject: Re: [PATCH 4/5] I2C: DaVinci: fix signal handling bug Date: Mon, 28 Apr 2008 22:50:11 +0200 Message-ID: <20080428225011.4d97736c@hyperion.delvare> References: <1209142694-30046-1-git-send-email-troy.kisky@boundarydevices.com> <1209142694-30046-2-git-send-email-troy.kisky@boundarydevices.com> <1209142694-30046-3-git-send-email-troy.kisky@boundarydevices.com> <1209142694-30046-4-git-send-email-troy.kisky@boundarydevices.com> <20080428191332.371e35e3@hyperion.delvare> <48161578.3080000@boundarydevices.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <48161578.3080000-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: i2c-bounces-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org Errors-To: i2c-bounces-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org To: Troy Kisky Cc: i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org, Kevin Hilman List-Id: linux-i2c@vger.kernel.org On Mon, 28 Apr 2008 11:20:40 -0700, Troy Kisky wrote: > Jean Delvare wrote: > > Hi Troy, > > > > On Fri, 25 Apr 2008 09:58:13 -0700, Troy Kisky wrote: > >> If wait_for_completion_interruptible_timeout exits due > >> to a signal, the i2c bus was locking up. > > > > What kind of signal (coming from where) are you talking about? > > With the user space i2c interface, a ^c was > locking the bus. Ah, OK. I admit I've never tested this on any i2c bus driver. > > If you don't want to be interrupted, why don't you simply use > > wait_for_completion_timeout() instead of > > wait_for_completion_interruptible_timeout()? > > I didn't make that choice. Perhaps wait_for_completion_timeout() > would be better. I just preferred fixing the lockup if a signal > happened. It seemed like a safer change to me. Can wait_for_completion_timeout() > return for any reason other the successful completion or timeout? I think not, but... > Will an explicit kill of the process return? I just don't know. I guess you'd have to try. > Do you want it changed to use wait_for_completion_timeout()? I'm suggesting this because it seems to be a much more simple way to fix the problem. If that works for you, why do something more complex? As a side note, I'm curious what happens if the call timeouts. Doesn't the hardware lockup happen? From the hardware's perspective I can't see any difference between the timeout case and the signal case. > >> +static inline void terminate_read(struct davinci_i2c_dev *dev) > >> +{ > >> + if (dev->buf_len) > >> + dev->buf_len--; > >> + if (dev->buf_len) { > > > > Please explain (in a comment) what you are doing here. Can't you just > > test for (dev->buf_len > 1)? > > Or maybe no test, just always set the nak bit and throw away the data?? You know the hardware, I don't, I just can't tell. My suggestion was only based on logic. > >> + u16 w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG); > >> + w |= DAVINCI_I2C_MDR_NACK; > >> + davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w); > >> + } -- Jean Delvare _______________________________________________ i2c mailing list i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org http://lists.lm-sensors.org/mailman/listinfo/i2c