Received: by 2002:ac0:aa62:0:0:0:0:0 with SMTP id w31-v6csp1217347ima; Sun, 21 Oct 2018 07:13:06 -0700 (PDT) X-Google-Smtp-Source: ACcGV61hXd56BiqjNfbnBOqMDLn4nPlrZvd9OB0QPCBT2x2S3zMWPnJrjQYAds1x0eD0S1+XoQA1 X-Received: by 2002:a17:902:24c:: with SMTP id 70-v6mr31575530plc.324.1540131186340; Sun, 21 Oct 2018 07:13:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1540131186; cv=none; d=google.com; s=arc-20160816; b=hZEK4HYyMSAGS4/u7/RzVqGBbY4zMwUx8TSa64ehfV8BsB0G3SC5BdqBjRdUX+lxyj YAMgGBaxMg3bLCIylCYKzVt+WhgdMT4gg6ftiW0l4DFNJEqwAygDkztJOddmeFpUdlN5 SPYggaUPubksW8CzcpJNoaupyfM4SV71TWsHXlI/E/ET6KFAeoG61SgRSHxXU+2sjlVA qoIkmVMen9wiK2BgjpfONsKaVJ0c5OMEu2LeYAJlcVd+VaWF5C9EOGgVl8ICINPrAwik gpttsiM44qgM8Ua19pp+akcYkqHBf2JFK9J2h2z5nQirXI8sSxqtyUqXr1ps+e6XOBOg PUvQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version; bh=CdhV1/U4LJvWkVTErpGCM4NWA8Xt9FYe63YOaCMUVAo=; b=k0y8VBskdy4FdVvUYxJXGMSxy5Br1RDBf1P7ILOgwExW321ZTvXJ/mn/hFMzjCacrL KPONJ2ckxJepSIN8Hh6B6PPlTfBoFDCv9FQvFZEP7NvvZW4V5CuLVxJqMwTNtPC0jtbE nzH+M1qhXo/JJl3/bRb+qrWDY0UcPnamKA6FiE7fkN+EfY0yv1ob1W0CMkNVb/Vybsrs iApC9F0Obw+MjKRuGJR+1QwHc6rT64H+mUXzIA9gTAuJwQs4wZwx+fPjxnPLWv/5nF0o aNaU4MMgZWZw6HlAr0ZUGkEAlOavHootOnKUaeuN45Kfg0y0c2SQsPj5sGxvDUgy+l51 J9kg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 20-v6si31299945pgk.190.2018.10.21.07.12.51; Sun, 21 Oct 2018 07:13:06 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727787AbeJUWZN (ORCPT + 99 others); Sun, 21 Oct 2018 18:25:13 -0400 Received: from mail-lf1-f66.google.com ([209.85.167.66]:40908 "EHLO mail-lf1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727266AbeJUWZN (ORCPT ); Sun, 21 Oct 2018 18:25:13 -0400 Received: by mail-lf1-f66.google.com with SMTP id n3-v6so1532778lfe.7; Sun, 21 Oct 2018 07:10:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=CdhV1/U4LJvWkVTErpGCM4NWA8Xt9FYe63YOaCMUVAo=; b=QeK8PbDbuU6wEAeaR7Qn4U7CkxUQOkJYMWh+wTqkRaEeTwbief5KpysxVuHkEGHQ2/ zeGd3tG6qdgealNynv0EDm/EgpUAAZbr/Pn/CSa6fzhJWcGZg2ZiYfS5++PgngexWnbt cl+5vOtjrNtc1tyjGePfBgAzmck5PX7JsYlKixkC9+IThMsqcucoURpCRTTJPi5B7DKy RcvSYqEo4Q4iffGh23ifVGvJQzhI8xCRTAOOvJ1aQ2XAFJ4zk2GQmLewMXCNVFl1pQjd uxrVs6hpgbWtPKfqgoFKsJ9zhUqfdjhgwvBCWmqlJpJm9rOvN/o3C6XWY7Dw7k3U9bBq r8Sw== X-Gm-Message-State: ABuFfojrHEDr05yauQAXkib2oj8hnruenLUwkP9m3LuXUKLtHSGE9ls4 u/NodjFmQK6pEam85C4V3hZmS3ajbrvLi1M7/dqrINqH X-Received: by 2002:a19:8fce:: with SMTP id s75mr6963679lfk.151.1540131042326; Sun, 21 Oct 2018 07:10:42 -0700 (PDT) MIME-Version: 1.0 References: <20180625161303.7991-1-federico.vaga@cern.ch> <20180625161303.7991-2-federico.vaga@cern.ch> In-Reply-To: <20180625161303.7991-2-federico.vaga@cern.ch> From: Peter Korsgaard Date: Sun, 21 Oct 2018 16:10:30 +0200 Message-ID: Subject: Re: [PATCH 1/3] i2c:ocores: stop transfer on timeout To: Federico Vaga Cc: linux-i2c , linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 25, 2018 at 6:14 PM Federico Vaga wrote: Hi, and sorry for the slow response. > Detecting a timeout is ok, but we also need to assert a STOP command on > the bus in order to prevent it from generating interrupts when there are > no on going transfers. > > Example: very long transmission. > > 1. ocores_xfer: START a transfer > 2. ocores_isr : handle byte by byte the transfer > 3. ocores_xfer: goes in timeout [[bugfix here]] > 4. ocores_xfer: return to I2C subsystem and to the I2C driver > 5. I2C driver : it may clean up the i2c_msg memory > 6. ocores_isr : receives another interrupt (pending bytes to be > transferred) but the i2c_msg memory is invalid now > > So, since the transfer was too long, we have to detect the timeout and > STOP the transfer. > > Another point is that we have a critical region here. When handling the > timeout condition we may have a running IRQ handler. For this reason I > introduce a spinlock. In the IRQ handler we can just use trylock because > if the lock is taken is because we are in timeout, so there is no need to > process the IRQ. > > Signed-off-by: Federico Vaga > --- > drivers/i2c/busses/i2c-ocores.c | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c > index 88444ef74943..98c0ef74882b 100644 > --- a/drivers/i2c/busses/i2c-ocores.c > +++ b/drivers/i2c/busses/i2c-ocores.c > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include > > struct ocores_i2c { > void __iomem *base; > @@ -36,6 +37,7 @@ struct ocores_i2c { > int pos; > int nmsgs; > int state; /* see STATE_ */ > + spinlock_t xfer_lock; > struct clk *clk; > int ip_clock_khz; > int bus_clock_khz; > @@ -207,15 +209,30 @@ static void ocores_process(struct ocores_i2c *i2c) > static irqreturn_t ocores_isr(int irq, void *dev_id) > { > struct ocores_i2c *i2c = dev_id; > + unsigned long flags; > + int ret; > + > + /* > + * We need to protect i2c against a timeout event (see ocores_xfer()) > + * If we cannot take this lock, it means that we are already in > + * timeout, so it's pointless to handle this interrupt because we > + * are going to abort the current transfer. > + */ > + ret = spin_trylock_irqsave(&i2c->xfer_lock, flags); This is very old code, so I might be missing something - But I still don't quite understand this trylock logic. If we end up here with the lock taken, then that must mean that we are on SMP system. We still need to ack the interrupt, so just spinning until the other CPU releases the lock seems more sensible? -- Bye, Peter Korsgaard