netdev - Re: [PATCH v5 4/9] net: stmmac: dwmac-loongson: Refactor code for loongson_dwmac

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]

Date: Sat, 11 Nov 2023 21:19:21 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Yanteng Si <siyanteng@...ngson.cn>
Cc: hkallweit1@...il.com, peppe.cavallaro@...com,
	alexandre.torgue@...s.st.com, joabreu@...opsys.com,
	fancer.lancer@...il.com, Jose.Abreu@...opsys.com,
	chenhuacai@...ngson.cn, linux@...linux.org.uk, dongbiao@...ngson.cn,
	guyinggang@...ngson.cn, netdev@...r.kernel.org,
	loongarch@...ts.linux.dev, chris.chenfeiyang@...il.com
Subject: Re: [PATCH v5 4/9] net: stmmac: dwmac-loongson: Refactor code for
 loongson_dwmac_probe()

On Fri, Nov 10, 2023 at 05:25:43PM +0800, Yanteng Si wrote:
> Add a setup() function to initialize data, and simplify code for
> loongson_dwmac_probe().

This does not look like a refactoring patch. Such patches just move
code around, but otherwise leave the code alone. There are real
changes in here.

> -	if (!of_device_is_compatible(np, "loongson, pci-gmac")) {
> -		pr_info("dwmac_loongson_pci: Incompatible OF node\n");
> -		return -ENODEV;
> -	}
> -

This just disappears. Why is it no longer needed?


>  	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
>  	if (!plat)
>  		return -ENOMEM;
>  
> +	plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> +					   sizeof(*plat->mdio_bus_data),
> +					   GFP_KERNEL);
> +	if (!plat->mdio_bus_data)
> +		return -ENOMEM;
> +
> +	plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg),
> +				     GFP_KERNEL);
> +	if (!plat->dma_cfg)
> +		return -ENOMEM;
> +
>  	plat->mdio_node = of_get_child_by_name(np, "mdio");
>  	if (plat->mdio_node) {
>  		dev_info(&pdev->dev, "Found MDIO subnode\n");
> -
> -		plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> -						   sizeof(*plat->mdio_bus_data),
> -						   GFP_KERNEL);
> -		if (!plat->mdio_bus_data) {
> -			ret = -ENOMEM;
> -			goto err_put_node;
> -		}

MDIO was conditional, but now is mandatory. Why? 

>  	if (ret) {
> -		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n", __func__);
> +		dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n",
> +			__func__);

Changes like this are a distraction. The reviewer is trying to
understand what has changed and why. If you want to make white space
changes, please do it in a patch of its own.

Please break this patch up into lots of smaller parts, each with a
good commit messaged explaining what is going on, and importantly,
why.

	Andrew