[PATCH v2 0/2] net: stmmac: phy logging fixes

[PATCH v2 0/2] net: stmmac: phy logging fixes

[Romain Perier]

This set of patches fixes issues related to logging. First it delete old
code that is no longer used in stmmac_mdio_register(). Then, it fixes a
crash that happens when phydev->drv is NULL and phy_attached_info() is
called prior phy_driver is binded to phydev.

Changes in v2:
- Fixed wording in commit message
- "commit" not needed in the "Fixes" tag

Romain Perier (2):
  net: stmmac: Delete dead code for MDIO registration
  net: phy: Don't use drv when it is NULL in phy_attached_print

 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 16 ----------------
 drivers/net/phy/phy_device.c                      | 13 +++++++++++--
 2 files changed, 11 insertions(+), 18 deletions(-)

-- 
2.11.0

[PATCH v2 1/2] net: stmmac: Delete dead code for MDIO registration

[Romain Perier]

This code is no longer used, the logging function was changed by commit
fbca164776e4 ("net: stmmac: Use the right logging functi").

Fixes: fbca164776e4 ("net: stmmac: Use the right logging functi")
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 72ec711fcba2..f5f37bfa1d58 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -248,9 +248,6 @@ int stmmac_mdio_register(struct net_device *ndev)
 	found = 0;
 	for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
 		struct phy_device *phydev = mdiobus_get_phy(new_bus, addr);
-		int act = 0;
-		char irq_num[4];
-		char *irq_str;
 
 		if (!phydev)
 			continue;
@@ -273,19 +270,6 @@ int stmmac_mdio_register(struct net_device *ndev)
 		if (priv->plat->phy_addr == -1)
 			priv->plat->phy_addr = addr;
 
-		act = (priv->plat->phy_addr == addr);
-		switch (phydev->irq) {
-		case PHY_POLL:
-			irq_str = "POLL";
-			break;
-		case PHY_IGNORE_INTERRUPT:
-			irq_str = "IGNORE";
-			break;
-		default:
-			sprintf(irq_num, "%d", phydev->irq);
-			irq_str = irq_num;
-			break;
-		}
 		phy_attached_info(phydev);
 		found = 1;
 	}
-- 
2.11.0

[PATCH v2 2/2] net: phy: Don't use drv when it is NULL in phy_attached_print

[Romain Perier]

Currently, if this logging function is used prior the phy driver is
bound to the phy device (that is usually done from .ndo_open),
'phydev->drv' might be NULL, resulting in a kernel crash. That is
typically the case in the stmmac driver, info about the phy is displayed
during the registration of the MDIO bus, and then genphy driver is bound
to this phydev when .ndo_open is called.

This commit fixes the issue by using the right genphy driver, when
phydev->drv is NULL.

