Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756375AbbGVH1j (ORCPT ); Wed, 22 Jul 2015 03:27:39 -0400 Received: from mail-wi0-f181.google.com ([209.85.212.181]:37177 "EHLO mail-wi0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756017AbbGVH1h (ORCPT ); Wed, 22 Jul 2015 03:27:37 -0400 Date: Wed, 22 Jul 2015 09:27:32 +0200 From: Pali =?utf-8?B?Um9ow6Fy?= To: Michael Welling Cc: Sebastian Reichel , Pavel Machek , Tony Lindgren , Ivaylo Dimitrov , Aaro Koskinen , Nishanth Menon , linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: linux 4.2-rc1 broken Nokia N900 Message-ID: <20150722072732.GB22138@pali> References: <201507132234.22082@pali> <20150713215232.GA22584@deathray> <201507140002.44119@pali> <20150713223607.GA23001@deathray> <20150714071412.GA12844@pali> <20150720204647.GA11931@deathstar> <20150721093441.GA23241@amd> <20150722001741.GA7950@deathstar> <20150722020306.GA8379@earth> <20150722051334.GA2165@deathstar> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150722051334.GA2165@deathstar> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4262 Lines: 123 On Wednesday 22 July 2015 00:13:34 Michael Welling wrote: > On Wed, Jul 22, 2015 at 04:03:07AM +0200, Sebastian Reichel wrote: > > Hi, > > > > On Tue, Jul 21, 2015 at 07:17:41PM -0500, Michael Welling wrote: > > > On Tue, Jul 21, 2015 at 11:34:41AM +0200, Pavel Machek wrote: > > > > > > This code has my head spinning. > > > > > > I found that the errors do not occur when the driver is built into the kernel. > > > > > > I also found that with the patch below the errors go away. > > > > > > Not sure if it is acceptible but see if it fixes things on your side. > > > > > > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > > index cf8b91b..3164d13 100644 > > > --- a/drivers/spi/spi.c > > > +++ b/drivers/spi/spi.c > > > @@ -1801,11 +1801,11 @@ int spi_setup(struct spi_device *spi) > > > if (!spi->max_speed_hz) > > > spi->max_speed_hz = spi->master->max_speed_hz; > > > > > > - spi_set_cs(spi, false); > > > - > > > if (spi->master->setup) > > > status = spi->master->setup(spi); > > > > > > + spi_set_cs(spi, false); > > > + > > > dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n", > > > (int) (spi->mode & (SPI_CPOL | SPI_CPHA)), > > > (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "", > > > > mh. maybe a runtime PM issue? > > mh? > > > > > * external abort on non-linefetch: address cannot be accessed, > > since the module's clocks are disabled > > * built-in works, module not: built-in is probably a little bit > > faster (module must not be loaded from filesystem), so that > > the device has not yet been suspended > > * Before 4.2, omap2_mcspi_set_cs() was called in the setup > > routine, which acquired runtime PM > > * In 4.2, omap2_mcspi_set_cs() seems to be called without a > > prior pm_runtime_get_sync() > > * With your workaround, the device has not yet returned to > > suspend after the runtime PM acquisition in setup() > > > > So I suggest trying the following (compile tested only) patch: > > > > It seems you are right. > > With this patch the SPI drivers no longer cause data aborts. > > I will wait for feedback from Pavel and Pali but it looks like > we have a winner. > I'm busy right now for testing something on my N900... > > -- Sebastian > > > > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c > > index 5867384..f7d9ffd 100644 > > --- a/drivers/spi/spi-omap2-mcspi.c > > +++ b/drivers/spi/spi-omap2-mcspi.c > > @@ -245,6 +245,7 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable) > > > > static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > > { > > + struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master); > > u32 l; > > > > /* The controller handles the inverted chip selects > > @@ -255,6 +256,8 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > > enable = !enable; > > > > if (spi->controller_state) { > > + pm_runtime_get_sync(mcspi->dev); > > + > > Should the return code should be checked here as with the other > instances of pm_runtime_get_sync? > Yes, you *must* check return value of pm_runtime_get_sync! Otherwise we can get another oops/kernel panic. See similar commits: f7b2b5dd6a62a47e871627c71ed01aa3482154d9 604c31039dae4653f33003d08c91ef58b70b5e63 Anyway, what do you think about adding gcc macro for pm_runtime_get_sync function which report compiler warning when return value is not checked? > > l = mcspi_cached_chconf0(spi); > > > > if (enable) > > @@ -263,6 +266,9 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) > > l |= OMAP2_MCSPI_CHCONF_FORCE; > > > > mcspi_write_chconf0(spi, l); > > + > > + pm_runtime_mark_last_busy(mcspi->dev); > > + pm_runtime_put_autosuspend(mcspi->dev); > > } > > } > > > > -- Pali Rohár pali.rohar@gmail.com -- 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/