Discussing the Linux ARM kernel

Lauri Hintsala | 10 Jul 2012 16:04

mmc: mxs: DEADLOCK

Hi,

I was able to get deadlock with CONFIG_DEBUG_SPINLOCK enabled. I added 
also CONFIG_PROVE_LOCKING to get more verbose output. I got following 
error message after SDIO device has been powered.

I'm able to replicate issue with Linux next-20120710. Platform is imx28.

[   79.660000] =============================================
[   79.660000] [ INFO: possible recursive locking detected ]
[   79.660000] 3.4.0-00009-g3e96082-dirty #11 Not tainted
[   79.660000] ---------------------------------------------
[   79.660000] swapper/0 is trying to acquire lock:
[   79.660000]  (&(&host->lock)->rlock#2){-.....}, at: [<c026ea3c>] 
mxs_mmc_enable_sdio_irq+0x18/0xd4
[   79.660000]
[   79.660000] but task is already holding lock:
[   79.660000]  (&(&host->lock)->rlock#2){-.....}, at: [<c026f744>] 
mxs_mmc_irq_handler+0x1c/0xe8
[   79.660000]
[   79.660000] other info that might help us debug this:
[   79.660000]  Possible unsafe locking scenario:
[   79.660000]
[   79.660000]        CPU0
[   79.660000]        ----
[   79.660000]   lock(&(&host->lock)->rlock#2);
[   79.660000]   lock(&(&host->lock)->rlock#2);
[   79.660000]
[   79.660000]  *** DEADLOCK ***
[   79.660000]

(Continue reading)

Marek Vasut | 10 Jul 2012 17:02

Picon

Picon

Re: mmc: mxs: DEADLOCK

Dear Lauri Hintsala,

[...]

> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
>  <at>  <at>  -278,11 +278,11  <at>  <at>  static irqreturn_t mxs_mmc_irq_handler(int irq,
> void *dev_id)
>   	writel(stat & MXS_MMC_IRQ_BITS,
>   	       host->base + HW_SSP_CTRL1(host) + STMP_OFFSET_REG_CLR);
> 
> +	spin_unlock(&host->lock);
> +
>   	if ((stat & BM_SSP_CTRL1_SDIO_IRQ) && (stat & BM_SSP_CTRL1_SDIO_IRQ_EN))
>   		mmc_signal_sdio_irq(host->mmc);
> 
> -	spin_unlock(&host->lock);
> -

Spinlock in irq handler is interesting too 

>   	if (stat & BM_SSP_CTRL1_RESP_TIMEOUT_IRQ)
>   		cmd->error = -ETIMEDOUT;
>   	else if (stat & BM_SSP_CTRL1_RESP_ERR_IRQ)
> 
> 
> Is there any reason to keep mmc_signal_sdio_irq inside the spinlock?
> mmc_signal_sdio_irq calls mxs_mmc_enable_sdio_irq and it tries to
> acquire lock while it is already acquired.
>

(Continue reading)

Shawn Guo | 11 Jul 2012 08:10

Re: mmc: mxs: DEADLOCK

On Tue, Jul 10, 2012 at 05:02:52PM +0200, Marek Vasut wrote:
> Dear Lauri Hintsala,
> 
> [...]
> 
> > --- a/drivers/mmc/host/mxs-mmc.c
> > +++ b/drivers/mmc/host/mxs-mmc.c
> >  <at>  <at>  -278,11 +278,11  <at>  <at>  static irqreturn_t mxs_mmc_irq_handler(int irq,
> > void *dev_id)
> >   	writel(stat & MXS_MMC_IRQ_BITS,
> >   	       host->base + HW_SSP_CTRL1(host) + STMP_OFFSET_REG_CLR);
> > 
> > +	spin_unlock(&host->lock);
> > +
> >   	if ((stat & BM_SSP_CTRL1_SDIO_IRQ) && (stat & BM_SSP_CTRL1_SDIO_IRQ_EN))
> >   		mmc_signal_sdio_irq(host->mmc);
> > 
> > -	spin_unlock(&host->lock);
> > -
> 
> Spinlock in irq handler is interesting too 
> 
For you information, the following is what I learnt from Arnd when I
was a beginner.

Regards,
Shawn

--- Quote Begins ---

(Continue reading)

Shawn Guo | 11 Jul 2012 08:06

Re: mmc: mxs: DEADLOCK

On Tue, Jul 10, 2012 at 05:04:42PM +0300, Lauri Hintsala wrote:
> Hi,
> 
> I was able to get deadlock with CONFIG_DEBUG_SPINLOCK enabled. I
> added also CONFIG_PROVE_LOCKING to get more verbose output. I got
> following error message after SDIO device has been powered.
> 
> I'm able to replicate issue with Linux next-20120710. Platform is imx28.
> 
The bug is there probably because the driver hasn't been widely tested
on SDIO card.

> I found a way to fix this issue:
> 
> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
>  <at>  <at>  -278,11 +278,11  <at>  <at>  static irqreturn_t mxs_mmc_irq_handler(int
> irq, void *dev_id)
>  	writel(stat & MXS_MMC_IRQ_BITS,
>  	       host->base + HW_SSP_CTRL1(host) + STMP_OFFSET_REG_CLR);
> 
> +	spin_unlock(&host->lock);
> +
>  	if ((stat & BM_SSP_CTRL1_SDIO_IRQ) && (stat & BM_SSP_CTRL1_SDIO_IRQ_EN))
>  		mmc_signal_sdio_irq(host->mmc);
> 
> -	spin_unlock(&host->lock);
> -
>  	if (stat & BM_SSP_CTRL1_RESP_TIMEOUT_IRQ)
>  		cmd->error = -ETIMEDOUT;

(Continue reading)

Lauri Hintsala | 11 Jul 2012 08:08

Re: mmc: mxs: DEADLOCK

On 07/11/2012 09:06 AM, Shawn Guo wrote:
>> --- a/drivers/mmc/host/mxs-mmc.c
>> +++ b/drivers/mmc/host/mxs-mmc.c
>>  <at>  <at>  -278,11 +278,11  <at>  <at>  static irqreturn_t mxs_mmc_irq_handler(int
>> irq, void *dev_id)
>>   	writel(stat & MXS_MMC_IRQ_BITS,
>>   	       host->base + HW_SSP_CTRL1(host) + STMP_OFFSET_REG_CLR);
>>
>> +	spin_unlock(&host->lock);
>> +
>>   	if ((stat & BM_SSP_CTRL1_SDIO_IRQ) && (stat & BM_SSP_CTRL1_SDIO_IRQ_EN))
>>   		mmc_signal_sdio_irq(host->mmc);
>>
>> -	spin_unlock(&host->lock);
>> -
>>   	if (stat & BM_SSP_CTRL1_RESP_TIMEOUT_IRQ)
>>   		cmd->error = -ETIMEDOUT;
>>   	else if (stat & BM_SSP_CTRL1_RESP_ERR_IRQ)
>>
>>
>> Is there any reason to keep mmc_signal_sdio_irq inside the spinlock?
>> mmc_signal_sdio_irq calls mxs_mmc_enable_sdio_irq and it tries to
>> acquire lock while it is already acquired.
>>
> The fix looks right to me.  You can have my ack when you send a patch
> for it.
>
> Acked-by: Shawn Guo <shawn.guo <at> linaro.org>

OK, I'll send a patch. Thanks!

(Continue reading)

Attila Kinali | 12 Jul 2012 16:00

Picon

Re: mmc: mxs: DEADLOCK

On Wed, 11 Jul 2012 14:06:09 +0800
Shawn Guo <shawn.guo <at> linaro.org> wrote:

> > I found a way to fix this issue:
> > 
> > --- a/drivers/mmc/host/mxs-mmc.c
> > +++ b/drivers/mmc/host/mxs-mmc.c
> >  <at>  <at>  -278,11 +278,11  <at>  <at>  static irqreturn_t mxs_mmc_irq_handler(int
> > irq, void *dev_id)
> >  	writel(stat & MXS_MMC_IRQ_BITS,
> >  	       host->base + HW_SSP_CTRL1(host) + STMP_OFFSET_REG_CLR);
> > 
> > +	spin_unlock(&host->lock);
> > +
> >  	if ((stat & BM_SSP_CTRL1_SDIO_IRQ) && (stat & BM_SSP_CTRL1_SDIO_IRQ_EN))
> >  		mmc_signal_sdio_irq(host->mmc);
> > 
> > -	spin_unlock(&host->lock);
> > -
> >  	if (stat & BM_SSP_CTRL1_RESP_TIMEOUT_IRQ)
> >  		cmd->error = -ETIMEDOUT;
> >  	else if (stat & BM_SSP_CTRL1_RESP_ERR_IRQ)
> > 
> > 
> > Is there any reason to keep mmc_signal_sdio_irq inside the spinlock?
> > mmc_signal_sdio_irq calls mxs_mmc_enable_sdio_irq and it tries to
> > acquire lock while it is already acquired.
> > 
> The fix looks right to me.  You can have my ack when you send a patch
> for it.

(Continue reading)

Shawn Guo | 12 Jul 2012 16:39

Re: mmc: mxs: DEADLOCK

On Thu, Jul 12, 2012 at 04:00:08PM +0200, Attila Kinali wrote:
> On Wed, 11 Jul 2012 14:06:09 +0800
> Shawn Guo <shawn.guo <at> linaro.org> wrote:
> 
> 
> > > I found a way to fix this issue:
> > > 
> > > --- a/drivers/mmc/host/mxs-mmc.c
> > > +++ b/drivers/mmc/host/mxs-mmc.c
> > >  <at>  <at>  -278,11 +278,11  <at>  <at>  static irqreturn_t mxs_mmc_irq_handler(int
> > > irq, void *dev_id)
> > >  	writel(stat & MXS_MMC_IRQ_BITS,
> > >  	       host->base + HW_SSP_CTRL1(host) + STMP_OFFSET_REG_CLR);
> > > 
> > > +	spin_unlock(&host->lock);
> > > +
> > >  	if ((stat & BM_SSP_CTRL1_SDIO_IRQ) && (stat & BM_SSP_CTRL1_SDIO_IRQ_EN))
> > >  		mmc_signal_sdio_irq(host->mmc);
> > > 
> > > -	spin_unlock(&host->lock);
> > > -
> > >  	if (stat & BM_SSP_CTRL1_RESP_TIMEOUT_IRQ)
> > >  		cmd->error = -ETIMEDOUT;
> > >  	else if (stat & BM_SSP_CTRL1_RESP_ERR_IRQ)
> > > 
> > > 
> > > Is there any reason to keep mmc_signal_sdio_irq inside the spinlock?
> > > mmc_signal_sdio_irq calls mxs_mmc_enable_sdio_irq and it tries to
> > > acquire lock while it is already acquired.
> > >

(Continue reading)

Attila Kinali | 12 Jul 2012 17:13

Picon

Re: mmc: mxs: DEADLOCK

On Thu, 12 Jul 2012 22:39:53 +0800
Shawn Guo <shawn.guo <at> linaro.org> wrote:

> > 
> > I ran into the same problem today, but the proposed fix doesn't seem
> > to work for me:
> > 
> It's a different problem from what Lauri reported and fixed.  

Ok... 

> I haven't
> played SDIO card that much, so I'm not completely clear about the SDIO
> calling sequence, but is it reasonable that mxs_mmc_enable_sdio_irq is
> being called recursively?

I don't know. I dont know the code at all and not how the sdio system
works. But a quick check shows, that mxs_mmc_enable_sdio_irq does not
call any other function (besides readel, writel) and hence cannot call itself.

For me it rather looks like that there seem to be two consequtive
irqs that get passed to sdio_irq_thread which then calls 
mxs_mmc_enable_sdio_irq.

But with my limited knowledge i cannot check this theory.
Can anyone give me some hints how i could verify this?

			Attila Kinali

--

--

(Continue reading)

Lauri Hintsala | 16 Jul 2012 07:57

Re: mmc: mxs: DEADLOCK

Hi Attila,

On 07/12/2012 05:00 PM, Attila Kinali wrote:
> I ran into the same problem today, but the proposed fix doesn't seem
> to work for me:
>
> ---schnipp---
> # modprobe libertas_sdio
> [   59.200000] lib80211: common routines for IEEE802.11 drivers
> [   59.240000] cfg80211: Calling CRDA to update world regulatory domain
> [   59.320000] libertas_sdio: Libertas SDIO driver
> [   59.330000] libertas_sdio: Copyright Pierre Ossman
> # modprobe mxs-mmc
> [   64.210000] mxs-mmc 80010000.ssp: initialized
> [   64.260000] mxs-mmc 80034000.ssp: initialized
> [   64.270000] mmc0: new SDIO card at address 0001
> # [   65.440000] libertas_sdio mmc0:0001:1: (unregistered net_device): 00:13:04:80:00:3f, fw
9.70.3p24, cap 0x00000303
> [   65.470000]
> [   65.470000] =============================================
> [   65.470000] [ INFO: possible recursive locking detected ]
> [   65.470000] 3.5.0-rc5 #2 Not tainted
> [   65.470000] ---------------------------------------------
> [   65.470000] ksdioirqd/mmc0/73 is trying to acquire lock:
> [   65.470000]  (&(&host->lock)->rlock#2){-.-...}, at: [<bf054120>]
mxs_mmc_enable_sdio_irq+0x18/0xdc [mxs_mmc]
> [   65.470000]
> [   65.470000] but task is already holding lock:
> [   65.470000]  (&(&host->lock)->rlock#2){-.-...}, at: [<bf054120>]
mxs_mmc_enable_sdio_irq+0x18/0xdc [mxs_mmc]

(Continue reading)

Attila Kinali | 16 Jul 2012 14:07

Picon

Re: mmc: mxs: DEADLOCK

Moin, Lauri,

On Mon, 16 Jul 2012 08:57:38 +0300
Lauri Hintsala <lauri.hintsala <at> bluegiga.com> wrote:

> 
> 
> Does this patch fix your issue?

A preliminary test shows that it at least fixes the oops at module loading.
I haven't had the chance yet to give it a full test, but i would say it
fixes it enough to be workable.

Thanks a lot!

			Attila Kinali

--

-- 
The trouble with you, Shev, is you don't say anything until you've saved
up a whole truckload of damned heavy brick arguments and then you dump
them all out and never look at the bleeding body mangled beneath the heap
		-- Tirin, The Dispossessed, U. Le Guin
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lauri Hintsala | 17 Jul 2012 06:54

Re: mmc: mxs: DEADLOCK

Shawn,

Could you review this patch? Attila reported it fixes his SDIO 
initialization issue.

Lauri

On 07/16/2012 08:57 AM, Lauri Hintsala wrote:
>> Any hints how to work around or fix this, would be appreciated
>
>
> Does this patch fix your issue?
>
>  >>>>>>>
> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
>  <at>  <at>  -637,11 +637,6  <at>  <at>  static void mxs_mmc_enable_sdio_irq(struct mmc_host
> *mmc, int enable)
>                  host->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
>           writel(BM_SSP_CTRL1_SDIO_IRQ_EN,
>                  host->base + HW_SSP_CTRL1(host) + STMP_OFFSET_REG_SET);
> -
> -        if (readl(host->base + HW_SSP_STATUS(host)) &
> -                BM_SSP_STATUS_SDIO_IRQ)
> -            mmc_signal_sdio_irq(host->mmc);
> -
>       } else {
>           writel(BM_SSP_CTRL0_SDIO_IRQ_CHECK,
>                  host->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
>  <at>  <at>  -650,6 +645,11  <at>  <at>  static void mxs_mmc_enable_sdio_irq(struct mmc_host

(Continue reading)

Shawn Guo | 17 Jul 2012 14:40

Re: mmc: mxs: DEADLOCK

On Tue, Jul 17, 2012 at 07:54:39AM +0300, Lauri Hintsala wrote:
> Shawn,
> 
> Could you review this patch? Attila reported it fixes his SDIO
> initialization issue.
> 
Thanks for fixing it.

Acked-by: Shawn Guo <shawn.guo <at> linaro.org>

> Lauri
> 
> 
> On 07/16/2012 08:57 AM, Lauri Hintsala wrote:
> >>Any hints how to work around or fix this, would be appreciated
> >
> >
> >Does this patch fix your issue?
> >
> > >>>>>>>
> >--- a/drivers/mmc/host/mxs-mmc.c
> >+++ b/drivers/mmc/host/mxs-mmc.c
> > <at>  <at>  -637,11 +637,6  <at>  <at>  static void mxs_mmc_enable_sdio_irq(struct mmc_host
> >*mmc, int enable)
> >                 host->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> >          writel(BM_SSP_CTRL1_SDIO_IRQ_EN,
> >                 host->base + HW_SSP_CTRL1(host) + STMP_OFFSET_REG_SET);
> >-
> >-        if (readl(host->base + HW_SSP_STATUS(host)) &
> >-                BM_SSP_STATUS_SDIO_IRQ)

(Continue reading)

Lauri Hintsala | 17 Jul 2012 15:03

Re: mmc: mxs: DEADLOCK

On 07/17/2012 03:40 PM, Shawn Guo wrote:
> On Tue, Jul 17, 2012 at 07:54:39AM +0300, Lauri Hintsala wrote:
>> Shawn,
>>
>> Could you review this patch? Attila reported it fixes his SDIO
>> initialization issue.
>>
> Thanks for fixing it.
>
> Acked-by: Shawn Guo <shawn.guo <at> linaro.org>

Thanks. I'll send both this and previous patches to mailing lists.

Lauri
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Return

Return to gmane.linux.ports.arm.kernel.

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane