All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net 0/2] phy init update and alu table correction
@ 2023-01-16 10:04 Rakesh Sankaranarayanan
  2023-01-16 10:04 ` [PATCH net 1/2] net: dsa: microchip: ksz9477: port map correction in ALU table entry register Rakesh Sankaranarayanan
  2023-01-16 10:05 ` [PATCH net 2/2] net: dsa: microchip: lan937x: run phy initialization during each link update Rakesh Sankaranarayanan
  0 siblings, 2 replies; 10+ messages in thread
From: Rakesh Sankaranarayanan @ 2023-01-16 10:04 UTC (permalink / raw)
  To: netdev, linux-kernel
  Cc: woojung.huh, UNGLinuxDriver, andrew, f.fainelli, olteanv, davem,
	edumazet, kuba, pabeni, arun.ramadoss, hkallweit1, linux

This bug-fix patch series contains following changes,

- Update forward table map in ksz9477_fdb_del API.
- Invoke phy init sequence each time of mode update in
  lan87xx_config_aneg.
- Execute phy slave init command only if phydev is configured in slave
  mode.

Rakesh Sankaranarayanan (2):
  net: dsa: microchip: ksz9477: port map correction in ALU table entry
    register
  net: dsa: microchip: lan937x: run phy initialization during each link
    update

 drivers/net/dsa/microchip/ksz9477.c |  4 +-
 drivers/net/phy/microchip_t1.c      | 70 +++++++++++++++++++++++------
 2 files changed, 58 insertions(+), 16 deletions(-)

-- 
2.34.1


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

* [PATCH net 1/2] net: dsa: microchip: ksz9477: port map correction in ALU table entry register
  2023-01-16 10:04 [PATCH net 0/2] phy init update and alu table correction Rakesh Sankaranarayanan
@ 2023-01-16 10:04 ` Rakesh Sankaranarayanan
  2023-01-16 20:59   ` Vladimir Oltean
  2023-01-16 10:05 ` [PATCH net 2/2] net: dsa: microchip: lan937x: run phy initialization during each link update Rakesh Sankaranarayanan
  1 sibling, 1 reply; 10+ messages in thread
From: Rakesh Sankaranarayanan @ 2023-01-16 10:04 UTC (permalink / raw)
  To: netdev, linux-kernel
  Cc: woojung.huh, UNGLinuxDriver, andrew, f.fainelli, olteanv, davem,
	edumazet, kuba, pabeni, arun.ramadoss, hkallweit1, linux

ALU table entry 2 register in KSZ9477 have bit positions reserved for
forwarding port map. This field is referred in ksz9477_fdb_del() for
clearing forward port map and alu table.

But current fdb_del refer ALU table entry 3 register for accessing forward
port map. Update ksz9477_fdb_del() to get forward port map from correct
alu table entry register.

With this bug, issue can be observed while deleting static MAC entries.
Delete any specific MAC entry using "bridge fdb del" command. This should
clear all the specified MAC entries. But it is observed that entries with
self static alone are retained.

Tested on LAN9370 EVB since ksz9477_fdb_del() is used common across
LAN937x and KSZ series.

Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
Signed-off-by: Rakesh Sankaranarayanan <rakesh.sankaranarayanan@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 47b54ecf2c6f..6178a96e389f 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -540,10 +540,10 @@ int ksz9477_fdb_del(struct ksz_device *dev, int port,
 		ksz_read32(dev, REG_SW_ALU_VAL_D, &alu_table[3]);
 
 		/* clear forwarding port */
-		alu_table[2] &= ~BIT(port);
+		alu_table[1] &= ~BIT(port);
 
 		/* if there is no port to forward, clear table */
-		if ((alu_table[2] & ALU_V_PORT_MAP) == 0) {
+		if ((alu_table[1] & ALU_V_PORT_MAP) == 0) {
 			alu_table[0] = 0;
 			alu_table[1] = 0;
 			alu_table[2] = 0;
-- 
2.34.1


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

* [PATCH net 2/2] net: dsa: microchip: lan937x: run phy initialization during each link update
  2023-01-16 10:04 [PATCH net 0/2] phy init update and alu table correction Rakesh Sankaranarayanan
  2023-01-16 10:04 ` [PATCH net 1/2] net: dsa: microchip: ksz9477: port map correction in ALU table entry register Rakesh Sankaranarayanan
@ 2023-01-16 10:05 ` Rakesh Sankaranarayanan
  2023-01-16 22:26   ` Vladimir Oltean
  1 sibling, 1 reply; 10+ messages in thread
From: Rakesh Sankaranarayanan @ 2023-01-16 10:05 UTC (permalink / raw)
  To: netdev, linux-kernel
  Cc: woojung.huh, UNGLinuxDriver, andrew, f.fainelli, olteanv, davem,
	edumazet, kuba, pabeni, arun.ramadoss, hkallweit1, linux

PHY initialization is supposed to run on every mode changes.
"lan87xx_config_aneg()" verifies every mode change using
"phy_modify_changed()" function. Earlier code had phy_modify_changed()
followed by genphy_soft_reset. But soft_reset resets all the
pre-configured register values to default state, and lost all the
initialization done. With this reason gen_phy_reset was removed.
But it need to go through init sequence each time the mode changed.
Update lan97xx_config_aneg() to invoke phy_init once successful mode
update is detected.

PHY init sequence added in lan87xx_phy_init() have slave init
commands executed every time. Update the init sequence to run
slave init only if phydev is in slave mode.

Test setup contains LAN9370 EVB connected to SAMA5D3 (Running DSA),
and issue can be reproduced by connecting link to any of the available
ports after SAMA5D3 boot-up. With this issue, port will fail to
update link state. But once the SAMA5D3 is reset with LAN9370 link in
connected state itself, on boot-up link state will be reported as UP. But
Again after some time, if link is moved to DOWN state, it will not get
reported.

Fixes: b2cd2cde7d69 ("net: phy: LAN87xx: remove genphy_softreset in config_aneg")
Signed-off-by: Rakesh Sankaranarayanan <rakesh.sankaranarayanan@microchip.com>
---
 drivers/net/phy/microchip_t1.c | 70 +++++++++++++++++++++++++++-------
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
index 8569a545e0a3..78618c8cb6bf 100644
--- a/drivers/net/phy/microchip_t1.c
+++ b/drivers/net/phy/microchip_t1.c
@@ -245,15 +245,42 @@ static int lan87xx_config_rgmii_delay(struct phy_device *phydev)
 			   PHYACC_ATTR_BANK_MISC, LAN87XX_CTRL_1, rc);
 }
 
+static int lan87xx_phy_init_cmd(struct phy_device *phydev,
+				const struct access_ereg_val *cmd_seq, int cnt)
+{
+	int ret, i;
+
+	for (i = 0; i < cnt; i++) {
+		if (cmd_seq[i].mode == PHYACC_ATTR_MODE_POLL &&
+		    cmd_seq[i].bank == PHYACC_ATTR_BANK_SMI) {
+			ret = access_smi_poll_timeout(phydev,
+						      cmd_seq[i].offset,
+						      cmd_seq[i].val,
+						      cmd_seq[i].mask);
+		} else {
+			ret = access_ereg(phydev, cmd_seq[i].mode,
+					  cmd_seq[i].bank, cmd_seq[i].offset,
+					  cmd_seq[i].val);
+		}
+		if (ret < 0)
+			return ret;
+	}
+
+	return ret;
+}
+
 static int lan87xx_phy_init(struct phy_device *phydev)
 {
-	static const struct access_ereg_val init[] = {
+	static const struct access_ereg_val hw_init[] = {
 		/* TXPD/TXAMP6 Configs */
 		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_AFE,
 		  T1_AFE_PORT_CFG1_REG,       0x002D,  0 },
 		/* HW_Init Hi and Force_ED */
 		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_SMI,
 		  T1_POWER_DOWN_CONTROL_REG,  0x0308,  0 },
+	};
+
+	static const struct access_ereg_val slave_init[] = {
 		/* Equalizer Full Duplex Freeze - T1 Slave */
 		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP,
 		  T1_EQ_FD_STG1_FRZ_CFG,     0x0002,  0 },
@@ -267,6 +294,9 @@ static int lan87xx_phy_init(struct phy_device *phydev)
 		  T1_EQ_WT_FD_LCK_FRZ_CFG,    0x0002,  0 },
 		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP,
 		  T1_PST_EQ_LCK_STG1_FRZ_CFG, 0x0002,  0 },
+	};
+
+	static const struct access_ereg_val phy_init[] = {
 		/* Slave Full Duplex Multi Configs */
 		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP,
 		  T1_SLV_FD_MULT_CFG_REG,     0x0D53,  0 },
@@ -397,7 +427,7 @@ static int lan87xx_phy_init(struct phy_device *phydev)
 		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_SMI,
 		  T1_POWER_DOWN_CONTROL_REG,	0x0300, 0 },
 	};
-	int rc, i;
+	int rc;
 
 	/* phy Soft reset */
 	rc = genphy_soft_reset(phydev);
@@ -405,21 +435,28 @@ static int lan87xx_phy_init(struct phy_device *phydev)
 		return rc;
 
 	/* PHY Initialization */
-	for (i = 0; i < ARRAY_SIZE(init); i++) {
-		if (init[i].mode == PHYACC_ATTR_MODE_POLL &&
-		    init[i].bank == PHYACC_ATTR_BANK_SMI) {
-			rc = access_smi_poll_timeout(phydev,
-						     init[i].offset,
-						     init[i].val,
-						     init[i].mask);
-		} else {
-			rc = access_ereg(phydev, init[i].mode, init[i].bank,
-					 init[i].offset, init[i].val);
-		}
+	rc = lan87xx_phy_init_cmd(phydev, hw_init, ARRAY_SIZE(hw_init));
+	if (rc < 0)
+		return rc;
+
+	rc = genphy_read_master_slave(phydev);
+	if (rc)
+		return rc;
+
+	/* Following squence need to run only if phydev is in
+	 * slave mode.
+	 */
+	if (phydev->master_slave_state == MASTER_SLAVE_STATE_SLAVE) {
+		rc = lan87xx_phy_init_cmd(phydev, slave_init,
+					  ARRAY_SIZE(slave_init));
 		if (rc < 0)
 			return rc;
 	}
 
+	rc = lan87xx_phy_init_cmd(phydev, phy_init, ARRAY_SIZE(phy_init));
+	if (rc < 0)
+		return rc;
+
 	return lan87xx_config_rgmii_delay(phydev);
 }
 
@@ -775,6 +812,7 @@ static int lan87xx_read_status(struct phy_device *phydev)
 static int lan87xx_config_aneg(struct phy_device *phydev)
 {
 	u16 ctl = 0;
+	int ret;
 
 	switch (phydev->master_slave_set) {
 	case MASTER_SLAVE_CFG_MASTER_FORCE:
@@ -790,7 +828,11 @@ static int lan87xx_config_aneg(struct phy_device *phydev)
 		return -EOPNOTSUPP;
 	}
 
-	return phy_modify_changed(phydev, MII_CTRL1000, CTL1000_AS_MASTER, ctl);
+	ret = phy_modify_changed(phydev, MII_CTRL1000, CTL1000_AS_MASTER, ctl);
+	if (ret == 1)
+		return phy_init_hw(phydev);
+
+	return ret;
 }
 
 static int lan87xx_get_sqi(struct phy_device *phydev)
-- 
2.34.1


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

* Re: [PATCH net 1/2] net: dsa: microchip: ksz9477: port map correction in ALU table entry register
  2023-01-16 10:04 ` [PATCH net 1/2] net: dsa: microchip: ksz9477: port map correction in ALU table entry register Rakesh Sankaranarayanan
@ 2023-01-16 20:59   ` Vladimir Oltean
  0 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2023-01-16 20:59 UTC (permalink / raw)
  To: Rakesh Sankaranarayanan
  Cc: netdev, linux-kernel, woojung.huh, UNGLinuxDriver, andrew,
	f.fainelli, davem, edumazet, kuba, pabeni, arun.ramadoss,
	hkallweit1, linux

On Mon, Jan 16, 2023 at 03:34:59PM +0530, Rakesh Sankaranarayanan wrote:
> ALU table entry 2 register in KSZ9477 have bit positions reserved for
> forwarding port map. This field is referred in ksz9477_fdb_del() for
> clearing forward port map and alu table.
> 
> But current fdb_del refer ALU table entry 3 register for accessing forward
> port map. Update ksz9477_fdb_del() to get forward port map from correct
> alu table entry register.
> 
> With this bug, issue can be observed while deleting static MAC entries.
> Delete any specific MAC entry using "bridge fdb del" command. This should
> clear all the specified MAC entries. But it is observed that entries with
> self static alone are retained.
> 
> Tested on LAN9370 EVB since ksz9477_fdb_del() is used common across
> LAN937x and KSZ series.
> 
> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Rakesh Sankaranarayanan <rakesh.sankaranarayanan@microchip.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Would be nice to convert the alu_table[] array into a proper structure
in a net-next patch.

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

