[PATCH v2] net: stmmac: Use the right logging function in stmmac_mdio_register

[PATCH v2] net: stmmac: Use the right logging function in stmmac_mdio_register

[Romain Perier]

Currently, the function stmmac_mdio_register() is only used by
stmmac_dvr_probe() from stmmac_main.c, in order to register the MDIO bus
and probe information about the PHY. As this function is called before
calling register_netdev(), all messages logged from stmmac_mdio_register
are prefixed by "(unnamed net_device)". The goal of netdev_info or
netdev_err is to dump useful infos about a net_device, when this data
structure is partially initialized, there is no point for using these
functions.

This commit fixes the issue by replacing all netdev_*() by the
corresponding dev_*() function for logging. The last netdev_info is
replaced by phy_attached_info(), as a valid phydev can be used at this
point.

Signed-off-by: Romain Perier <romain.perier@collabora.com>
---

Changes in v2:
- Replaced dev_info() by phy_attached_print()
- Simplified message because informations were redudant
- Updated commit message

 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index db157a47000c..e0fbf8657103 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -204,6 +204,7 @@ int stmmac_mdio_register(struct net_device *ndev)
 	struct stmmac_priv *priv = netdev_priv(ndev);
 	struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data;
 	struct device_node *mdio_node = priv->plat->mdio_node;
+	struct device *dev = ndev->dev.parent;
 	int addr, found;
 
 	if (!mdio_bus_data)
@@ -237,7 +238,7 @@ int stmmac_mdio_register(struct net_device *ndev)
 	else
 		err = mdiobus_register(new_bus);
 	if (err != 0) {
-		netdev_err(ndev, "Cannot register the MDIO bus\n");
+		dev_err(dev, "Cannot register the MDIO bus\n");
 		goto bus_register_fail;
 	}
 
@@ -285,14 +286,14 @@ int stmmac_mdio_register(struct net_device *ndev)
 			irq_str = irq_num;
 			break;
 		}
-		netdev_info(ndev, "PHY ID %08x at %d IRQ %s (%s)%s\n",
-			    phydev->phy_id, addr, irq_str, phydev_name(phydev),
-			    act ? " active" : "");
+		phy_attached_print(phydev, "PHY ID %08x IRQ %s %s\n",
+				   phydev->phy_id, irq_str,
+				   act ? " active" : "");
 		found = 1;
 	}
 
 	if (!found && !mdio_node) {
-		netdev_warn(ndev, "No PHY found\n");
+		dev_warn(dev, "No PHY found\n");
 		mdiobus_unregister(new_bus);
 		mdiobus_free(new_bus);
 		return -ENODEV;
-- 
2.11.0

Re: [PATCH v2] net: stmmac: Use the right logging function in stmmac_mdio_register

[Andrew Lunn]

On Thu, Aug 10, 2017 at 09:38:26AM +0200, Romain Perier wrote:
> Currently, the function stmmac_mdio_register() is only used by
> stmmac_dvr_probe() from stmmac_main.c, in order to register the MDIO bus
> and probe information about the PHY. As this function is called before
> calling register_netdev(), all messages logged from stmmac_mdio_register
> are prefixed by "(unnamed net_device)". The goal of netdev_info or
> netdev_err is to dump useful infos about a net_device, when this data
> structure is partially initialized, there is no point for using these
> functions.
> 
> This commit fixes the issue by replacing all netdev_*() by the
> corresponding dev_*() function for logging. The last netdev_info is
> replaced by phy_attached_info(), as a valid phydev can be used at this
> point.
> 
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
> 
> Changes in v2:
> - Replaced dev_info() by phy_attached_print()
> - Simplified message because informations were redudant
> - Updated commit message
> 
>  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index db157a47000c..e0fbf8657103 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -204,6 +204,7 @@ int stmmac_mdio_register(struct net_device *ndev)
>  	struct stmmac_priv *priv = netdev_priv(ndev);
>  	struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data;
>  	struct device_node *mdio_node = priv->plat->mdio_node;
> +	struct device *dev = ndev->dev.parent;
>  	int addr, found;
>  
>  	if (!mdio_bus_data)
> @@ -237,7 +238,7 @@ int stmmac_mdio_register(struct net_device *ndev)
>  	else
>  		err = mdiobus_register(new_bus);
>  	if (err != 0) {
> -		netdev_err(ndev, "Cannot register the MDIO bus\n");
> +		dev_err(dev, "Cannot register the MDIO bus\n");
>  		goto bus_register_fail;
>  	}
>  
> @@ -285,14 +286,14 @@ int stmmac_mdio_register(struct net_device *ndev)
>  			irq_str = irq_num;
>  			break;
>  		}
> -		netdev_info(ndev, "PHY ID %08x at %d IRQ %s (%s)%s\n",
> -			    phydev->phy_id, addr, irq_str, phydev_name(phydev),
> -			    act ? " active" : "");
> +		phy_attached_print(phydev, "PHY ID %08x IRQ %s %s\n",
> +				   phydev->phy_id, irq_str,
> +				   act ? " active" : "");

I don't want to go round and round with this patch, with feature creep. 

But there are only two drivers which use phy_attached_print() to print
more than the standard printed by phy_attached_info().
phy_attached_info() will give you the driver name, so do you need the
ID? phy_attached_info() prints the irq as a number, rather than a nice
string.

act is just odd. The loop is effectively phy_find_first(). The act
means it has found a phy, but not the phy indicated in the platform
data, but it is going to use it anyway!

So i will give:

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

but if you think phy_attached_info() is acceptable, that would be
great.

    Andrew

Re: [PATCH v2] net: stmmac: Use the right logging function in stmmac_mdio_register

[Romain Perier]

Hi Andrew,


Le 10/08/2017 à 15:56, Andrew Lunn a écrit :
> On Thu, Aug 10, 2017 at 09:38:26AM +0200, Romain Perier wrote:
>> Currently, the function stmmac_mdio_register() is only used by
>> stmmac_dvr_probe() from stmmac_main.c, in order to register the MDIO bus
>> and probe information about the PHY. As this function is called before
>> calling register_netdev(), all messages logged from stmmac_mdio_register
>> are prefixed by "(unnamed net_device)". The goal of netdev_info or
>> netdev_err is to dump useful infos about a net_device, when this data
>> structure is partially initialized, there is no point for using these
>> functions.
>>
>> This commit fixes the issue by replacing all netdev_*() by the
>> corresponding dev_*() function for logging. The last netdev_info is
>> replaced by phy_attached_info(), as a valid phydev can be used at this
>> point.
>>
>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>> ---
>>
>> Changes in v2:
>> - Replaced dev_info() by phy_attached_print()
>> - Simplified message because informations were redudant
>> - Updated commit message
>>
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>> index db157a47000c..e0fbf8657103 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>> @@ -204,6 +204,7 @@ int stmmac_mdio_register(struct net_device *ndev)
>>  	struct stmmac_priv *priv = netdev_priv(ndev);
>>  	struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data;
>>  	struct device_node *mdio_node = priv->plat->mdio_node;
>> +	struct device *dev = ndev->dev.parent;
>>  	int addr, found;
>>  
>>  	if (!mdio_bus_data)
>> @@ -237,7 +238,7 @@ int stmmac_mdio_register(struct net_device *ndev)
>>  	else
>>  		err = mdiobus_register(new_bus);
>>  	if (err != 0) {
>> -		netdev_err(ndev, "Cannot register the MDIO bus\n");
>> +		dev_err(dev, "Cannot register the MDIO bus\n");
>>  		goto bus_register_fail;
>>  	}
>>  
>> @@ -285,14 +286,14 @@ int stmmac_mdio_register(struct net_device *ndev)
>>  			irq_str = irq_num;
>>  			break;
>>  		}
>> -		netdev_info(ndev, "PHY ID %08x at %d IRQ %s (%s)%s\n",
>> -			    phydev->phy_id, addr, irq_str, phydev_name(phydev),
>> -			    act ? " active" : "");
>> +		phy_attached_print(phydev, "PHY ID %08x IRQ %s %s\n",
>> +				   phydev->phy_id, irq_str,
>> +				   act ? " active" : "");
> I don't want to go round and round with this patch, with feature creep. 
>
> But there are only two drivers which use phy_attached_print() to print
> more than the standard printed by phy_attached_info().
> phy_attached_info() will give you the driver name, so do you need the
> ID? phy_attached_info() prints the irq as a number, rather than a nice
> string.
>
> act is just odd. The loop is effectively phy_find_first(). The act
> means it has found a phy, but not the phy indicated in the platform
> data, but it is going to use it anyway!
>
> So i will give:
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
> but if you think phy_attached_info() is acceptable, that would be
> great.
>
>     Andrew

Good point, initially I just wanted to propose a fix about logging and
not change the message, in order to avoid to lose informations (but
that's very arguable).
phy_attached_print or phy_attached_info already display the model of the
PHY, so I think that removing PHY ID is okay, because in this case it is
completely useless.

I will post a v3 with your Reviewed-by:

Romain