netdev - Re: [PATCH net-next v9 6/6] net: tn40xx: add phylink support

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: <ZmWFNATfPWEPSLyf@shell.armlinux.org.uk>
Date: Sun, 9 Jun 2024 11:34:28 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, andrew@...n.ch, horms@...nel.org,
	kuba@...nel.org, jiri@...nulli.us, pabeni@...hat.com,
	hfdevel@....net, naveenm@...vell.com, jdamato@...tly.com
Subject: Re: [PATCH net-next v9 6/6] net: tn40xx: add phylink support

On Thu, Jun 06, 2024 at 08:26:08AM +0900, FUJITA Tomonori wrote:
> @@ -1670,6 +1681,12 @@ static int tn40_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto err_unset_drvdata;
>  	}
>  
> +	ret = tn40_mdiobus_init(priv);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to initialize mdio bus.\n");
> +		goto err_free_irq;
> +	}
> +
...
> +err_unregister_phydev:
> +	tn40_phy_unregister(priv);
>  err_free_irq:
>  	pci_free_irq_vectors(pdev);
>  err_unset_drvdata:

and from previous patches:

+       pci_set_drvdata(pdev, NULL);
+err_iounmap:
+       iounmap(regs);
+err_free_regions:
+       pci_release_regions(pdev);
+err_disable_device:
+       pci_disable_device(pdev);
+       return ret;
+}

So, if tn40_mdiobus_init() returns non-zero, this value will be returned
to higher kernel levels via tn40_probe().

...
> +int tn40_phy_register(struct tn40_priv *priv)
> +{
> +	struct phylink_config *config;
> +	struct phy_device *phydev;
> +	struct phylink *phylink;
> +
> +	phydev = phy_find_first(priv->mdio);
> +	if (!phydev) {
> +		dev_err(&priv->pdev->dev, "PHY isn't found\n");
> +		return -1;

And my email client, setup with rules to catch common programming
mistakes, highlights the above line. I have no idea why people do
this... why people think "lets return -1 on error". It seems to be
a very common pattern... but it's utterly wrong. -1 is -EPERM, aka
"Operation not permitted". This is not what you mean here. Please
return a more suitable negative errno symbol... and please refrain
from using "return -1" in kernel code.

(The only case where "return -1" may be permissible is where the
value doesn't get propagated outside of the compilation unit, but
even there, there is the possibility that later changes may end
up propagating it outside... personally, I would like to see
"return -1" totally banned from the kernel.)

Thanks.

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