public inbox for netdev@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support
@ 2026-04-20 13:45 Lucien.Jheng
  2026-04-20 14:25 ` Bjørn Mork
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Lucien.Jheng @ 2026-04-20 13:45 UTC (permalink / raw)
  To: andrew, hkallweit1, linux, davem, edumazet, kuba, pabeni, netdev,
	linux-kernel, bjorn
  Cc: ericwouds, frank-w, daniel, lucien.jheng, albert-al.lee,
	Lucien.Jheng

AN8811HB needs a MCU soft-reset cycle before firmware loading begins.
Assert the MCU (hold it in reset) and immediately deassert (release)
via a dedicated PBUS register pair (0x5cf9f8 / 0x5cf9fc), accessed
through a registered mdio_device at PHY-addr+8.

Add __air_pbus_reg_write() as a low-level helper taking a struct
mdio_device *, create and register the PBUS mdio_device in
an8811hb_probe() and store it in priv->pbusdev, then implement
an8811hb_mcu_assert() / _deassert() on top of it. Add
an8811hb_remove() to unregister the PBUS device on teardown. Wire
both calls into an8811hb_load_firmware() and en8811h_restart_mcu()
so every firmware load or MCU restart on AN8811HB correctly sequences
the reset control registers.

Fixes: 0a55766b7711 ("net: phy: air_en8811h: add Airoha AN8811HB support")
Signed-off-by: Lucien Jheng <lucienzx159@gmail.com>
---
Changes in v3:
- Register a dedicated mdio_device for the PBUS node (addr+8) in
  an8811hb_probe() and store it in priv->pbusdev, replacing the
  inline addr+8 computation in v2.
- Change __air_pbus_reg_write() to take struct mdio_device * instead
  of struct phy_device *.
- Add an8811hb_remove() to unregister and free the PBUS device.
- Fix error handling in an8811hb_probe() with goto error paths to
  ensure the PBUS device is always cleaned up on failure.

 drivers/net/phy/air_en8811h.c | 142 +++++++++++++++++++++++++++++++++-
 1 file changed, 138 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/air_en8811h.c b/drivers/net/phy/air_en8811h.c
index 29ae73e65..1fe2d8bec 100644
--- a/drivers/net/phy/air_en8811h.c
+++ b/drivers/net/phy/air_en8811h.c
@@ -170,9 +170,21 @@
 #define   AN8811HB_CLK_DRV_CKO_LDPWD		BIT(13)
 #define   AN8811HB_CLK_DRV_CKO_LPPWD		BIT(14)
 
+#define AN8811HB_MCU_SW_RST		0x5cf9f8
+#define   AN8811HB_MCU_SW_RST_HOLD		BIT(16)
+#define   AN8811HB_MCU_SW_RST_RUN		(BIT(16) | BIT(0))
+#define AN8811HB_MCU_SW_START		0x5cf9fc
+#define   AN8811HB_MCU_SW_START_EN		BIT(16)
+
+/* MII register constants for PBUS access (PHY addr + 8) */
+#define AIR_PBUS_ADDR_HIGH		0x1c
+#define AIR_PBUS_DATA_HIGH		0x10
+
 /* Led definitions */
 #define EN8811H_LED_COUNT	3
 
+#define EN8811H_PBUS_ADDR_OFFS	8
+
 /* Default LED setup:
  * GPIO5 <-> LED0  On: Link detected, blink Rx/Tx
  * GPIO4 <-> LED1  On: Link detected at 2500 or 1000 Mbps
@@ -201,6 +213,7 @@ struct en8811h_priv {
 	struct clk_hw		hw;
 	struct phy_device	*phydev;
 	unsigned int		cko_is_enabled;
+	struct mdio_device	*pbusdev;
 };
 
 enum {
@@ -254,6 +267,31 @@ static int air_phy_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, AIR_EXT_PAGE_ACCESS, page);
 }
 
+static int __air_pbus_reg_write(struct mdio_device *mdiodev,
+				u32 pbus_reg, u32 pbus_data)
+{
+	int ret;
+
+	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_EXT_PAGE_ACCESS,
+			      upper_16_bits(pbus_reg));
+	if (ret < 0)
+		return ret;
+
+	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_ADDR_HIGH,
+			      (pbus_reg & GENMASK(15, 6)) >> 6);
+	if (ret < 0)
+		return ret;
+
+	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr,
+			      (pbus_reg & GENMASK(5, 2)) >> 2,
+			      lower_16_bits(pbus_data));
+	if (ret < 0)
+		return ret;
+
+	return __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_DATA_HIGH,
+			       upper_16_bits(pbus_data));
+}
+
 static int __air_buckpbus_reg_write(struct phy_device *phydev,
 				    u32 pbus_address, u32 pbus_data)
 {
@@ -570,10 +608,67 @@ static int an8811hb_load_file(struct phy_device *phydev, const char *name,
 	return ret;
 }
 
+static int an8811hb_mcu_assert(struct phy_device *phydev)
+{
+	struct en8811h_priv *priv = phydev->priv;
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+
+	ret = __air_pbus_reg_write(priv->pbusdev, AN8811HB_MCU_SW_RST,
+				   AN8811HB_MCU_SW_RST_HOLD);
+	if (ret < 0)
+		goto unlock;
+
+	ret = __air_pbus_reg_write(priv->pbusdev, AN8811HB_MCU_SW_START, 0);
+	if (ret < 0)
+		goto unlock;
+
+	msleep(50);
+	phydev_dbg(phydev, "MCU asserted\n");
+
+unlock:
+	phy_unlock_mdio_bus(phydev);
+	return ret;
+}
+
+static int an8811hb_mcu_deassert(struct phy_device *phydev)
+{
+	struct en8811h_priv *priv = phydev->priv;
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+
+	ret = __air_pbus_reg_write(priv->pbusdev, AN8811HB_MCU_SW_START,
+				   AN8811HB_MCU_SW_START_EN);
+	if (ret < 0)
+		goto unlock;
+
+	ret = __air_pbus_reg_write(priv->pbusdev, AN8811HB_MCU_SW_RST,
+				   AN8811HB_MCU_SW_RST_RUN);
+	if (ret < 0)
+		goto unlock;
+
+	msleep(50);
+	phydev_dbg(phydev, "MCU deasserted\n");
+
+unlock:
+	phy_unlock_mdio_bus(phydev);
+	return ret;
+}
+
 static int an8811hb_load_firmware(struct phy_device *phydev)
 {
 	int ret;
 
+	ret = an8811hb_mcu_assert(phydev);
+	if (ret < 0)
+		return ret;
+
+	ret = an8811hb_mcu_deassert(phydev);
+	if (ret < 0)
+		return ret;
+
 	ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
 				     EN8811H_FW_CTRL_1_START);
 	if (ret < 0)
@@ -662,6 +757,16 @@ static int en8811h_restart_mcu(struct phy_device *phydev)
 {
 	int ret;
 
+	if (phy_id_compare_model(phydev->phy_id, AN8811HB_PHY_ID)) {
+		ret = an8811hb_mcu_assert(phydev);
+		if (ret < 0)
+			return ret;
+
+		ret = an8811hb_mcu_deassert(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
 	ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
 				     EN8811H_FW_CTRL_1_START);
 	if (ret < 0)
@@ -1166,6 +1271,7 @@ static int en8811h_leds_setup(struct phy_device *phydev)
 
 static int an8811hb_probe(struct phy_device *phydev)
 {
+	struct mdio_device *mdiodev;
 	struct en8811h_priv *priv;
 	int ret;
 
@@ -1175,10 +1281,22 @@ static int an8811hb_probe(struct phy_device *phydev)
 		return -ENOMEM;
 	phydev->priv = priv;
 
+	mdiodev = mdio_device_create(phydev->mdio.bus,
+				     phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS);
+	if (IS_ERR(mdiodev))
+		return PTR_ERR(mdiodev);
+
+	ret = mdio_device_register(mdiodev);
+	if (ret) {
+		mdio_device_free(mdiodev);
+		return ret;
+	}
+	priv->pbusdev = mdiodev;
+
 	ret = an8811hb_load_firmware(phydev);
 	if (ret < 0) {
 		phydev_err(phydev, "Load firmware failed: %d\n", ret);
-		return ret;
+		goto error;
 	}
 
 	en8811h_print_fw_version(phydev);
@@ -1191,22 +1309,27 @@ static int an8811hb_probe(struct phy_device *phydev)
 
 	ret = en8811h_leds_setup(phydev);
 	if (ret < 0)
-		return ret;
+		goto error;
 
 	priv->phydev = phydev;
 	/* Co-Clock Output */
 	ret = an8811hb_clk_provider_setup(&phydev->mdio.dev, &priv->hw);
 	if (ret)
-		return ret;
+		goto error;
 
 	/* Configure led gpio pins as output */
 	ret = air_buckpbus_reg_modify(phydev, AN8811HB_GPIO_OUTPUT,
 				      AN8811HB_GPIO_OUTPUT_345,
 				      AN8811HB_GPIO_OUTPUT_345);
 	if (ret < 0)
-		return ret;
+		goto error;
 
 	return 0;
+
+error:
+	mdio_device_remove(priv->pbusdev);
+	mdio_device_free(priv->pbusdev);
+	return ret;
 }
 
 static int en8811h_probe(struct phy_device *phydev)
@@ -1561,6 +1684,16 @@ static int en8811h_suspend(struct phy_device *phydev)
 	return genphy_suspend(phydev);
 }
 
+static void an8811hb_remove(struct phy_device *phydev)
+{
+	struct en8811h_priv *priv = phydev->priv;
+
+	if (priv->pbusdev) {
+		mdio_device_remove(priv->pbusdev);
+		mdio_device_free(priv->pbusdev);
+	}
+}
+
 static struct phy_driver en8811h_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(EN8811H_PHY_ID),
@@ -1587,6 +1720,7 @@ static struct phy_driver en8811h_driver[] = {
 	PHY_ID_MATCH_MODEL(AN8811HB_PHY_ID),
 	.name			= "Airoha AN8811HB",
 	.probe			= an8811hb_probe,
+	.remove			= an8811hb_remove,
 	.get_features		= en8811h_get_features,
 	.config_init		= an8811hb_config_init,
 	.get_rate_matching	= en8811h_get_rate_matching,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support
  2026-04-20 13:45 [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support Lucien.Jheng
@ 2026-04-20 14:25 ` Bjørn Mork
  2026-04-23 11:19 ` Paolo Abeni
  2026-04-23 17:21 ` Jakub Kicinski
  2 siblings, 0 replies; 8+ messages in thread
