From patchwork Mon Apr 6 16:26:54 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Grygorii.Strashko@linaro.org" X-Patchwork-Id: 46788 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-wg0-f70.google.com (mail-wg0-f70.google.com [74.125.82.70]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id B53D0214B1 for ; Mon, 6 Apr 2015 16:27:02 +0000 (UTC) Received: by wgiv13 with SMTP id v13sf6839985wgi.3 for ; Mon, 06 Apr 2015 09:27:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:from:message-id:date:user-agent :mime-version:to:cc:subject:references:in-reply-to:content-type :content-transfer-encoding:sender:precedence:list-id :x-original-sender:x-original-authentication-results:mailing-list :list-post:list-help:list-archive:list-unsubscribe; bh=YOILlWt0FjdYtBjnIPIgv+rLddGC6YWXzvtys3uAcMg=; b=dkpkB7QFyCQ1u7oXWbVmWI7JJ1PVjygrb6zg954QqglIZ9jntRDw6abc0utJER6Dt1 cYv1f1TirY/JbU4IVUASFaQTxPdhTLIcEmMd7irDelpajraHjTn50BiXjbrmeDK/8hEP 0s/1bf/SEc4Skwx8F+Vv01R8yaD9VVC+SD2z0Ti7u52uz0FFMHCCqJLrM+ucUIlj2BMa dIlgafdDflWVASzEJCCGAOimMbdfYgSdjBCxYzJSjnLwHBoD1iU5K7iYtm1ovQ2rcOz3 oASWmZcWdxvMAKdTZMt6vRYCQIRxW3XCf7DKN0S1IOAEix5DzVifDdmGSncY+s0edFuw 1KKQ== X-Gm-Message-State: ALoCoQnKlmloX+IlWBjLJ1xOR2NvyYDePVf9dmBS7NF1q/yamWzfioWrIPl1x1I+U72CRL9qzAM/ X-Received: by 10.112.118.162 with SMTP id kn2mr3365742lbb.22.1428337621753; Mon, 06 Apr 2015 09:27:01 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.8.232 with SMTP id u8ls572838laa.24.gmail; Mon, 06 Apr 2015 09:27:01 -0700 (PDT) X-Received: by 10.152.20.71 with SMTP id l7mr14108567lae.69.1428337621601; Mon, 06 Apr 2015 09:27:01 -0700 (PDT) Received: from mail-la0-f49.google.com (mail-la0-f49.google.com. [209.85.215.49]) by mx.google.com with ESMTPS id l10si3906166lal.82.2015.04.06.09.27.01 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 06 Apr 2015 09:27:01 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.49 as permitted sender) client-ip=209.85.215.49; Received: by labbd9 with SMTP id bd9so11813125lab.2 for ; Mon, 06 Apr 2015 09:27:01 -0700 (PDT) X-Received: by 10.112.119.234 with SMTP id kx10mr10591648lbb.35.1428337621391; Mon, 06 Apr 2015 09:27:01 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.57.201 with SMTP id k9csp3448767lbq; Mon, 6 Apr 2015 09:27:00 -0700 (PDT) X-Received: by 10.68.196.228 with SMTP id ip4mr27008185pbc.92.1428337619104; Mon, 06 Apr 2015 09:26:59 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id d16si7294883pdk.183.2015.04.06.09.26.58 for ; Mon, 06 Apr 2015 09:26:59 -0700 (PDT) Received-SPF: none (google.com: linux-i2c-owner@vger.kernel.org does not designate permitted sender hosts) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752693AbbDFQ05 (ORCPT ); Mon, 6 Apr 2015 12:26:57 -0400 Received: from mail-lb0-f169.google.com ([209.85.217.169]:35198 "EHLO mail-lb0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751331AbbDFQ04 (ORCPT ); Mon, 6 Apr 2015 12:26:56 -0400 Received: by lbbuc2 with SMTP id uc2so19888769lbb.2 for ; Mon, 06 Apr 2015 09:26:55 -0700 (PDT) X-Received: by 10.112.10.197 with SMTP id k5mr14309494lbb.86.1428337614847; Mon, 06 Apr 2015 09:26:54 -0700 (PDT) Received: from [172.22.39.17] ([195.238.92.128]) by mx.google.com with ESMTPSA id ba3sm1099424lbc.35.2015.04.06.09.26.53 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 06 Apr 2015 09:26:54 -0700 (PDT) From: "Grygorii.Strashko@linaro.org" Message-ID: <5522B3CE.4030302@linaro.org> Date: Mon, 06 Apr 2015 19:26:54 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Wolfram Sang , Alexander Sverdlin CC: linux-i2c@vger.kernel.org, Sekhar Nori , Murali Karicheri , Mastalski Bartosz , Mike Looijmans , Santosh Shilimkar Subject: Re: [PATCH 1/3] i2c: davinci: Rework racy ISR References: <55003E64.2030701@nokia.com> <550191AB.8000103@linaro.org> <550299D5.5080000@nokia.com> <20150403201530.GG2016@katana> In-Reply-To: <20150403201530.GG2016@katana> Sender: linux-i2c-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-i2c@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: grygorii.strashko@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.49 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , Hi Wolfram, Alexander, On 04/03/2015 11:15 PM, Wolfram Sang wrote: > On Fri, Mar 13, 2015 at 09:03:33AM +0100, Alexander Sverdlin wrote: >> Hello! >> >> On 12/03/15 14:16, ext Grygorii.Strashko@linaro.org wrote: >>>> There is one big problem in the current design: ISR accesses the controller >>>>> registers in parallel with i2c_davinci_xfer_msg() in process context. The whole >>>>> logic is not obvious, many operations are performed in process context while >>>>> ISR is always enabled and does something asynchronous even while it's not >>>>> expected. We have faced these races on 4-cores Keystone chip. Some examples: >>>>> >>>>> - when non-existing device is accessed first comes NAK IRQ, then ARDY IRQ. After >>>>> NAK we already jump out of wait_for_completion_timeout() and depending on how >>>>> lucky we are ARDY IRQ will access MDR register in the middle of some other >>>>> operation in process context; >>>>> >>>>> - STOP condition is triggered in many places in the driver, in ISR, in >>>>> i2c_davinci_xfer_msg(), but there is no code which guarantees that STOP will >>>>> be really completed. We have seen many STOP conditions simply missing in >>>>> back-to-back transfers, when next i2c_davinci_xfer_msg() call simply overwrites >>>>> MDR register while STOP is still not generated. >>>>> >>>>> So, make the design more robust and obvious: >>>>> - leave hot path (buffers management) in ISR, remove other registers access from >>>>> ISR; >>>>> - introduce second synchronization point, to make sure that STOP condition is >>>>> really generated and it's safe to begin next transfer; >>>>> - simplify the state machine; >>>>> - enable IRQs only when they are expected, disable them in ISR when transfer is >>>>> completed/failed; >>>>> - STOP is normally set simultaneously with START condition (in case of last >>>>> message); only special case when STOP is additionally generated is received NAK >>>>> -- this case is handled separately. >>> I'm not sure about this change (- It's too significant and definitely will need more review & Tested-by. >> >> Maybe you can offer this patch the customers who suddenly cannot access the devices on the bus until reboot... >> Because it's not a "bus lockup". >> >>> We need to be careful with it, especially taking into account DAVINCI_I2C_MDR_RM mode and future >>> changes like https://lkml.org/lkml/2014/5/1/348. >>> >>> May be you can split it? >> >> I can may be split it into two patches, but I'm not sure about the result. Each of them will only solve >> 50% of the problem and then nobody will see a clear benefit applying only one. But what I can offer you is In my opinion, this one can be split as it fixes few issues at once (see below). >> to spend the same effort on rebasing the "DAVINCI_I2C_MDR_RM mode" patch you are referring to. I can rebase >> it and take it into my series if you wish. > > So, shall I take this into i2c/for-next? > As i mentioned before, this patch should get Acked/Tested-by from Davinci community at least (I understand the issue and that it should be fixed, but personally I don't like the way it's done) - The of ISR code have not been changed significantly from the very beginning of Davinci I2C driver's life :( - As I understand from commits history your patch most probably will break I2C_FUNC_SMBUS_QUICK, but I can't verify it (c6c7c72 i2c: davinci: Fix smbus Oops with AIC33 usage). So I have following propositions: 1) Get rid of obsolete code left after commit 5a0d5f5 i2c-davinci: Fix signal handling bug because commit 900ef80 'i2c: davinci: don't use interruptible completion" coverts wait_for_completion_interruptible_timeout() -> wait_for_completion_timeout() [part of this patch] 2) Add i2c_davinci_wait_bus_not_busy() at the end of i2c_davinci_xfer(), so driver will wait BF before continue (should fix STP issue) 3) Give a try below diff which could fix NACK -> ARDY issue --- 4) Clean up ICSTR in i2c_davinci_xfer_msg() [part of this patch], follows SPRUH77A - TRM 'OMAP-L138 DSP+ARM Processor' 23.2.11.1 Configuring the I2C in Master Receiver Mode and Servicing Receive Data via CPU 5) Correct IRQ configuration in i2c_davinci_xfer_msg(), now DAVINCI_I2C_IMR_RRDY and DAVINCI_I2C_IMR_XRDY will never be cleared once set. 6) [optional] Enable/disable IRQ in i2c_davinci_xfer_msg() or davinci_xfer_msg() only when driver is ready to handle it. I'll be glad to help if needed, but the main problem from my side is that I have no HW to test it (buggy scenario with NACK) and see no way to simulate it. regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index 4788a32..a053c55 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -485,9 +485,8 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop) if (dev->cmd_err & DAVINCI_I2C_STR_NACK) { if (msg->flags & I2C_M_IGNORE_NAK) return msg->len; - w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG); - w |= DAVINCI_I2C_MDR_STP; - davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w); + /* assume STP will be sent from ISR + * TODO: msg->flags & I2C_M_IGNORE_NAK ?*/ return -EREMOTEIO; } return -EIO; @@ -581,7 +580,6 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id) case DAVINCI_I2C_IVR_NACK: dev->cmd_err |= DAVINCI_I2C_STR_NACK; dev->buf_len = 0; - complete(&dev->cmd_complete); break; case DAVINCI_I2C_IVR_ARDY: @@ -594,8 +592,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id) w |= DAVINCI_I2C_MDR_STP; davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w); + } else { + complete(&dev->cmd_complete); } - complete(&dev->cmd_complete); break; case DAVINCI_I2C_IVR_RDR: