[PATCH 1/2] mmc: mxs: Initialize the spinlock prior to using it

[PATCH 1/2] mmc: mxs: Initialize the spinlock prior to using it

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@nxp.com>

An interrupt may occur right after devm_request_irq() is called and
prior to the spinlock initialization, leading to a kernel oops,
as the interrupt handler uses the spinlock.

In order to prevent this problem, move the spinlock initialization 
prior to requesting the interrupts.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 drivers/mmc/host/mxs-mmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
index d839147..44ecebd 100644
--- a/drivers/mmc/host/mxs-mmc.c
+++ b/drivers/mmc/host/mxs-mmc.c
@@ -661,13 +661,13 @@ static int mxs_mmc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mmc);
 
+	spin_lock_init(&host->lock);
+
 	ret = devm_request_irq(&pdev->dev, irq_err, mxs_mmc_irq_handler, 0,
 			       dev_name(&pdev->dev), host);
 	if (ret)
 		goto out_free_dma;
 
-	spin_lock_init(&host->lock);
-
 	ret = mmc_add_host(mmc);
 	if (ret)
 		goto out_free_dma;
-- 
2.7.4

[PATCH 2/2] mmc: mxs: Use the spinlock irq variants

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@nxp.com>

Inside an interrupt handler we should use the spin_lock_irqsave()/
spin_unlock_irqrestore() variants, so fix it accordingly.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 drivers/mmc/host/mxs-mmc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
index 44ecebd..14b7548 100644
--- a/drivers/mmc/host/mxs-mmc.c
+++ b/drivers/mmc/host/mxs-mmc.c
@@ -189,15 +189,16 @@ static irqreturn_t mxs_mmc_irq_handler(int irq, void *dev_id)
 	struct mmc_command *cmd = host->cmd;
 	struct mmc_data *data = host->data;
 	struct mxs_ssp *ssp = &host->ssp;
+	unsigned long flags;
 	u32 stat;
 
-	spin_lock(&host->lock);
+	spin_lock_irqsave(&host->lock, flags);
 
 	stat = readl(ssp->base + HW_SSP_CTRL1(ssp));
 	writel(stat & MXS_MMC_IRQ_BITS,
 	       ssp->base + HW_SSP_CTRL1(ssp) + STMP_OFFSET_REG_CLR);
 
-	spin_unlock(&host->lock);
+	spin_unlock_irqrestore(&host->lock, flags);
 
 	if ((stat & BM_SSP_CTRL1_SDIO_IRQ) && (stat & BM_SSP_CTRL1_SDIO_IRQ_EN))
 		mmc_signal_sdio_irq(host->mmc);
-- 
2.7.4

Re: [PATCH 1/2] mmc: mxs: Initialize the spinlock prior to using it

[Marek Vasut]

On 10/27/2016 08:57 PM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> An interrupt may occur right after devm_request_irq() is called and
> prior to the spinlock initialization, leading to a kernel oops,
> as the interrupt handler uses the spinlock.
> 
> In order to prevent this problem, move the spinlock initialization 
> prior to requesting the interrupts.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

Nice, thanks.

Could you also check whether you can disable and clear interrupts in the
probe routine ? I think that might make sense too.

Reviewed-by: Marek Vasut <marex@denx.de>

> ---
>  drivers/mmc/host/mxs-mmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
> index d839147..44ecebd 100644
> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
> @@ -661,13 +661,13 @@ static int mxs_mmc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, mmc);
>  
> +	spin_lock_init(&host->lock);
> +
>  	ret = devm_request_irq(&pdev->dev, irq_err, mxs_mmc_irq_handler, 0,
>  			       dev_name(&pdev->dev), host);
>  	if (ret)
>  		goto out_free_dma;
>  
> -	spin_lock_init(&host->lock);
> -
>  	ret = mmc_add_host(mmc);
>  	if (ret)
>  		goto out_free_dma;
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH 2/2] mmc: mxs: Use the spinlock irq variants

[Marek Vasut]

On 10/27/2016 08:57 PM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> Inside an interrupt handler we should use the spin_lock_irqsave()/
> spin_unlock_irqrestore() variants, so fix it accordingly.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

Reviewed-by: Marek Vasut <marex@denx.de>

-- 
Best regards,
Marek Vasut

Re: [PATCH 1/2] mmc: mxs: Initialize the spinlock prior to using it

[Fabio Estevam]

Hi Marek,

On Thu, Oct 27, 2016 at 5:06 PM, Marek Vasut <marex@denx.de> wrote:

> Nice, thanks.
>
> Could you also check whether you can disable and clear interrupts in the
> probe routine ? I think that might make sense too.

Yes, that's a good idea. I will look into that after Ulf applies this
series. Thanks

Re: [PATCH 1/2] mmc: mxs: Initialize the spinlock prior to using it

[Marek Vasut]

On 10/27/2016 09:17 PM, Fabio Estevam wrote:
> Hi Marek,
> 
> On Thu, Oct 27, 2016 at 5:06 PM, Marek Vasut <marex@denx.de> wrote:
> 
>> Nice, thanks.
>>
>> Could you also check whether you can disable and clear interrupts in the
>> probe routine ? I think that might make sense too.
> 
> Yes, that's a good idea. I will look into that after Ulf applies this
> series. Thanks
> 
Cool, thanks!

-- 
Best regards,
Marek Vasut

Re: [PATCH 2/2] mmc: mxs: Use the spinlock irq variants

[Jisheng Zhang]

On Thu, 27 Oct 2016 16:57:52 -0200 Fabio Estevam wrote:

> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> Inside an interrupt handler we should use the spin_lock_irqsave()/
> spin_unlock_irqrestore() variants, so fix it accordingly.

hmm, in interrupt handler the irq is disabled, so IMHO there's no need to
use irqsave/irqrestore spinlock variants.

Thanks,
Jisheng

> 
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
>  drivers/mmc/host/mxs-mmc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
> index 44ecebd..14b7548 100644
> --- a/drivers/mmc/host/mxs-mmc.c
> +++ b/drivers/mmc/host/mxs-mmc.c
> @@ -189,15 +189,16 @@ static irqreturn_t mxs_mmc_irq_handler(int irq, void *dev_id)
>  	struct mmc_command *cmd = host->cmd;
>  	struct mmc_data *data = host->data;
>  	struct mxs_ssp *ssp = &host->ssp;
> +	unsigned long flags;
>  	u32 stat;
>  
> -	spin_lock(&host->lock);
> +	spin_lock_irqsave(&host->lock, flags);
>  
>  	stat = readl(ssp->base + HW_SSP_CTRL1(ssp));
>  	writel(stat & MXS_MMC_IRQ_BITS,
>  	       ssp->base + HW_SSP_CTRL1(ssp) + STMP_OFFSET_REG_CLR);
>  
> -	spin_unlock(&host->lock);
> +	spin_unlock_irqrestore(&host->lock, flags);
>  
>  	if ((stat & BM_SSP_CTRL1_SDIO_IRQ) && (stat & BM_SSP_CTRL1_SDIO_IRQ_EN))
>  		mmc_signal_sdio_irq(host->mmc);

Re: [PATCH 2/2] mmc: mxs: Use the spinlock irq variants

[Fabio Estevam]

Hi Jisheng,

On Fri, Oct 28, 2016 at 12:23 AM, Jisheng Zhang <jszhang@marvell.com> wrote:

> hmm, in interrupt handler the irq is disabled, so IMHO there's no need to
> use irqsave/irqrestore spinlock variants.

Please check Documentation/locking/spinlocks.txt:

"IFF you know that the spinlocks are
never used in interrupt handlers, you can use the non-irq versions:"

Re: [PATCH 2/2] mmc: mxs: Use the spinlock irq variants

[Jisheng Zhang]

Hi Fabio,

On Fri, 28 Oct 2016 09:40:29 -0200 Fabio Estevam wrote:

> Hi Jisheng,
> 
> On Fri, Oct 28, 2016 at 12:23 AM, Jisheng Zhang <jszhang@marvell.com> wrote:
> 
> > hmm, in interrupt handler the irq is disabled, so IMHO there's no need to
> > use irqsave/irqrestore spinlock variants.  
> 
> Please check Documentation/locking/spinlocks.txt:
> 
> "IFF you know that the spinlocks are
> never used in interrupt handlers, you can use the non-irq versions:"

This is not the key. The key here is: 

compared with the non-irq version, the irq-version spin lock does one extra
work: disable the local interrupts. But since we are in interrupt handler, 
the local irq is already disabled, so there's no need for the irq-versions.

I'm not sure I understand the situation, comments are welcome.

Thanks,
Jisheng

Re: [PATCH 1/2] mmc: mxs: Initialize the spinlock prior to using it

[Stefan Wahren]

Hi Fabio,

Am 27.10.2016 um 20:57 schrieb Fabio Estevam:
> From: Fabio Estevam <fabio.estevam@nxp.com>
>
> An interrupt may occur right after devm_request_irq() is called and
> prior to the spinlock initialization, leading to a kernel oops,
> as the interrupt handler uses the spinlock.
>
> In order to prevent this problem, move the spinlock initialization 
> prior to requesting the interrupts.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

this seems to be a bugfix, so please add a fixes tag here.

Maybe this could go to stable.

Thanks
Stefan

Re: [PATCH 2/2] mmc: mxs: Use the spinlock irq variants

[Fabio Estevam]

Hi Jisheng,

On Fri, Oct 28, 2016 at 9:49 AM, Jisheng Zhang <jszhang@marvell.com> wrote:

> This is not the key. The key here is:
>
> compared with the non-irq version, the irq-version spin lock does one extra
> work: disable the local interrupts. But since we are in interrupt handler,
> the local irq is already disabled, so there's no need for the irq-versions.
>
> I'm not sure I understand the situation, comments are welcome.

Quoting  http://www.xml.com/ldd/chapter/book/ch09.html:

"Many users of spinlocks stick to spin_lock and spin_unlock. If you
are using spinlocks in interrupt handlers, however, you must use the
IRQ-disabling versions (usually spin_lock_irqsave and
spin_unlock_irqsave) in the noninterrupt code. To do otherwise is to
invite a deadlock situation.

It is worth considering an example here. Assume that your driver is
running in its read method, and it obtains a lock with spin_lock.
While the read method is holding the lock, your device interrupts, and
your interrupt handler is executed on the same processor. If it
attempts to use the same lock, it will go into a busy-wait loop, since
your read method already holds the lock. But, since the interrupt
routine has preempted that method, the lock will never be released and
the processor deadlocks, which is probably not what you wanted.

This problem can be avoided by using spin_lock_irqsave to disable
interrupts on the local processor while the lock is held. When in
doubt, use the _irqsave versions of the primitives and you will not
need to worry about deadlocks. Remember, though, that the flags value
from spin_lock_irqsave must not be passed to other functions."

And as I mentioned before Documentation/locking/spinlocks.txt is very
clear about the need of using spin_lock_irqsave inside interrupt
handlers.

Feel free to submit a patch to Documentation/locking/spinlocks.txt if
you think this is incorrect.

Thanks

Re: [PATCH 2/2] mmc: mxs: Use the spinlock irq variants

[Fabio Estevam]

Hi Jisheng,

On Sat, Nov 5, 2016 at 4:56 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Jisheng,
>
> On Fri, Oct 28, 2016 at 9:49 AM, Jisheng Zhang <jszhang@marvell.com> wrote:
>
>> This is not the key. The key here is:
>>
>> compared with the non-irq version, the irq-version spin lock does one extra
>> work: disable the local interrupts. But since we are in interrupt handler,
>> the local irq is already disabled, so there's no need for the irq-versions.
>>
>> I'm not sure I understand the situation, comments are welcome.
>
> Quoting  http://www.xml.com/ldd/chapter/book/ch09.html:
>
> "Many users of spinlocks stick to spin_lock and spin_unlock. If you
> are using spinlocks in interrupt handlers, however, you must use the
> IRQ-disabling versions (usually spin_lock_irqsave and
> spin_unlock_irqsave) in the noninterrupt code. To do otherwise is to
> invite a deadlock situation.

Ah, after re-reading it several times I agree with you now. It is the
noninterrupt code that needs the irq spinlock versions.

I will resend the series without this patch.

Thanks