* Re: [PATCH net 2/2] net: dsa: microchip: lan937x: run phy initialization during each link update
  2023-01-16 10:05 ` [PATCH net 2/2] net: dsa: microchip: lan937x: run phy initialization during each link update Rakesh Sankaranarayanan
@ 2023-01-16 22:26   ` Vladimir Oltean
  2023-01-19 11:34     ` Rakesh.Sankaranarayanan
  0 siblings, 1 reply; 10+ messages in thread
From: Vladimir Oltean @ 2023-01-16 22:26 UTC (permalink / raw)
  To: Rakesh Sankaranarayanan
  Cc: netdev, linux-kernel, woojung.huh, UNGLinuxDriver, andrew,
	f.fainelli, davem, edumazet, kuba, pabeni, arun.ramadoss,
	hkallweit1, linux

On Mon, Jan 16, 2023 at 03:35:00PM +0530, Rakesh Sankaranarayanan wrote:
> PHY initialization is supposed to run on every mode changes.
> "lan87xx_config_aneg()" verifies every mode change using
> "phy_modify_changed()" function. Earlier code had phy_modify_changed()
> followed by genphy_soft_reset. But soft_reset resets all the
> pre-configured register values to default state, and lost all the
> initialization done. With this reason gen_phy_reset was removed.
> But it need to go through init sequence each time the mode changed.
> Update lan97xx_config_aneg() to invoke phy_init once successful mode
> update is detected.
> 
> PHY init sequence added in lan87xx_phy_init() have slave init
> commands executed every time. Update the init sequence to run
> slave init only if phydev is in slave mode.
> 
> Test setup contains LAN9370 EVB connected to SAMA5D3 (Running DSA),
> and issue can be reproduced by connecting link to any of the available
> ports after SAMA5D3 boot-up. With this issue, port will fail to
> update link state. But once the SAMA5D3 is reset with LAN9370 link in
> connected state itself, on boot-up link state will be reported as UP. But
> Again after some time, if link is moved to DOWN state, it will not get
> reported.
> 
> Fixes: b2cd2cde7d69 ("net: phy: LAN87xx: remove genphy_softreset in config_aneg")
> Signed-off-by: Rakesh Sankaranarayanan <rakesh.sankaranarayanan@microchip.com>
> ---

Process related comments:

1. Don't prefix a patch with "net: dsa: microchip: " unless it touches
   the drivers/net/dsa/microchip/ folder.

2. Don't make unrelated patches on different drivers part of the same
   patch set.

3. AFAIU, this is the second fixup of a feature which never worked well
   (changing master/slave setting through ethtool). Not sure exactly
   what are the rules, but at some point, maintainers might say
   "hey, let go, this never worked, just send your fixes to net-next".
   I mean: (1) fixes of fixes of smth that never worked can't be sent ad
   infinitum, especially if not small and (2) there needs to be some
   incentive to submit code that actually works and was tested, rather
   than a placeholder which can be fixed up later, right? In this case,
   I'm not sure, this seems borderline net-next. Let's see what the PHY
   library maintainers think.

The code seems ok, I couldn't find flaws in it.

>  drivers/net/phy/microchip_t1.c | 70 +++++++++++++++++++++++++++-------
>  1 file changed, 56 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
> index 8569a545e0a3..78618c8cb6bf 100644
> --- a/drivers/net/phy/microchip_t1.c
> +++ b/drivers/net/phy/microchip_t1.c
> @@ -245,15 +245,42 @@ static int lan87xx_config_rgmii_delay(struct phy_device *phydev)
>  			   PHYACC_ATTR_BANK_MISC, LAN87XX_CTRL_1, rc);
>  }
>  
> +static int lan87xx_phy_init_cmd(struct phy_device *phydev,
> +				const struct access_ereg_val *cmd_seq, int cnt)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < cnt; i++) {
> +		if (cmd_seq[i].mode == PHYACC_ATTR_MODE_POLL &&
> +		    cmd_seq[i].bank == PHYACC_ATTR_BANK_SMI) {
> +			ret = access_smi_poll_timeout(phydev,
> +						      cmd_seq[i].offset,
> +						      cmd_seq[i].val,
> +						      cmd_seq[i].mask);
> +		} else {
> +			ret = access_ereg(phydev, cmd_seq[i].mode,
> +					  cmd_seq[i].bank, cmd_seq[i].offset,
> +					  cmd_seq[i].val);
> +		}
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return ret;
> +}
> +
>  static int lan87xx_phy_init(struct phy_device *phydev)
>  {
> -	static const struct access_ereg_val init[] = {
> +	static const struct access_ereg_val hw_init[] = {
>  		/* TXPD/TXAMP6 Configs */
>  		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_AFE,
>  		  T1_AFE_PORT_CFG1_REG,       0x002D,  0 },
>  		/* HW_Init Hi and Force_ED */
>  		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_SMI,
>  		  T1_POWER_DOWN_CONTROL_REG,  0x0308,  0 },
> +	};
> +
> +	static const struct access_ereg_val slave_init[] = {
>  		/* Equalizer Full Duplex Freeze - T1 Slave */
>  		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP,
>  		  T1_EQ_FD_STG1_FRZ_CFG,     0x0002,  0 },
> @@ -267,6 +294,9 @@ static int lan87xx_phy_init(struct phy_device *phydev)
>  		  T1_EQ_WT_FD_LCK_FRZ_CFG,    0x0002,  0 },
>  		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP,
>  		  T1_PST_EQ_LCK_STG1_FRZ_CFG, 0x0002,  0 },
> +	};
> +
> +	static const struct access_ereg_val phy_init[] = {
>  		/* Slave Full Duplex Multi Configs */
>  		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_DSP,
>  		  T1_SLV_FD_MULT_CFG_REG,     0x0D53,  0 },
> @@ -397,7 +427,7 @@ static int lan87xx_phy_init(struct phy_device *phydev)
>  		{ PHYACC_ATTR_MODE_WRITE, PHYACC_ATTR_BANK_SMI,
>  		  T1_POWER_DOWN_CONTROL_REG,	0x0300, 0 },
>  	};
> -	int rc, i;
> +	int rc;
>  
>  	/* phy Soft reset */
>  	rc = genphy_soft_reset(phydev);
> @@ -405,21 +435,28 @@ static int lan87xx_phy_init(struct phy_device *phydev)
>  		return rc;
>  
>  	/* PHY Initialization */
> -	for (i = 0; i < ARRAY_SIZE(init); i++) {
> -		if (init[i].mode == PHYACC_ATTR_MODE_POLL &&
> -		    init[i].bank == PHYACC_ATTR_BANK_SMI) {
> -			rc = access_smi_poll_timeout(phydev,
> -						     init[i].offset,
> -						     init[i].val,
> -						     init[i].mask);
> -		} else {
> -			rc = access_ereg(phydev, init[i].mode, init[i].bank,
> -					 init[i].offset, init[i].val);
> -		}
> +	rc = lan87xx_phy_init_cmd(phydev, hw_init, ARRAY_SIZE(hw_init));
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = genphy_read_master_slave(phydev);
> +	if (rc)
> +		return rc;
> +
> +	/* Following squence need to run only if phydev is in

s/Following squence need/The following sequence needs/

> +	 * slave mode.
> +	 */
> +	if (phydev->master_slave_state == MASTER_SLAVE_STATE_SLAVE) {
> +		rc = lan87xx_phy_init_cmd(phydev, slave_init,
> +					  ARRAY_SIZE(slave_init));
>  		if (rc < 0)
>  			return rc;
>  	}
>  
> +	rc = lan87xx_phy_init_cmd(phydev, phy_init, ARRAY_SIZE(phy_init));
> +	if (rc < 0)
> +		return rc;
> +
>  	return lan87xx_config_rgmii_delay(phydev);
>  }
>  
> @@ -775,6 +812,7 @@ static int lan87xx_read_status(struct phy_device *phydev)
>  static int lan87xx_config_aneg(struct phy_device *phydev)
>  {
>  	u16 ctl = 0;
> +	int ret;
>  
>  	switch (phydev->master_slave_set) {
>  	case MASTER_SLAVE_CFG_MASTER_FORCE:
> @@ -790,7 +828,11 @@ static int lan87xx_config_aneg(struct phy_device *phydev)
>  		return -EOPNOTSUPP;
>  	}
>  
> -	return phy_modify_changed(phydev, MII_CTRL1000, CTL1000_AS_MASTER, ctl);
> +	ret = phy_modify_changed(phydev, MII_CTRL1000, CTL1000_AS_MASTER, ctl);
> +	if (ret == 1)
> +		return phy_init_hw(phydev);
> +
> +	return ret;
>  }
>  
>  static int lan87xx_get_sqi(struct phy_device *phydev)
> -- 
> 2.34.1
> 


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

* Re: [PATCH net 2/2] net: dsa: microchip: lan937x: run phy initialization during each link update
  2023-01-16 22:26   ` Vladimir Oltean
@ 2023-01-19 11:34     ` Rakesh.Sankaranarayanan
  2023-01-19 11:36       ` Vladimir Oltean
  2023-01-19 17:27       ` Andrew Lunn
  0 siblings, 2 replies; 10+ messages in thread
From: Rakesh.Sankaranarayanan @ 2023-01-19 11:34 UTC (permalink / raw)
  To: olteanv, andrew
  Cc: davem, pabeni, hkallweit1, Arun.Ramadoss, Woojung.Huh,
	linux-kernel, linux, f.fainelli, kuba, edumazet, UNGLinuxDriver,
	netdev

Hi Vladimir,
Thanks for the comments.

> 1. Don't prefix a patch with "net: dsa: microchip: " unless it
> touches
>    the drivers/net/dsa/microchip/ folder.
> 
> 2. Don't make unrelated patches on different drivers part of the same
>    patch set.
> 
I will update the patch in next revision.

> 3. AFAIU, this is the second fixup of a feature which never worked
> well
>    (changing master/slave setting through ethtool). Not sure exactly
>    what are the rules, but at some point, maintainers might say
>    "hey, let go, this never worked, just send your fixes to net-
> next".
>    I mean: (1) fixes of fixes of smth that never worked can't be sent
> ad
>    infinitum, especially if not small and (2) there needs to be some
>    incentive to submit code that actually works and was tested,
> rather
>    than a placeholder which can be fixed up later, right? In this
> case,
>    I'm not sure, this seems borderline net-next. Let's see what the
> PHY
>    library maintainers think.
> 

Thanks for pointing this out. Do you think submitting this patch in
net-next is the right way?

@andrew,
Do you have any thoughts on this?

Thanks,
Rakesh S.

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

* Re: [PATCH net 2/2] net: dsa: microchip: lan937x: run phy initialization during each link update
  2023-01-19 11:34     ` Rakesh.Sankaranarayanan