From: Bjørn Mork @ 2026-04-20 14:25 UTC (permalink / raw)
  To: Lucien.Jheng
  Cc: andrew, hkallweit1, linux, davem, edumazet, kuba, pabeni, netdev,
	linux-kernel, ericwouds, frank-w, daniel, lucien.jheng,
	albert-al.lee

"Lucien.Jheng" <lucienzx159@gmail.com> writes:

> AN8811HB needs a MCU soft-reset cycle before firmware loading begins.

Sorry for having missed that.  I assume it explains a rare issue I've
observed where a link down/up sometimes was necessary after a cold boot.
I've been running with your v2 patch for a while, and cannot reproduce
this issue anymore.

Tested-by: Bjørn Mork <bjorn@mork.no>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support
  2026-04-20 13:45 [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support Lucien.Jheng
  2026-04-20 14:25 ` Bjørn Mork
@ 2026-04-23 11:19 ` Paolo Abeni
  2026-04-23 11:58   ` Andrew Lunn
  2026-04-24 15:56   ` Lucien.Jheng
  2026-04-23 17:21 ` Jakub Kicinski
  2 siblings, 2 replies; 8+ messages in thread
From: Paolo Abeni @ 2026-04-23 11:19 UTC (permalink / raw)
  To: Lucien.Jheng, andrew, hkallweit1, linux, davem, edumazet, kuba,
	netdev, linux-kernel, bjorn
  Cc: ericwouds, frank-w, daniel, lucien.jheng, albert-al.lee

On 4/20/26 3:45 PM, Lucien.Jheng wrote:
> AN8811HB needs a MCU soft-reset cycle before firmware loading begins.
> Assert the MCU (hold it in reset) and immediately deassert (release)
> via a dedicated PBUS register pair (0x5cf9f8 / 0x5cf9fc), accessed
> through a registered mdio_device at PHY-addr+8.
> 
> Add __air_pbus_reg_write() as a low-level helper taking a struct
> mdio_device *, create and register the PBUS mdio_device in
> an8811hb_probe() and store it in priv->pbusdev, then implement
> an8811hb_mcu_assert() / _deassert() on top of it. Add
> an8811hb_remove() to unregister the PBUS device on teardown. Wire
> both calls into an8811hb_load_firmware() and en8811h_restart_mcu()
> so every firmware load or MCU restart on AN8811HB correctly sequences
> the reset control registers.
> 
> Fixes: 0a55766b7711 ("net: phy: air_en8811h: add Airoha AN8811HB support")

The hash is incorrect, should be 5afda1d734ed

[...]
> @@ -254,6 +267,31 @@ static int air_phy_write_page(struct phy_device *phydev, int page)
>  	return __phy_write(phydev, AIR_EXT_PAGE_ACCESS, page);
>  }
>  
> +static int __air_pbus_reg_write(struct mdio_device *mdiodev,
> +				u32 pbus_reg, u32 pbus_data)
> +{
> +	int ret;
> +
> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_EXT_PAGE_ACCESS,
> +			      upper_16_bits(pbus_reg));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_ADDR_HIGH,
> +			      (pbus_reg & GENMASK(15, 6)) >> 6);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr,
> +			      (pbus_reg & GENMASK(5, 2)) >> 2,
> +			      lower_16_bits(pbus_data));
> +	if (ret < 0)
> +		return ret;
> +
> +	return __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_DATA_HIGH,
> +			       upper_16_bits(pbus_data));

Sashiko says:

Will writing the lower 16 bits before the upper 16 bits cause the
hardware transaction to fire with stale upper data?
The __air_buckpbus_reg_write() helper triggers the 32-bit transaction
using the lower 16 bits as the execution trigger. If this hardware
behaves similarly, should AIR_PBUS_DATA_HIGH be populated before writing
the lower 16 bits?

[...]
> @@ -1175,10 +1281,22 @@ static int an8811hb_probe(struct phy_device *phydev)
>  		return -ENOMEM;
>  	phydev->priv = priv;
>  
> +	mdiodev = mdio_device_create(phydev->mdio.bus,
> +				     phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS);

Sashiko says:

Can this create an out-of-bounds array access if the base PHY address is
high?
The mdio_map array in struct mii_bus has a fixed size of PHY_MAX_ADDR (32).
If phydev->mdio.addr is 24 or higher, adding EN8811H_PBUS_ADDR_OFFS (8)
will result in an address of 32 or more.
Neither mdio_device_create() nor mdio_device_register() validate that
the address is within PHY_MAX_ADDR. When mdiobus_register_device()
executes mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev, could this
write past the end of the array and corrupt adjacent memory?

/P


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support
  2026-04-23 11:19 ` Paolo Abeni
@ 2026-04-23 11:58   ` Andrew Lunn
  2026-04-24 15:57     ` Lucien.Jheng
  2026-04-24 15:56   ` Lucien.Jheng
  1 sibling, 1 reply; 8+ messages in thread
From: Andrew Lunn @ 2026-04-23 11:58 UTC (permalink / raw)
  To: Paolo Abeni
  Cc: Lucien.Jheng, hkallweit1, linux, davem, edumazet, kuba, netdev,
	linux-kernel, bjorn, ericwouds, frank-w, daniel, lucien.jheng,
	albert-al.lee

> > @@ -1175,10 +1281,22 @@ static int an8811hb_probe(struct phy_device *phydev)
> >  		return -ENOMEM;
> >  	phydev->priv = priv;
> >  
> > +	mdiodev = mdio_device_create(phydev->mdio.bus,
> > +				     phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS);
> 
> Sashiko says:
> 
> Can this create an out-of-bounds array access if the base PHY address is
> high?
> The mdio_map array in struct mii_bus has a fixed size of PHY_MAX_ADDR (32).
> If phydev->mdio.addr is 24 or higher, adding EN8811H_PBUS_ADDR_OFFS (8)
> will result in an address of 32 or more.
> Neither mdio_device_create() nor mdio_device_register() validate that
> the address is within PHY_MAX_ADDR. When mdiobus_register_device()
> executes mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev, could this
> write past the end of the array and corrupt adjacent memory?

This has been discussed once, but Sashiko has a shorter memory than a
goldfish. It is guaranteed by hardware design that + 8 will work.

	  Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support
  2026-04-20 13:45 [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support Lucien.Jheng
  2026-04-20 14:25 ` Bjørn Mork
  2026-04-23 11:19 ` Paolo Abeni
@ 2026-04-23 17:21 ` Jakub Kicinski
  2026-04-24 15:59   ` Lucien.Jheng
  2 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2026-04-23 17:21 UTC (permalink / raw)
  To: Lucien.Jheng
  Cc: andrew, hkallweit1, linux, davem, edumazet, pabeni, netdev,
	linux-kernel, bjorn, ericwouds, frank-w, daniel, lucien.jheng,
	albert-al.lee

Since you need to repost to fix the fixes tag, plaese also improve the
error handling

On Mon, 20 Apr 2026 21:45:06 +0800 Lucien.Jheng wrote:
>  static int an8811hb_probe(struct phy_device *phydev)
>  {
> +	struct mdio_device *mdiodev;
>  	struct en8811h_priv *priv;
>  	int ret;
>  
> @@ -1175,10 +1281,22 @@ static int an8811hb_probe(struct phy_device *phydev)
>  		return -ENOMEM;
>  	phydev->priv = priv;
>  
> +	mdiodev = mdio_device_create(phydev->mdio.bus,
> +				     phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS);
> +	if (IS_ERR(mdiodev))
> +		return PTR_ERR(mdiodev);
> +
> +	ret = mdio_device_register(mdiodev);
> +	if (ret) {

goto err_dev_free;

> +		mdio_device_free(mdiodev);
> +		return ret;
> +	}
> +	priv->pbusdev = mdiodev;
> +
>  	ret = an8811hb_load_firmware(phydev);
>  	if (ret < 0) {
>  		phydev_err(phydev, "Load firmware failed: %d\n", ret);
> -		return ret;
> +		goto error;

goto err_dev_create;

>  	}
>  
>  	en8811h_print_fw_version(phydev);
> @@ -1191,22 +1309,27 @@ static int an8811hb_probe(struct phy_device *phydev)
>  
>  	ret = en8811h_leds_setup(phydev);
>  	if (ret < 0)
> -		return ret;
> +		goto error;
>  
>  	priv->phydev = phydev;
>  	/* Co-Clock Output */
>  	ret = an8811hb_clk_provider_setup(&phydev->mdio.dev, &priv->hw);
>  	if (ret)
> -		return ret;
> +		goto error;
>  
>  	/* Configure led gpio pins as output */
>  	ret = air_buckpbus_reg_modify(phydev, AN8811HB_GPIO_OUTPUT,
>  				      AN8811HB_GPIO_OUTPUT_345,
>  				      AN8811HB_GPIO_OUTPUT_345);
>  	if (ret < 0)
> -		return ret;
> +		goto error;
>  
>  	return 0;
> +
> +error:

err_dev_free:

> +	mdio_device_remove(priv->pbusdev);

err_dev_create:

> +	mdio_device_free(priv->pbusdev);
> +	return ret;

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support
  2026-04-23 11:19 ` Paolo Abeni
  2026-04-23 11:58   ` Andrew Lunn
@ 2026-04-24 15:56   ` Lucien.Jheng
  1 sibling, 0 replies; 8+ messages in thread
From: Lucien.Jheng @ 2026-04-24 15:56 UTC (permalink / raw)
  To: Paolo Abeni, andrew, hkallweit1, linux, davem, edumazet, kuba,
	netdev, linux-kernel, bjorn
  Cc: ericwouds, frank-w, daniel, lucien.jheng, albert-al.lee


Paolo Abeni 於 2026/4/23 下午 07:19 寫道:
> On 4/20/26 3:45 PM, Lucien.Jheng wrote:
>> AN8811HB needs a MCU soft-reset cycle before firmware loading begins.
>> Assert the MCU (hold it in reset) and immediately deassert (release)
>> via a dedicated PBUS register pair (0x5cf9f8 / 0x5cf9fc), accessed
>> through a registered mdio_device at PHY-addr+8.
>>
>> Add __air_pbus_reg_write() as a low-level helper taking a struct
>> mdio_device *, create and register the PBUS mdio_device in
>> an8811hb_probe() and store it in priv->pbusdev, then implement
>> an8811hb_mcu_assert() / _deassert() on top of it. Add
>> an8811hb_remove() to unregister the PBUS device on teardown. Wire
>> both calls into an8811hb_load_firmware() and en8811h_restart_mcu()
>> so every firmware load or MCU restart on AN8811HB correctly sequences
>> the reset control registers.
>>
>> Fixes: 0a55766b7711 ("net: phy: air_en8811h: add Airoha AN8811HB support")
> The hash is incorrect, should be 5afda1d734ed
I will fix it in the next patch.
>
> [...]
>> @@ -254,6 +267,31 @@ static int air_phy_write_page(struct phy_device *phydev, int page)
>>   	return __phy_write(phydev, AIR_EXT_PAGE_ACCESS, page);
>>   }
>>   
>> +static int __air_pbus_reg_write(struct mdio_device *mdiodev,
>> +				u32 pbus_reg, u32 pbus_data)
>> +{
>> +	int ret;
>> +
>> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_EXT_PAGE_ACCESS,
>> +			      upper_16_bits(pbus_reg));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_ADDR_HIGH,
>> +			      (pbus_reg & GENMASK(15, 6)) >> 6);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr,
>> +			      (pbus_reg & GENMASK(5, 2)) >> 2,
>> +			      lower_16_bits(pbus_data));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_DATA_HIGH,
>> +			       upper_16_bits(pbus_data));
> Sashiko says:
>
> Will writing the lower 16 bits before the upper 16 bits cause the
> hardware transaction to fire with stale upper data?
> The __air_buckpbus_reg_write() helper triggers the 32-bit transaction
> using the lower 16 bits as the execution trigger. If this hardware
> behaves similarly, should AIR_PBUS_DATA_HIGH be populated before writing
> the lower 16 bits?
This is an8811hb pbus sequence. Therefore, it doesn't need to be modified.
> [...]
>> @@ -1175,10 +1281,22 @@ static int an8811hb_probe(struct phy_device *phydev)
>>   		return -ENOMEM;
>>   	phydev->priv = priv;
>>   
>> +	mdiodev = mdio_device_create(phydev->mdio.bus,
>> +				     phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS);
> Sashiko says:
>
> Can this create an out-of-bounds array access if the base PHY address is
> high?
> The mdio_map array in struct mii_bus has a fixed size of PHY_MAX_ADDR (32).
> If phydev->mdio.addr is 24 or higher, adding EN8811H_PBUS_ADDR_OFFS (8)
> will result in an address of 32 or more.
> Neither mdio_device_create() nor mdio_device_register() validate that
> the address is within PHY_MAX_ADDR. When mdiobus_register_device()
> executes mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev, could this
> write past the end of the array and corrupt adjacent memory?
>
> /P
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support
  2026-04-23 11:58   ` Andrew Lunn
@ 2026-04-24 15:57     ` Lucien.Jheng
  0 siblings, 0 replies; 8+ messages in thread
From: Lucien.Jheng @ 2026-04-24 15:57 UTC (permalink / raw)
  To: Andrew Lunn, Paolo Abeni
  Cc: hkallweit1, linux, davem, edumazet, kuba, netdev, linux-kernel,
	bjorn, ericwouds, frank-w, daniel, lucien.jheng, albert-al.lee


Andrew Lunn 於 2026/4/23 下午 07:58 寫道:
>>> @@ -1175,10 +1281,22 @@ static int an8811hb_probe(struct phy_device *phydev)
>>>   		return -ENOMEM;
>>>   	phydev->priv = priv;
>>>   
>>> +	mdiodev = mdio_device_create(phydev->mdio.bus,
>>> +				     phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS);
>> Sashiko says:
>>
>> Can this create an out-of-bounds array access if the base PHY address is
>> high?
>> The mdio_map array in struct mii_bus has a fixed size of PHY_MAX_ADDR (32).
>> If phydev->mdio.addr is 24 or higher, adding EN8811H_PBUS_ADDR_OFFS (8)
>> will result in an address of 32 or more.
>> Neither mdio_device_create() nor mdio_device_register() validate that
>> the address is within PHY_MAX_ADDR. When mdiobus_register_device()
>> executes mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev, could this
>> write past the end of the array and corrupt adjacent memory?
> This has been discussed once, but Sashiko has a shorter memory than a
> goldfish. It is guaranteed by hardware design that + 8 will work.

