[PATCH v2] net: phy: Fix return value when !CONFIG_PHYLIB

[PATCH v2] net: phy: Fix return value when !CONFIG_PHYLIB

[hhtracer]

From: huhai <huhai@kylinos.cn>

Many call sites of get_phy_device() and fwnode_get_phy_node(), such as
sfp_sm_probe_phy(), phylink_fwnode_phy_connect(), etc., rely on IS_ERR()
to check for errors in the returned pointer.

Furthermore, the implementations of get_phy_device() and
fwnode_get_phy_node() themselves use ERR_PTR() to return error codes.

Therefore, when CONFIG_PHYLIB is disabled, returning NULL is incorrect,
as this would bypass IS_ERR() checks and may lead to NULL pointer
dereference.

Returning ERR_PTR(-ENXIO) is the correct and consistent way to indicate
that PHY support is not available, and it avoids such issues.

Signed-off-by: huhai <huhai@kylinos.cn>
---
 include/linux/phy.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index a2bfae80c449..be299c572d73 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1787,13 +1787,13 @@ static inline struct phy_device *device_phy_find_device(struct device *dev)
 static inline
 struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
 {
-	return NULL;
+	return ERR_PTR(-ENXIO);
 }
 
 static inline
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 {
-	return NULL;
+	return ERR_PTR(-ENXIO);
 }
 
 static inline int phy_device_register(struct phy_device *phy)
-- 
2.25.1

Re: [PATCH v2] net: phy: Fix return value when !CONFIG_PHYLIB

[Heiner Kallweit]

On 13.04.2025 15:37, hhtracer@gmail.com wrote:
> From: huhai <huhai@kylinos.cn>
> 
> Many call sites of get_phy_device() and fwnode_get_phy_node(), such as
> sfp_sm_probe_phy(), phylink_fwnode_phy_connect(), etc., rely on IS_ERR()
> to check for errors in the returned pointer.
> 
> Furthermore, the implementations of get_phy_device() and
> fwnode_get_phy_node() themselves use ERR_PTR() to return error codes.
> 
> Therefore, when CONFIG_PHYLIB is disabled, returning NULL is incorrect,
> as this would bypass IS_ERR() checks and may lead to NULL pointer
> dereference.
> 
Is there actually any call site which doesn't select PHYLIB directly or
indirectly? When briefly checking I didn't find one.
So my question would be rather: Do we need/want stubs for the following
functions at all?

fwnode_get_phy_id
fwnode_mdio_find_device
fwnode_phy_find_device
device_phy_find_device
fwnode_get_phy_node
get_phy_device
phy_device_register


And a formal remark:
Your v2 has no change log, and please allow 24h before sending a new version.

> Returning ERR_PTR(-ENXIO) is the correct and consistent way to indicate
> that PHY support is not available, and it avoids such issues.
> 
> Signed-off-by: huhai <huhai@kylinos.cn>
> ---
>  include/linux/phy.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index a2bfae80c449..be299c572d73 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1787,13 +1787,13 @@ static inline struct phy_device *device_phy_find_device(struct device *dev)
>  static inline
>  struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
>  {
> -	return NULL;
> +	return ERR_PTR(-ENXIO);
>  }
>  
>  static inline
>  struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
>  {
> -	return NULL;
> +	return ERR_PTR(-ENXIO);
>  }
>  
>  static inline int phy_device_register(struct phy_device *phy)

Re: [PATCH v2] net: phy: Fix return value when !CONFIG_PHYLIB

[Russell King (Oracle)]

On Sun, Apr 13, 2025 at 09:37:09PM +0800, hhtracer@gmail.com wrote:
> Many call sites of get_phy_device() and fwnode_get_phy_node(), such as
> sfp_sm_probe_phy(), phylink_fwnode_phy_connect(), etc., rely on IS_ERR()
> to check for errors in the returned pointer.
> 
> Furthermore, the implementations of get_phy_device() and
> fwnode_get_phy_node() themselves use ERR_PTR() to return error codes.
> 
> Therefore, when CONFIG_PHYLIB is disabled, returning NULL is incorrect,
> as this would bypass IS_ERR() checks and may lead to NULL pointer
> dereference.
> 
> Returning ERR_PTR(-ENXIO) is the correct and consistent way to indicate
> that PHY support is not available, and it avoids such issues.

I wonder whether it's crossed one's mind that returning NULL may be
intentional to avoid erroring out at the callsites, and returning
an error may cause runtime failures.

You need to do way more investigation before posting a patch like this:

- Analyse each call site.
- Determine whether that call site can exist if PHYLIB is not built.
- Determine whether returning an error in that case instead of NULL
  may be detrimental, or at the very least list the call sites that
  would be affected by the change.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2] net: phy: Fix return value when !CONFIG_PHYLIB

[胡海]

Russell King (Oracle) <linux@armlinux.org.uk> 于2025年4月13日周日 22:13写道：
>
> On Sun, Apr 13, 2025 at 09:37:09PM +0800, hhtracer@gmail.com wrote:
> > Many call sites of get_phy_device() and fwnode_get_phy_node(), such as
> > sfp_sm_probe_phy(), phylink_fwnode_phy_connect(), etc., rely on IS_ERR()
> > to check for errors in the returned pointer.
> >
> > Furthermore, the implementations of get_phy_device() and
> > fwnode_get_phy_node() themselves use ERR_PTR() to return error codes.
> >
> > Therefore, when CONFIG_PHYLIB is disabled, returning NULL is incorrect,
> > as this would bypass IS_ERR() checks and may lead to NULL pointer
> > dereference.
> >
> > Returning ERR_PTR(-ENXIO) is the correct and consistent way to indicate
> > that PHY support is not available, and it avoids such issues.
>
> I wonder whether it's crossed one's mind that returning NULL may be
> intentional to avoid erroring out at the callsites, and returning
> an error may cause runtime failures.
>
> You need to do way more investigation before posting a patch like this:
>
> - Analyse each call site.
> - Determine whether that call site can exist if PHYLIB is not built.
> - Determine whether returning an error in that case instead of NULL
>   may be detrimental, or at the very least list the call sites that
>   would be affected by the change.

Yes, when PHYLIB is not built, the relevant call sites are not
compiled in, so they won't exist.

Thanks for the clarification!

>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!