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

[sashiko-bot@kernel.org]

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/