| Message ID | 1477594672-31611-2-git-send-email-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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); -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:" -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);