Message-ID: <1491488527.10415.20.camel@codethink.co.uk>
Subject: Re: [PATCH 4.4 42/76] mmc: sdhci: Do not disable interrupts while
 waiting for clock
From: Ben Hutchings <ben.hutchings@codethink.co.uk>
To: Ludovic Desroches <ludovic.desroches@microchip.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org, Adrian Hunter <adrian.hunter@intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: Thu, 06 Apr 2017 15:22:07 +0100
In-Reply-To: <20170406121209.qrtfkdmbsstuvbel@rfolt0960.corp.atmel.com>
References: <20170328122559.966310440@linuxfoundation.org>
         <20170328122601.653745040@linuxfoundation.org>
         <1491324650.10415.3.camel@codethink.co.uk>
         <20170406121209.qrtfkdmbsstuvbel@rfolt0960.corp.atmel.com>
Organization: Codethink Ltd.
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1562
Lines: 39

On Thu, 2017-04-06 at 14:12 +0200, Ludovic Desroches wrote:
> On Tue, Apr 04, 2017 at 05:50:50PM +0100, Ben Hutchings wrote:
> > On Tue, 2017-03-28 at 14:30 +0200, Greg Kroah-Hartman wrote:
> > > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > > 
> > > ------------------
> > > 
> > > From: Adrian Hunter <adrian.hunter@intel.com>
> > > 
> > > commit e2ebfb2142acefecc2496e71360f50d25726040b upstream.
> > > 
> > > Disabling interrupts for even a millisecond can cause problems for some
> > > devices. That can happen when sdhci changes clock frequency because it
> > > waits for the clock to become stable under a spin lock.
> > > 
> > > The spin lock is not necessary here. Anything that is racing with changes
> > > to the I/O state is already broken. The mmc core already provides
> > > synchronization via "claiming" the host.
> > [...]
> > 
> > In mainline, drivers/mmc/host/sdhci-of-at91.c has a slightly different
> > version of this code that seems to have the same issue.  In 4.4 there's
> > another (conditional) mdelay(1) further up this function that seems to
> > be related to that hardware, and probably ought to have an unlock/lock
> > around it.
> 
> Right, how do you want to proceed? Do you want me to send a patch on top
> of it to manage this extra mdelay?

This change doesn't appear to break anything; I'm just saying that it's
an incomplete fix.  The other case where there's a delay with IRQs
disabled should be fixed with an additional patch.

Ben.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.