Fixes: fbca164776e4 ("net: stmmac: Use the right logging functi")
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/net/phy/phy_device.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1790f7fec125..6af6dc6dfeaf 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -864,15 +864,24 @@ EXPORT_SYMBOL(phy_attached_info);
 #define ATTACHED_FMT "attached PHY driver [%s] (mii_bus:phy_addr=%s, irq=%d)"
 void phy_attached_print(struct phy_device *phydev, const char *fmt, ...)
 {
+	struct phy_driver *drv = phydev->drv;
+
+	if (!drv) {
+		if (phydev->is_c45)
+			drv = &genphy_10g_driver;
+		else
+			drv = &genphy_driver;
+	}
+
 	if (!fmt) {
 		dev_info(&phydev->mdio.dev, ATTACHED_FMT "\n",
-			 phydev->drv->name, phydev_name(phydev),
+			 drv->name, phydev_name(phydev),
 			 phydev->irq);
 	} else {
 		va_list ap;
 
 		dev_info(&phydev->mdio.dev, ATTACHED_FMT,
-			 phydev->drv->name, phydev_name(phydev),
+			 drv->name, phydev_name(phydev),
 			 phydev->irq);
 
 		va_start(ap, fmt);
-- 
2.11.0

Re: [PATCH v2 2/2] net: phy: Don't use drv when it is NULL in phy_attached_print

[Andrew Lunn]

On Mon, Aug 21, 2017 at 01:45:30PM +0200, Romain Perier wrote:
> Currently, if this logging function is used prior the phy driver is
> bound to the phy device (that is usually done from .ndo_open),
> 'phydev->drv' might be NULL, resulting in a kernel crash. That is
> typically the case in the stmmac driver, info about the phy is displayed
> during the registration of the MDIO bus, and then genphy driver is bound
> to this phydev when .ndo_open is called.
> 
> This commit fixes the issue by using the right genphy driver, when
> phydev->drv is NULL.
> 
> Fixes: fbca164776e4 ("net: stmmac: Use the right logging functi")
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
>  drivers/net/phy/phy_device.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 1790f7fec125..6af6dc6dfeaf 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -864,15 +864,24 @@ EXPORT_SYMBOL(phy_attached_info);
>  #define ATTACHED_FMT "attached PHY driver [%s] (mii_bus:phy_addr=%s, irq=%d)"
>  void phy_attached_print(struct phy_device *phydev, const char *fmt, ...)
>  {
> +	struct phy_driver *drv = phydev->drv;
> +
> +	if (!drv) {
> +		if (phydev->is_c45)
> +			drv = &genphy_10g_driver;
> +		else
> +			drv = &genphy_driver;
> +	}
> +

As i said in my comment to v1, i don't like this.

   Andrew

Re: [PATCH v2 1/2] net: stmmac: Delete dead code for MDIO registration

[Florian Fainelli]

On 08/21/2017 04:45 AM, Romain Perier wrote:
> This code is no longer used, the logging function was changed by commit
> fbca164776e4 ("net: stmmac: Use the right logging functi").
> 
> Fixes: fbca164776e4 ("net: stmmac: Use the right logging functi")
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index 72ec711fcba2..f5f37bfa1d58 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -248,9 +248,6 @@ int stmmac_mdio_register(struct net_device *ndev)
>  	found = 0;
>  	for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
>  		struct phy_device *phydev = mdiobus_get_phy(new_bus, addr);
> -		int act = 0;
> -		char irq_num[4];
> -		char *irq_str;
>  
>  		if (!phydev)
>  			continue;
> @@ -273,19 +270,6 @@ int stmmac_mdio_register(struct net_device *ndev)
>  		if (priv->plat->phy_addr == -1)
>  			priv->plat->phy_addr = addr;
>  
> -		act = (priv->plat->phy_addr == addr);
> -		switch (phydev->irq) {
> -		case PHY_POLL:
> -			irq_str = "POLL";
> -			break;
> -		case PHY_IGNORE_INTERRUPT:
> -			irq_str = "IGNORE";
> -			break;
> -		default:
> -			sprintf(irq_num, "%d", phydev->irq);
> -			irq_str = irq_num;
> -			break;
> -		}

I was actually just looking into moving these prints to
phy_attached_info(), since it is useful to know whether the interrupt is
POLL, IGNORE, or valid. Can you move that there? Then you can really
migrate over phy_attached_info() with no information loss.

Thanks!
-- 
Florian

Re: [PATCH v2 2/2] net: phy: Don't use drv when it is NULL in phy_attached_print

[Florian Fainelli]

On 08/21/2017 07:24 AM, Andrew Lunn wrote:
> On Mon, Aug 21, 2017 at 01:45:30PM +0200, Romain Perier wrote:
>> Currently, if this logging function is used prior the phy driver is
>> bound to the phy device (that is usually done from .ndo_open),
>> 'phydev->drv' might be NULL, resulting in a kernel crash. That is
>> typically the case in the stmmac driver, info about the phy is displayed
>> during the registration of the MDIO bus, and then genphy driver is bound
>> to this phydev when .ndo_open is called.
>>
>> This commit fixes the issue by using the right genphy driver, when
>> phydev->drv is NULL.
>>
>> Fixes: fbca164776e4 ("net: stmmac: Use the right logging functi")
>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>> ---
>>  drivers/net/phy/phy_device.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 1790f7fec125..6af6dc6dfeaf 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -864,15 +864,24 @@ EXPORT_SYMBOL(phy_attached_info);
>>  #define ATTACHED_FMT "attached PHY driver [%s] (mii_bus:phy_addr=%s, irq=%d)"
>>  void phy_attached_print(struct phy_device *phydev, const char *fmt, ...)
>>  {
>> +	struct phy_driver *drv = phydev->drv;
>> +
>> +	if (!drv) {
>> +		if (phydev->is_c45)
>> +			drv = &genphy_10g_driver;
>> +		else
>> +			drv = &genphy_driver;
>> +	}
>> +
> 
> As i said in my comment to v1, i don't like this.

Agreed, just use Andrew's earlier suggestion of checking phydev->drv
validity.

We don't have an equivalent of "unregistered" in the PHY layer, but
"unbound" seems like it could be what we want here.

Thanks
-- 
Florian

Re: [PATCH v2 1/2] net: stmmac: Delete dead code for MDIO registration

[Romain Perier]

Hello,


Le 22/08/2017 à 02:45, Florian Fainelli a écrit :
> On 08/21/2017 04:45 AM, Romain Perier wrote:
>> This code is no longer used, the logging function was changed by commit
>> fbca164776e4 ("net: stmmac: Use the right logging functi").
>>
>> Fixes: fbca164776e4 ("net: stmmac: Use the right logging functi")
>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 16 ----------------
>>  1 file changed, 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>> index 72ec711fcba2..f5f37bfa1d58 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
>> @@ -248,9 +248,6 @@ int stmmac_mdio_register(struct net_device *ndev)
>>  	found = 0;
>>  	for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
>>  		struct phy_device *phydev = mdiobus_get_phy(new_bus, addr);
>> -		int act = 0;
>> -		char irq_num[4];
>> -		char *irq_str;
>>  
>>  		if (!phydev)
>>  			continue;
>> @@ -273,19 +270,6 @@ int stmmac_mdio_register(struct net_device *ndev)
>>  		if (priv->plat->phy_addr == -1)
>>  			priv->plat->phy_addr = addr;
>>  
>> -		act = (priv->plat->phy_addr == addr);
>> -		switch (phydev->irq) {
>> -		case PHY_POLL:
>> -			irq_str = "POLL";
>> -			break;
>> -		case PHY_IGNORE_INTERRUPT:
>> -			irq_str = "IGNORE";
>> -			break;
>> -		default:
>> -			sprintf(irq_num, "%d", phydev->irq);
>> -			irq_str = irq_num;
>> -			break;
>> -		}
> I was actually just looking into moving these prints to
> phy_attached_info(), since it is useful to know whether the interrupt is
> POLL, IGNORE, or valid. Can you move that there? Then you can really
> migrate over phy_attached_info() with no information loss.
>
> Thanks!
Sure, I can take a look.

Thanks,
Romain