Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1716359 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 2/2] net: phy: Don't use drv when it is NULL in phy_attached_print Romain Perier <romain.perier@collabora.com> - 2017-08-21 13:50 +0200
    Re: [PATCH v2 2/2] net: phy: Don't use drv when it is NULL in  phy_attached_print Andrew Lunn <andrew@lunn.ch> - 2017-08-21 16:30 +0200
      Re: [PATCH v2 2/2] net: phy: Don't use drv when it is NULL in  phy_attached_print Florian Fainelli <f.fainelli@gmail.com> - 2017-08-22 02:50 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web