Re: [PATCH 55/58] mtd: rawnand: r852: retain chip state across removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mtd: rawnand: r852: r852_irq() mishandles card IRQs when
  removing is set
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] mtd: rawnand: r852: work re-queue race condition in
  r852_shutdown() [1]
--

commit 85c05cbe4afe297d6f7e2a571f936dd2dc915c61
Author: James Hilliard <james.hilliard1@gmail.com>

mtd: rawnand: r852: retain chip state across removal

This commit allocates a separate nand_chip per card insertion, ties chip and
controller lifetimes to MTD release callbacks, and stops card-detection work
scheduling in r852_irq() during PCI removal.

> diff --git a/drivers/mtd/nand/raw/r852.c b/drivers/mtd/nand/raw/r852.c
> index 92c47d351c278..0fb320cd7b771 100644
> --- a/drivers/mtd/nand/raw/r852.c
> +++ b/drivers/mtd/nand/raw/r852.c
[ ... ]
> @@ -734,7 +773,8 @@ static irqreturn_t r852_irq(int irq, void *data)
>  	card_status = r852_read_reg(dev, R852_CARD_IRQ_STA);
>  	r852_write_reg(dev, R852_CARD_IRQ_STA, card_status);
>  
> -	if (card_status & (R852_CARD_IRQ_INSERT|R852_CARD_IRQ_REMOVE)) {
> +	if (!dev->removing &&
> +	    (card_status & (R852_CARD_IRQ_INSERT | R852_CARD_IRQ_REMOVE))) {
>  
>  		ret = IRQ_HANDLED;
>  		dev->card_detected = !!(card_status & R852_CARD_IRQ_INSERT);

[Severity: Medium]
Does checking !dev->removing on the outer condition cause r852_irq() to
mishandle card interrupts that fire during r852_remove()?

Hardware interrupts remain enabled while r852_remove() cancels the workqueue
and runs r852_unregister_nand_device().  If a card insert or remove interrupt
arrives after dev->removing is set, r852_irq() clears R852_CARD_IRQ_STA in
hardware but skips the handler block, returning IRQ_NONE on a shared IRQ line
and failing to set dev->card_unstable or call r852_disable_irqs() to abort
in-flight I/O.  Should dev->removing only guard queue_delayed_work()?

[ ... ]
> @@ -976,6 +1003,11 @@ static int  r852_probe(struct pci_dev *pci_dev, const struct pci_device_id *id)
>  static void r852_remove(struct pci_dev *pci_dev)
>  {
>  	struct r852_device *dev = pci_get_drvdata(pci_dev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->irqlock, flags);
> +	dev->removing = true;
> +	spin_unlock_irqrestore(&dev->irqlock, flags);
>  
>  	/* Stop detect workqueue -
>  		we are going to unregister the device anyway*/

[Severity: High]
This problem wasn't introduced by this patch, but can r852_irq() re-queue
dev->card_detect_work during r852_shutdown() and r852_suspend() [1]?

Both functions call cancel_delayed_work_sync() before r852_disable_irqs()
without setting dev->removing:

drivers/mtd/nand/raw/r852.c:r852_shutdown() {
	struct r852_device *dev = pci_get_drvdata(pci_dev);

	cancel_delayed_work_sync(&dev->card_detect_work);
	r852_disable_irqs(dev);
	synchronize_irq(dev->irq);
	pci_disable_device(pci_dev);
}

An interrupt arriving between cancel_delayed_work_sync() and
r852_disable_irqs() can re-queue card_detect_work:

CPU0                              CPU1
r852_shutdown()
  cancel_delayed_work_sync()
                                  r852_irq()
                                    queue_delayed_work()
  r852_disable_irqs()
  synchronize_irq()
  pci_disable_device()
                                  r852_card_detect_work()
                                    r852_read_reg()  <-- MMIO after disable

When the delayed work runs after pci_disable_device(), it accesses disabled
PCI MMIO space.

[1] https://sashiko.dev/#/bug/linux-ce28ca2b-1bf1-4d27-8bcd-a0ec14b99281

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-submit-rawnand-retained-handle-lifetime-v1-v1-0-0181ef0cc4c8@gmail.com?part=55