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

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!