@ 2023-01-19 11:36       ` Vladimir Oltean
  2023-01-19 17:27       ` Andrew Lunn
  1 sibling, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2023-01-19 11:36 UTC (permalink / raw)
  To: Rakesh.Sankaranarayanan
  Cc: andrew, davem, pabeni, hkallweit1, Arun.Ramadoss, Woojung.Huh,
	linux-kernel, linux, f.fainelli, kuba, edumazet, UNGLinuxDriver,
	netdev

On Thu, Jan 19, 2023 at 11:34:00AM +0000, Rakesh.Sankaranarayanan@microchip.com wrote:
> Thanks for pointing this out. Do you think submitting this patch in
> net-next is the right way?

Hmm, I guess you could submit it to 'net'.

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

* Re: [PATCH net 2/2] net: dsa: microchip: lan937x: run phy initialization during each link update
  2023-01-19 11:34     ` Rakesh.Sankaranarayanan
  2023-01-19 11:36       ` Vladimir Oltean
@ 2023-01-19 17:27       ` Andrew Lunn
  2023-01-19 17:35         ` Jakub Kicinski
  1 sibling, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2023-01-19 17:27 UTC (permalink / raw)
  To: Rakesh.Sankaranarayanan
  Cc: olteanv, davem, pabeni, hkallweit1, Arun.Ramadoss, Woojung.Huh,
	linux-kernel, linux, f.fainelli, kuba, edumazet, UNGLinuxDriver,
	netdev

