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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!