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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210124122809.GV1551@shell.armlinux.org.uk>
Date:   Sun, 24 Jan 2021 12:28:10 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     stefanc@...vell.com
Cc:     netdev@...r.kernel.org, thomas.petazzoni@...tlin.com,
        davem@...emloft.net, nadavh@...vell.com, ymarkman@...vell.com,
        linux-kernel@...r.kernel.org, kuba@...nel.org, mw@...ihalf.com,
        andrew@...n.ch, atenart@...nel.org
Subject: 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@...vell.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!