On Thu, Jan 19, 2023 at 11:34:00AM +0000, Rakesh.Sankaranarayanan@microchip.com wrote:
> Hi Vladimir,
> Thanks for the comments.
> 
> > 1. Don't prefix a patch with "net: dsa: microchip: " unless it
> > touches
> >    the drivers/net/dsa/microchip/ folder.
> > 
> > 2. Don't make unrelated patches on different drivers part of the same
> >    patch set.
> > 
> I will update the patch in next revision.
> 
> > 3. AFAIU, this is the second fixup of a feature which never worked
> > well
> >    (changing master/slave setting through ethtool). Not sure exactly
> >    what are the rules, but at some point, maintainers might say
> >    "hey, let go, this never worked, just send your fixes to net-
> > next".
> >    I mean: (1) fixes of fixes of smth that never worked can't be sent
> > ad
> >    infinitum, especially if not small and (2) there needs to be some
> >    incentive to submit code that actually works and was tested,
> > rather
> >    than a placeholder which can be fixed up later, right? In this
> > case,
> >    I'm not sure, this seems borderline net-next. Let's see what the
> > PHY
> >    library maintainers think.
> > 
> 
> Thanks for pointing this out. Do you think submitting this patch in
> net-next is the right way?

I would probably go for net-next. That will give it more soak time to
find the next way it is broken....

You might find i gets back ported to stable anyway, due to the ML bot
spotting it.

	 Andrew

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

* Re: [PATCH net 2/2] net: dsa: microchip: lan937x: run phy initialization during each link update
  2023-01-19 17:27       ` Andrew Lunn
@ 2023-01-19 17:35         ` Jakub Kicinski
  2023-01-20 10:19           ` Rakesh.Sankaranarayanan
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2023-01-19 17:35 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Rakesh.Sankaranarayanan, olteanv, davem, pabeni, hkallweit1,
	Arun.Ramadoss, Woojung.Huh, linux-kernel, linux, f.fainelli,
	edumazet, UNGLinuxDriver, netdev

On Thu, 19 Jan 2023 18:27:52 +0100 Andrew Lunn wrote:
> > Thanks for pointing this out. Do you think submitting this patch in
> > net-next is the right way?  
> 
> I would probably go for net-next. That will give it more soak time to
> find the next way it is broken....

Either a fix or not a fix :( Meaning - if we opt for net-next
please drop the Fixes tag.

FWIW Greg promised that if we put some sort of a tag or information 
to delay backporting to stable they will obey it. We should test that
at some point.

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

* Re: [PATCH net 2/2] net: dsa: microchip: lan937x: run phy initialization during each link update
  2023-01-19 17:35         ` Jakub Kicinski
@ 2023-01-20 10:19           ` Rakesh.Sankaranarayanan
  0 siblings, 0 replies; 10+ messages in thread
From: Rakesh.Sankaranarayanan @ 2023-01-20 10:19 UTC (permalink / raw)
  To: olteanv, kuba, andrew
  Cc: Arun.Ramadoss, linux, hkallweit1, davem, Woojung.Huh,
	linux-kernel, f.fainelli, pabeni, edumazet, UNGLinuxDriver,
	netdev

Thanks Vladimir, Andrew and Jakub for the support.

I will resubmit the patch on net-next.

Thanks,
Rakesh S.

On Thu, 2023-01-19 at 09:35 -0800, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Thu, 19 Jan 2023 18:27:52 +0100 Andrew Lunn wrote:
> > > Thanks for pointing this out. Do you think submitting this patch
> > > in
> > > net-next is the right way?
> > 
> > I would probably go for net-next. That will give it more soak time
> > to
> > find the next way it is broken....
> 
> Either a fix or not a fix :( Meaning - if we opt for net-next
> please drop the Fixes tag.
> 
> FWIW Greg promised that if we put some sort of a tag or information
> to delay backporting to stable they will obey it. We should test that
> at some point.


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

end of thread, other threads:[~2023-01-20 10:19 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-16 10:04 [PATCH net 0/2] phy init update and alu table correction Rakesh Sankaranarayanan
2023-01-16 10:04 ` [PATCH net 1/2] net: dsa: microchip: ksz9477: port map correction in ALU table entry register Rakesh Sankaranarayanan
2023-01-16 20:59   ` Vladimir Oltean
2023-01-16 10:05 ` [PATCH net 2/2] net: dsa: microchip: lan937x: run phy initialization during each link update Rakesh Sankaranarayanan
2023-01-16 22:26   ` Vladimir Oltean
2023-01-19 11:34     ` Rakesh.Sankaranarayanan
2023-01-19 11:36       ` Vladimir Oltean
2023-01-19 17:27       ` Andrew Lunn
2023-01-19 17:35         ` Jakub Kicinski
2023-01-20 10:19           ` Rakesh.Sankaranarayanan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.