linux-kernel - Re: [PATCH RFC net-next 3/6] net: hibmcge: create a software node for phy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <543efb90-da56-4190-afa7-665d32303c8c@lunn.ch>
Date: Tue, 16 Dec 2025 08:17:00 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Jijie Shao <shaojijie@...wei.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
	Frank.Sae@...or-comm.com, hkallweit1@...il.com,
	linux@...linux.org.uk, shenjian15@...wei.com,
	liuyonglong@...wei.com, chenhao418@...wei.com,
	jonathan.cameron@...wei.com, salil.mehta@...wei.com,
	shiyongbang@...wei.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net-next 3/6] net: hibmcge: create a software node
 for phy_led

On Mon, Dec 15, 2025 at 08:57:02PM +0800, Jijie Shao wrote:
> Currently, phy_led supports of node, acpi node, and software node.
> The hibmcge hardware(including leds) is on the BMC side, while
> the driver runs on the host side.
> 
> An apic or of node needs to be created on the host side to
> support phy_led; otherwise, phy_led will not be supported.
> 
> Therefore, in order to support phy_led, this patch will create
> a software node when it detects that the phy device is not
> bound to any fwnode.
> 
> Closes: https://lore.kernel.org/all/023a85e4-87e2-4bd3-9727-69a2bfdc4145@lunn.ch/
> Signed-off-by: Jijie Shao <shaojijie@...wei.com>
> ---
>  .../ethernet/hisilicon/hibmcge/hbg_common.h   |  11 ++
>  .../net/ethernet/hisilicon/hibmcge/hbg_mdio.c | 120 +++++++++++++++++-
>  2 files changed, 124 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h b/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h
> index 8e134da3e217..d20dd324e129 100644
> --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h
> +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_common.h
> @@ -5,8 +5,10 @@
>  #define __HBG_COMMON_H
>  
>  #include <linux/ethtool.h>
> +#include <linux/fwnode.h>
>  #include <linux/netdevice.h>
>  #include <linux/pci.h>
> +#include <linux/property.h>
>  #include <net/page_pool/helpers.h>
>  #include "hbg_reg.h"
>  
> @@ -130,6 +132,9 @@ struct hbg_vector {
>  	u32 info_array_len;
>  };
>  
> +#define HBG_LED_MAX_NUM			(sizeof(u32) / sizeof(u8))
> +#define HBG_LED_SOFTWARE_NODE_MAX_NUM	(HBG_LED_MAX_NUM + 2)
> +
>  struct hbg_mac {
>  	struct mii_bus *mdio_bus;
>  	struct phy_device *phydev;
> @@ -140,6 +145,12 @@ struct hbg_mac {
>  	u32 autoneg;
>  	u32 link_status;
>  	u32 pause_autoneg;
> +
> +	struct software_node phy_node;
> +	struct software_node leds_node;
> +	struct software_node led_nodes[HBG_LED_MAX_NUM];
> +	struct property_entry leds_props[HBG_LED_MAX_NUM][4];
> +	const struct software_node *nodes[HBG_LED_SOFTWARE_NODE_MAX_NUM + 1];
>  };
>  
>  struct hbg_mac_table_entry {
> diff --git a/drivers/net/ethernet/hisilicon/hibmcge/hbg_mdio.c b/drivers/net/ethernet/hisilicon/hibmcge/hbg_mdio.c
> index b6f0a2780ea8..edd8ccefbb62 100644
> --- a/drivers/net/ethernet/hisilicon/hibmcge/hbg_mdio.c
> +++ b/drivers/net/ethernet/hisilicon/hibmcge/hbg_mdio.c
> @@ -263,11 +263,119 @@ static int hbg_fixed_phy_init(struct hbg_priv *priv)
>  	return hbg_phy_connect(priv);
>  }
>  
> -int hbg_mdio_init(struct hbg_priv *priv)
> +static void hbg_phy_device_free(void *data)
> +{
> +	phy_device_free((struct phy_device *)data);
> +}
> +
> +static void hbg_phy_device_remove(void *data)
> +{
> +	phy_device_remove((struct phy_device *)data);
> +}

Alarm bells are ringing! A MAC driver should not be doing anything
like this.

> +int hbg_mdio_init(struct hbg_priv *priv)
> +{
> +	struct device *dev = &priv->pdev->dev;
> +	struct hbg_mac *mac = &priv->mac;
>  	struct mii_bus *mdio_bus;
>  	int ret;
>  
> @@ -281,7 +389,7 @@ int hbg_mdio_init(struct hbg_priv *priv)
>  
>  	mdio_bus->parent = dev;
>  	mdio_bus->priv = priv;
> -	mdio_bus->phy_mask = ~(1 << mac->phy_addr);
> +	mdio_bus->phy_mask = 0xFFFFFFFF; /* not scan phy device */
>  	mdio_bus->name = "hibmcge mii bus";
>  	mac->mdio_bus = mdio_bus;

I think you are taking the wrong approach.

Please consider trying to use of_mdiobus_register(). Either load a DT
overlay, or see if you can construct a tree of properties as you have
been doing, and pass it to of_mdiobus_register(). You then don't need
to destroy and create PHY devices.

	Andrew