Re: [PATCH v2] mtd: rawnand: r852: preserve card changes during detection work

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mtd: rawnand: r852: inconsistent card state in
  r852_update_media_status()
--

commit 047b9555fe843aa22fb04dabbc400ccd2117fd0a
Author: James Hilliard <james.hilliard1@gmail.com>

mtd: rawnand: r852: preserve card changes during detection work

This commit serializes card-detect interrupt mask updates under irqlock
and samples card presence once in r852_card_detect_work() to avoid losing
concurrent card state transitions.

> diff --git a/drivers/mtd/nand/raw/r852.c b/drivers/mtd/nand/raw/r852.c
> index 92c47d351c278..4c094648673b4 100644
> --- a/drivers/mtd/nand/raw/r852.c
> +++ b/drivers/mtd/nand/raw/r852.c
[ ... ]
> @@ -684,20 +701,20 @@ static void r852_card_detect_work(struct work_struct *work)
>  {
>  	struct r852_device *dev =
>  		container_of(work, struct r852_device, card_detect_work.work);
> +	bool present;
>  
> -	r852_card_update_present(dev);
> +	present = r852_card_update_present(dev);
>  	r852_update_card_detect(dev);
> -	dev->card_unstable = 0;
>  
>  	/* False alarm */
> -	if (dev->card_detected == dev->card_registered)
> +	if (present == dev->card_registered)
>  		goto exit;
>  
>  	/* Read media properties */
>  	r852_update_media_status(dev);
>  
>  	/* Register the card */
> -	if (dev->card_detected)
> +	if (present)
>  		r852_register_nand_device(dev);
>  	else
>  		r852_unregister_nand_device(dev);

[Severity: Medium]
Can r852_update_media_status() observe a different card state than
r852_card_detect_work() when an interrupt fires after
r852_update_card_detect() unmasks card interrupts?

kworker                                   <hardirq>
r852_card_detect_work()
  present = r852_card_update_present(dev)
    /* present == true */
  r852_update_card_detect(dev)
                                          r852_irq()
                                            dev->card_detected = 0
                                            dev->card_unstable = 1
  r852_update_media_status(dev)
    if (!dev->card_detected)
      return;  <-- skips reading media status
  if (present)
    r852_register_nand_device(dev)

Because r852_update_media_status() still checks dev->card_detected while
r852_card_detect_work() branches on present, a removal interrupt during
insertion causes r852_update_media_status() to return early without
updating dev->readonly or dev->sm.  r852_register_nand_device() then runs
with stale media properties, enables the hardware engine via
r852_engine_enable(), and fails in sm_register_device() without disabling
the engine on the error1 path; the subsequent delayed work sees
present == dev->card_registered (0 == 0) and exits early, leaving the
engine enabled with no card present.

Conversely, if an insertion interrupt fires while handling a removal
(present == false), r852_update_media_status() sees dev->card_detected set
and reads R852_CARD_STA and R852_DMA_CAP while dev->card_unstable is set
right before r852_unregister_nand_device() runs.  Passing present to
r852_update_media_status() (or only calling it when present is true) would
keep both steps consistent.

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

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