[PATCH] mtd: rawnand: r852: lock card-detection interrupt updates

[PATCH] mtd: rawnand: r852: lock card-detection interrupt updates

[James Hilliard]

r852_update_card_detect() reads and writes the card interrupt enable
register without holding irqlock. A card interrupt can mask detection
interrupts and mark the card unstable between those accesses, only for
the worker to overwrite the mask with its saved value.

Hold irqlock across the register update and the transition to stable card
state. Remove the redundant unlocked card_unstable assignment in the
worker, which could otherwise clear an instability indication recorded
by an interrupt after the helper returns.

Fixes: 67e054e91924 ("mtd: nand: Add driver for Ricoh xD/SmartMedia reader")
Cc: stable@vger.kernel.org
Closes: https://lore.kernel.org/r/sashiko-outbox-204068@kernel.org/
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/mtd/nand/raw/r852.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/r852.c b/drivers/mtd/nand/raw/r852.c
index 92c47d351c27..3ff4294a80dc 100644
--- a/drivers/mtd/nand/raw/r852.c
+++ b/drivers/mtd/nand/raw/r852.c
@@ -574,7 +574,11 @@ static void r852_card_update_present(struct r852_device *dev)
  */
 static void r852_update_card_detect(struct r852_device *dev)
 {
-	int card_detect_reg = r852_read_reg(dev, R852_CARD_IRQ_ENABLE);
+	unsigned long flags;
+	int card_detect_reg;
+
+	spin_lock_irqsave(&dev->irqlock, flags);
+	card_detect_reg = r852_read_reg(dev, R852_CARD_IRQ_ENABLE);
 	dev->card_unstable = 0;
 
 	card_detect_reg &= ~(R852_CARD_IRQ_REMOVE | R852_CARD_IRQ_INSERT);
@@ -584,6 +588,7 @@ static void r852_update_card_detect(struct r852_device *dev)
 		R852_CARD_IRQ_REMOVE : R852_CARD_IRQ_INSERT;
 
 	r852_write_reg(dev, R852_CARD_IRQ_ENABLE, card_detect_reg);
+	spin_unlock_irqrestore(&dev->irqlock, flags);
 }
 
 static ssize_t media_type_show(struct device *sys_dev,
@@ -687,7 +692,6 @@ static void r852_card_detect_work(struct work_struct *work)
 
 	r852_card_update_present(dev);
 	r852_update_card_detect(dev);
-	dev->card_unstable = 0;
 
 	/* False alarm */
 	if (dev->card_detected == dev->card_registered)

---
base-commit: 30daaf3e7897b4a8d9b72e5b0c86dd6e2a4d7d85
change-id: 20261108-submit-r852-card-detect-lock-v1-713bf5cd926b

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: r852: lock card-detection interrupt updates

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mtd: rawnand: r852: r852_update_card_detect() still clobbers
  unstable state
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] mtd: rawnand: r852: double unmap in r852_dma_done() [1]
--

commit 8cd576bef40790bcf94d0e276f1f7477d504eeef
Author: James Hilliard <james.hilliard1@gmail.com>

mtd: rawnand: r852: lock card-detection interrupt updates

This commit takes dev->irqlock in r852_update_card_detect() when updating
R852_CARD_IRQ_ENABLE and clearing dev->card_unstable, and removes the
unlocked assignment to dev->card_unstable in r852_card_detect_work().

> diff --git a/drivers/mtd/nand/raw/r852.c b/drivers/mtd/nand/raw/r852.c
> index 92c47d351c278..3ff4294a80dcd 100644
> --- a/drivers/mtd/nand/raw/r852.c
> +++ b/drivers/mtd/nand/raw/r852.c
> @@ -574,7 +574,11 @@ static void r852_card_update_present(struct r852_device *dev)
>   */
>  static void r852_update_card_detect(struct r852_device *dev)
>  {
> -	int card_detect_reg = r852_read_reg(dev, R852_CARD_IRQ_ENABLE);
> +	unsigned long flags;
> +	int card_detect_reg;
> +
> +	spin_lock_irqsave(&dev->irqlock, flags);
> +	card_detect_reg = r852_read_reg(dev, R852_CARD_IRQ_ENABLE);
>  	dev->card_unstable = 0;
>  
>  	card_detect_reg &= ~(R852_CARD_IRQ_REMOVE | R852_CARD_IRQ_INSERT);
> @@ -584,6 +588,7 @@ static void r852_update_card_detect(struct r852_device *dev)
>  		R852_CARD_IRQ_REMOVE : R852_CARD_IRQ_INSERT;
>  
>  	r852_write_reg(dev, R852_CARD_IRQ_ENABLE, card_detect_reg);
> +	spin_unlock_irqrestore(&dev->irqlock, flags);
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but nothing serializes [1]
r852_dma_done() when a DMA timeout in r852_do_dma() races with the completion
interrupt in r852_irq():

drivers/mtd/nand/raw/r852.c:r852_do_dma() {
	...
	error = r852_dma_wait(dev);

	if (error) {
		r852_dma_done(dev, error);
		return;
	}
	...
}

drivers/mtd/nand/raw/r852.c:r852_dma_done() {
	...
	if (dev->phys_dma_addr && dev->phys_dma_addr != dev->phys_bounce_buffer)
		dma_unmap_single(&dev->pci_dev->dev, dev->phys_dma_addr,
			R852_DMA_LEN,
			dev->dma_dir ? DMA_FROM_DEVICE : DMA_TO_DEVICE);
}

CPU0                              CPU1
r852_do_dma()                     r852_irq()
  r852_dma_wait(dev)
  /* times out */                   spin_lock_irqsave(&dev->irqlock, ...)
  r852_dma_done(dev, error)         r852_dma_done(dev, 0)
    if (dev->phys_dma_addr)           if (dev->phys_dma_addr)
      dma_unmap_single(...)             dma_unmap_single(...) <-- double unmap

r852_do_dma() calls r852_dma_done() without holding dev->irqlock, and
r852_dma_done() does not clear dev->phys_dma_addr after unmapping.  Can both
paths unmap dev->phys_dma_addr and corrupt DMA API / SWIOTLB state?

[ ... ]

> @@ -687,7 +692,6 @@ static void r852_card_detect_work(struct work_struct *work)
>  
>  	r852_card_update_present(dev);
>  	r852_update_card_detect(dev);
> -	dev->card_unstable = 0;
>  
>  	/* False alarm */
>  	if (dev->card_detected == dev->card_registered)

[Severity: Medium]
Can the second r852_update_card_detect() call at the end of
r852_card_detect_work() still overwrite dev->card_unstable and re-enable
card-detection interrupts after an interrupt has fired?

drivers/mtd/nand/raw/r852.c:r852_card_detect_work() {
	...
	r852_card_update_present(dev);
	r852_update_card_detect(dev);

	/* False alarm */
	if (dev->card_detected == dev->card_registered)
		goto exit;

	/* Read media properties */
	r852_update_media_status(dev);

	/* Register the card */
	if (dev->card_detected)
		r852_register_nand_device(dev);
	else
		r852_unregister_nand_device(dev);
exit:
	r852_update_card_detect(dev);
}

kworker                           <hardirq>
r852_card_detect_work()
  r852_update_card_detect(dev)
    /* enables card IRQs */
  ...                             r852_irq()
                                    r852_disable_irqs(dev)
                                    dev->card_unstable = 1
                                    queue_delayed_work(..., 100ms)
  r852_update_card_detect(dev)
    dev->card_unstable = 0  <-- clobbers unstable state and re-enables IRQs

The first r852_update_card_detect() call unmasks card interrupts before the
worker registers or unregisters the NAND device.  If a card interrupt fires
during that window, r852_irq() masks interrupts, sets dev->card_unstable = 1
and queues card_detect_work for the 100ms debounce delay, only for the exit
call to clear dev->card_unstable and unmask card interrupts while the card is
still settling.

Also, because card interrupts are already unmasked at the first call,
r852_irq() can change dev->card_detected between the unlocked checks
(dev->card_detected == dev->card_registered and if (dev->card_detected)),
which can call r852_register_nand_device() when dev->card_registered is
already set and hit WARN_ON(dev->card_registered).

[1] https://sashiko.dev/#/bug/linux-69af48df-28fa-4234-8f16-07c609427395

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-r852-card-detect-lock-v1-v1-1-5514e7543209@gmail.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/