Re: [PATCH v2 RFC net-next 03/18] net: mvpp2: add CM3 SRAM memory map

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Sun, Jan 24, 2021 at 01:43:52PM +0200, stefanc@marvell.com wrote:
> +static int mvpp2_get_sram(struct platform_device *pdev,
> +			  struct mvpp2 *priv)
> +{
> +	struct device_node *dn = pdev->dev.of_node;
> +	static bool defer_once;
> +	struct resource *res;
> +
> +	if (has_acpi_companion(&pdev->dev)) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +		if (!res) {
> +			dev_warn(&pdev->dev, "ACPI is too old, Flow control not supported\n");
> +			return 0;
> +		}
> +		priv->cm3_base = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(priv->cm3_base))
> +			return PTR_ERR(priv->cm3_base);
> +	} else {
> +		priv->sram_pool = of_gen_pool_get(dn, "cm3-mem", 0);
> +		if (!priv->sram_pool) {
> +			if (!defer_once) {
> +				defer_once = true;
> +				/* Try defer once */
> +				return -EPROBE_DEFER;
> +			}
> +			dev_warn(&pdev->dev, "DT is too old, Flow control not supported\n");
> +			return -ENOMEM;
> +		}

Above, priv->sram_pool will only be non-NULL if the pool has been
initialised.

> +err_cm3:
> +	if (!has_acpi_companion(&pdev->dev) && priv->cm3_base)
> +		gen_pool_free(priv->sram_pool, (unsigned long)priv->cm3_base,
> +			      MSS_SRAM_SIZE);

So wouldn't:
	if (priv->sram_pool && priv->cm3_base)

be more appropriate here?

> +	if (!has_acpi_companion(&pdev->dev) && priv->cm3_base) {
> +		gen_pool_free(priv->sram_pool, (unsigned long)priv->cm3_base,
> +			      MSS_SRAM_SIZE);
> +		gen_pool_destroy(priv->sram_pool);
> +	}

Same here.

Why is it correct to call gen_pool_destroy() in the remove path but not
the error path? I think you want to drop this - the pool is created and
destroyed by the SRAM driver, users of it should not be destroying it.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!