Got it

Thanks

>
> 	  Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support
  2026-04-23 17:21 ` Jakub Kicinski
@ 2026-04-24 15:59   ` Lucien.Jheng
  0 siblings, 0 replies; 8+ messages in thread
From: Lucien.Jheng @ 2026-04-24 15:59 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: andrew, hkallweit1, linux, davem, edumazet, pabeni, netdev,
	linux-kernel, bjorn, ericwouds, frank-w, daniel, lucien.jheng,
	albert-al.lee


Jakub Kicinski 於 2026/4/24 上午 01:21 寫道:
> Since you need to repost to fix the fixes tag, plaese also improve the
> error handling
>
> On Mon, 20 Apr 2026 21:45:06 +0800 Lucien.Jheng wrote:
>>   static int an8811hb_probe(struct phy_device *phydev)
>>   {
>> +	struct mdio_device *mdiodev;
>>   	struct en8811h_priv *priv;
>>   	int ret;
>>   
>> @@ -1175,10 +1281,22 @@ static int an8811hb_probe(struct phy_device *phydev)
>>   		return -ENOMEM;
>>   	phydev->priv = priv;
>>   
>> +	mdiodev = mdio_device_create(phydev->mdio.bus,
>> +				     phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS);
>> +	if (IS_ERR(mdiodev))
>> +		return PTR_ERR(mdiodev);
>> +
>> +	ret = mdio_device_register(mdiodev);
>> +	if (ret) {
> goto err_dev_free;
>
>> +		mdio_device_free(mdiodev);
>> +		return ret;
>> +	}
>> +	priv->pbusdev = mdiodev;
>> +
>>   	ret = an8811hb_load_firmware(phydev);
>>   	if (ret < 0) {
>>   		phydev_err(phydev, "Load firmware failed: %d\n", ret);
>> -		return ret;
>> +		goto error;
> goto err_dev_create;
>
>>   	}
>>   
>>   	en8811h_print_fw_version(phydev);
>> @@ -1191,22 +1309,27 @@ static int an8811hb_probe(struct phy_device *phydev)
>>   
>>   	ret = en8811h_leds_setup(phydev);
>>   	if (ret < 0)
>> -		return ret;
>> +		goto error;
>>   
>>   	priv->phydev = phydev;
>>   	/* Co-Clock Output */
>>   	ret = an8811hb_clk_provider_setup(&phydev->mdio.dev, &priv->hw);
>>   	if (ret)
>> -		return ret;
>> +		goto error;
>>   
>>   	/* Configure led gpio pins as output */
>>   	ret = air_buckpbus_reg_modify(phydev, AN8811HB_GPIO_OUTPUT,
>>   				      AN8811HB_GPIO_OUTPUT_345,
>>   				      AN8811HB_GPIO_OUTPUT_345);
>>   	if (ret < 0)
>> -		return ret;
>> +		goto error;
>>   
>>   	return 0;
>> +
>> +error:
> err_dev_free:
>
>> +	mdio_device_remove(priv->pbusdev);
> err_dev_create:
>
>> +	mdio_device_free(priv->pbusdev);
>> +	return ret;
I will include this in the next patch.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-04-24 15:59 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-20 13:45 [PATCH v3] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support Lucien.Jheng
2026-04-20 14:25 ` Bjørn Mork
2026-04-23 11:19 ` Paolo Abeni
2026-04-23 11:58   ` Andrew Lunn
2026-04-24 15:57     ` Lucien.Jheng
2026-04-24 15:56   ` Lucien.Jheng
2026-04-23 17:21 ` Jakub Kicinski
2026-04-24 15:59   ` Lucien.Jheng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox