[PATCH 1/3] net: stmmac: dwmac-qcom-ethqos: Return device_get_phy_mode() errors properly

[PATCH 1/3] net: stmmac: dwmac-qcom-ethqos: Return device_get_phy_mode() errors properly

[Andrew Halaney]

Other than -ENODEV, other errors resulted in -EINVAL being returned
instead of the actual error.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index e62940414e54..3bf025e8e2bd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -721,6 +721,9 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ethqos->phy_mode = device_get_phy_mode(dev);
+	if (ethqos->phy_mode < 0)
+		return dev_err_probe(dev, ethqos->phy_mode,
+				     "Failed to get phy mode\n");
 	switch (ethqos->phy_mode) {
 	case PHY_INTERFACE_MODE_RGMII:
 	case PHY_INTERFACE_MODE_RGMII_ID:
@@ -731,8 +734,6 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 	case PHY_INTERFACE_MODE_SGMII:
 		ethqos->configure_func = ethqos_configure_sgmii;
 		break;
-	case -ENODEV:
-		return -ENODEV;
 	default:
 		return -EINVAL;
 	}
-- 
2.41.0

[PATCH 2/3] net: stmmac: dwmac-qcom-ethqos: Use dev_err_probe()

[Andrew Halaney]

Using dev_err_probe() logs to devices_deferred which is helpful
when debugging.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 3bf025e8e2bd..a40869b2dd64 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -710,8 +710,8 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 
 	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
 	if (IS_ERR(plat_dat)) {
-		dev_err(dev, "dt configuration failed\n");
-		return PTR_ERR(plat_dat);
+		return dev_err_probe(dev, PTR_ERR(plat_dat),
+				     "dt configuration failed\n");
 	}
 
 	plat_dat->clks_config = ethqos_clks_config;
-- 
2.41.0

Re: [PATCH 2/3] net: stmmac: dwmac-qcom-ethqos: Use dev_err_probe()

[Andrew Lunn]

On Thu, Jun 29, 2023 at 02:14:17PM -0500, Andrew Halaney wrote:
> Using dev_err_probe() logs to devices_deferred which is helpful
> when debugging.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH 3/3] net: stmmac: dwmac-qcom-ethqos: Log more errors in probe

[Andrew Halaney]

These are useful to see when debugging a probe failure.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index a40869b2dd64..d792b7dd9fc3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -706,7 +706,8 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 
 	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
 	if (ret)
-		return ret;
+		return dev_err_probe(dev, ret,
+				     "Failed to get platform resources\n");
 
 	plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
 	if (IS_ERR(plat_dat)) {
@@ -735,13 +736,16 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 		ethqos->configure_func = ethqos_configure_sgmii;
 		break;
 	default:
+		dev_err(dev, "Unsupported phy mode %s\n",
+			phy_modes(ethqos->phy_mode));
 		return -EINVAL;
 	}
 
 	ethqos->pdev = pdev;
 	ethqos->rgmii_base = devm_platform_ioremap_resource_byname(pdev, "rgmii");
 	if (IS_ERR(ethqos->rgmii_base))
-		return PTR_ERR(ethqos->rgmii_base);
+		return dev_err_probe(dev, PTR_ERR(ethqos->rgmii_base),
+				     "Failed to map rgmii resource\n");
 
 	ethqos->mac_base = stmmac_res.addr;
 
@@ -753,7 +757,8 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 
 	ethqos->link_clk = devm_clk_get(dev, data->link_clk_name ?: "rgmii");
 	if (IS_ERR(ethqos->link_clk))
-		return PTR_ERR(ethqos->link_clk);
+		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),
+				     "Failed to get link_clk\n");
 
 	ret = ethqos_clks_config(ethqos, true);
 	if (ret)
@@ -765,7 +770,8 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 
 	ethqos->serdes_phy = devm_phy_optional_get(dev, "serdes");
 	if (IS_ERR(ethqos->serdes_phy))
-		return PTR_ERR(ethqos->serdes_phy);
+		return dev_err_probe(dev, PTR_ERR(ethqos->serdes_phy),
+				     "Failed to get serdes phy\n");
 
 	ethqos->speed = SPEED_1000;
 	ethqos_update_link_clk(ethqos, SPEED_1000);
-- 
2.41.0

Re: [PATCH 3/3] net: stmmac: dwmac-qcom-ethqos: Log more errors in probe

[Andrew Lunn]

On Thu, Jun 29, 2023 at 02:14:18PM -0500, Andrew Halaney wrote:
> These are useful to see when debugging a probe failure.

Since this is used for debugging, maybe netdev_dbg(). Anybody actually
doing debugging should be able to turn that on.

      Andrew

Re: [PATCH 3/3] net: stmmac: dwmac-qcom-ethqos: Log more errors in probe

[Andrew Halaney]

On Thu, Jun 29, 2023 at 10:32:24PM +0200, Andrew Lunn wrote:
> On Thu, Jun 29, 2023 at 02:14:18PM -0500, Andrew Halaney wrote:
> > These are useful to see when debugging a probe failure.
> 
> Since this is used for debugging, maybe netdev_dbg(). Anybody actually
> doing debugging should be able to turn that on.
> 

In my opinion it is better to use dev_err_probe() as done here because:

1. If it's -EPROBE_DEFER it will be under debug level already
2. If it's anything else, its an error and the logs are useful

I've ran into both ends of this now (failure of a platform dependency to
load, be it a bug in the driver, or just failing to select said driver),
and I've seen issues where new integrators (say you're bringing up a new
board) leave something out, etc, and run into issues because of that.

Thanks,
Andrew

Re: [PATCH 1/3] net: stmmac: dwmac-qcom-ethqos: Return device_get_phy_mode() errors properly

[Simon Horman]

On Thu, Jun 29, 2023 at 02:14:16PM -0500, Andrew Halaney wrote:
> Other than -ENODEV, other errors resulted in -EINVAL being returned
> instead of the actual error.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>

Hi Andrew,

I'm assuming this series is targeted at 'net-next', as opposed to 'net',
which is for fixes. In any case, the target tree should be included in the
subject.

        Subject: [PATCH net-next v2 1/3] ...

If it is for net-next, then please repost when net-next reopens after July 10th.

Link: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle

Also, it usually best to provide a cover-letter for patch-sets with more than
once patch.

--
pw-bot: deferred

Re: [PATCH 1/3] net: stmmac: dwmac-qcom-ethqos: Return device_get_phy_mode() errors properly

[Andrew Lunn]

On Thu, Jun 29, 2023 at 02:14:16PM -0500, Andrew Halaney wrote:
> Other than -ENODEV, other errors resulted in -EINVAL being returned
> instead of the actual error.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index e62940414e54..3bf025e8e2bd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -721,6 +721,9 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	ethqos->phy_mode = device_get_phy_mode(dev);
> +	if (ethqos->phy_mode < 0)
> +		return dev_err_probe(dev, ethqos->phy_mode,
> +				     "Failed to get phy mode\n");

If this every used on anything other than device tree?

of_get_phy_mode() has a better API, there is a clear separation of the
return value indicating success/fail, and the interface mode found in
DT. You can then change phy_mode in struct qcom_ethqos to be
phy_interface_t.

	Andrew