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

Groups > linux.kernel > #1716153 > unrolled thread

[PATCH 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 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 10:00 +0200
    Re: [PATCH 2/2] net: phy: Don't use drv when it is NULL in  phy_attached_print Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2017-08-21 11:50 +0200
      Re: [PATCH 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 2/2] net: phy: Don't use drv when it is NULL in  phy_attached_print Andrew Lunn <andrew@lunn.ch> - 2017-08-21 15:20 +0200

#1716153 — [PATCH 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
binded 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 binded
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: commit 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 9493fb369682..b38926bc275f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -877,15 +877,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]

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

Hello!

On 8/21/2017 10:52 AM, Romain Perier wrote:

> Currently, if this logging function is used prior the phy driver is
> binded to the phy device (that is usually done from .ndo_open),

    s/binded/bound/.

> '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 binded

    Likewise.

> 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: commit fbca164776e4 ("net: stmmac: Use the right logging functi")

    "Commit" not needed here.

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

MBR, Sergei

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

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

Hello,


Le 21/08/2017 à 11:45, Sergei Shtylyov a écrit :
> Hello!
>
> On 8/21/2017 10:52 AM, Romain Perier wrote:
>
>> Currently, if this logging function is used prior the phy driver is
>> binded to the phy device (that is usually done from .ndo_open),
>
>    s/binded/bound/.
>
>> '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
>> binded
>
>    Likewise.
>
>> 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: commit fbca164776e4 ("net: stmmac: Use the right logging functi")
>
>    "Commit" not needed here.

Fixed, thanks

Romain

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

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

On Mon, Aug 21, 2017 at 09:52:35AM +0200, Romain Perier wrote:
> Currently, if this logging function is used prior the phy driver is
> binded 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 binded
> 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: commit 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 9493fb369682..b38926bc275f 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -877,15 +877,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;
> +	}

Hi Romain

I don't like this. You end up with the same code twice. c45 does not
imply 10g, so i would not be surprised if sometime in the future this
changes. And then we have two places we need to make the same change.

I also wonder what happens if you load the PHY driver later, but
before it is bound. Will it then use the correct driver?


> +
>  	if (!fmt) {
>  		dev_info(&phydev->mdio.dev, ATTACHED_FMT "\n",
> -			 phydev->drv->name, phydev_name(phydev),
> +			 drv->name, phydev_name(phydev),

I would prefer (phydev->drv ? phydev->drv->name, "unknown")

  Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web