Re: [PATCH 18/58] mtd: rawnand: atmel: retain chips across controller removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] mtd: rawnand: atmel: initialize __free(kfree) variables at
  declaration
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] mtd: rawnand: atmel: unclocked register access in
  atmel_nfc_interrupt() [1]
--

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

mtd: rawnand: atmel: retain chips across controller removal

This commit retains NAND chip and controller allocations until all MTD
references are released and disconnects chips before tearing down controller
resources.

> diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c b/drivers/mtd/nand/raw/atmel/nand-controller.c
> index 0bdbcbb50d94c..10b725fa61d86 100644
> --- a/drivers/mtd/nand/raw/atmel/nand-controller.c
> +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c

[ ... ]

> @@ -1638,7 +1640,7 @@ static struct atmel_nand *atmel_nand_create(struct atmel_nand_controller *nc,
>  					    struct device_node *np,
>  					    int reg_cells)
>  {
> -	struct atmel_nand *nand;
> +	struct atmel_nand *nand __free(kfree) = NULL;
>  	struct gpio_desc *gpio;
>  	int numcs, ret, i;
>  
> @@ -1649,7 +1651,7 @@ static struct atmel_nand *atmel_nand_create(struct atmel_nand_controller *nc,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	nand = devm_kzalloc(nc->dev, struct_size(nand, cs, numcs), GFP_KERNEL);
> +	nand = kzalloc(struct_size(nand, cs, numcs), GFP_KERNEL);
>  	if (!nand)
>  		return ERR_PTR(-ENOMEM);

[ ... ]

> @@ -1791,7 +1809,7 @@ atmel_nand_controller_legacy_add_nands(struct atmel_nand_controller *nc)
>  {
>  	struct device *dev = nc->dev;
>  	struct platform_device *pdev = to_platform_device(dev);
> -	struct atmel_nand *nand;
> +	struct atmel_nand *nand __free(kfree) = NULL;
>  	struct gpio_desc *gpio;
>  	struct resource *res;
>  
> @@ -1799,7 +1817,7 @@ atmel_nand_controller_legacy_add_nands(struct atmel_nand_controller *nc)
>  	 * Legacy bindings only allow connecting a single NAND with a unique CS
>  	 * line to the controller.
>  	 */
> -	nand = devm_kzalloc(nc->dev, struct_size(nand, cs, 1), GFP_KERNEL);
> +	nand = kzalloc(struct_size(nand, cs, 1), GFP_KERNEL);
>  	if (!nand)
>  		return -ENOMEM;

[Severity: Low]
This isn't a bug, but per the Cleanup API conventions in
include/linux/cleanup.h, __free() variables should be defined and
initialized in a single statement at their allocation point rather than
declared as "= NULL" at the top of atmel_nand_create() and
atmel_nand_controller_legacy_add_nands() and assigned later.

[ ... ]

> @@ -2349,6 +2366,11 @@ atmel_hsmc_nand_controller_remove(struct atmel_nand_controller *nc)
>  	return 0;
>  }
>  
> +static void atmel_hsmc_nand_controller_release(struct nand_controller *controller)
> +{
> +	kfree(to_hsmc_nand_controller(controller));
> +}
> +
>  static int atmel_hsmc_nand_controller_probe(struct platform_device *pdev,
>  				const struct atmel_nand_controller_caps *caps)
>  {
> @@ -2356,9 +2378,13 @@ static int atmel_hsmc_nand_controller_probe(struct platform_device *pdev,
>  	struct atmel_hsmc_nand_controller *nc;
>  	int ret;
>  
> -	nc = devm_kzalloc(dev, sizeof(*nc), GFP_KERNEL);
> +	nc = kzalloc_obj(*nc);
>  	if (!nc)
>  		return -ENOMEM;
> +	ret = devm_nand_controller_init(dev, &nc->base.base,
> +					atmel_hsmc_nand_controller_release);
> +	if (ret)
> +		return ret;

[Severity: High]
This problem wasn't introduced by this patch, but can atmel_nfc_interrupt()
perform an unclocked register read during teardown [1]?

atmel_hsmc_nand_controller_probe() registers atmel_nfc_interrupt() with
devm_request_irq(..., IRQF_SHARED, ...), while
atmel_hsmc_nand_controller_remove() manually disables hsmc_nc->clk before
devres releases the IRQ:

drivers/mtd/nand/raw/atmel/nand-controller.c:atmel_hsmc_nand_controller_remove() {
    ...
	if (hsmc_nc->clk) {
		clk_disable_unprepare(hsmc_nc->clk);
		clk_put(hsmc_nc->clk);
	}
    ...
}

CPU0                                    CPU1
atmel_hsmc_nand_controller_remove()
  clk_disable_unprepare(hsmc_nc->clk)
                                        <shared IRQ fires>
                                        atmel_nfc_interrupt()
                                          regmap_read(..., ATMEL_HSMC_NFC_SR, ...)
                                            <-- unclocked register access
<devres cleanup>
  devm_irq_release()

If another device on the shared IRQ line raises an interrupt before devres
frees the handler, reading ATMEL_HSMC_NFC_SR with the clock disabled can
trigger a synchronous external abort.

[1] https://sashiko.dev/#/bug/linux-94d27cd6-f6fe-452a-863e-36cbb87e9adc

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic