Re: [PATCH net-next v4 4/5] net: altera_tse: explicitly disable autoscan on the regmap-mdio bus

Re: [PATCH net-next v4 4/5] net: altera_tse: explicitly disable autoscan on the regmap-mdio bus

[Russell King (Oracle)]

On Wed, Jun 07, 2023 at 03:59:40PM +0200, Maxime Chevallier wrote:
> Set the .autoscan flag to false on the regmap-mdio bus, to avoid using a
> random uninitialized value. We don't want autoscan in this case as the
> mdio device is a PCS and not a PHY.

Isn't this now covered by patch 1's memset of mrc?

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

Re: [PATCH net-next v4 5/5] net: dwmac_socfpga: initialize local data for mdio regmap configuration

[Russell King (Oracle)]

On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> @@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
>  		struct mdio_regmap_config mrc;
>  		struct regmap *pcs_regmap;
>  		struct mii_bus *pcs_bus;
>  
...
> +		memset(&mrc, 0, sizeof(mrc));
...
>  		mrc.parent = &pdev->dev;
>  		mrc.valid_addr = 0x0;
> +		mrc.autoscan = false;

Isn't this covered by the memset() ?

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

[PATCH net-next v4 0/5] Followup fixes for the dwmac and altera lynx conversion

[Maxime Chevallier]

Hello everyone,

Here's yet another version of the cleanup series for the TSE PCS replacement
by PCS Lynx. It includes Kconfig fixups, some missing initialisations
and a slight rework suggested by Russell for the dwmac cleanup sequence,
along with more explicit zeroing of local structures as per MAciej's
review.

V3->V4 :
 - Zero mdio_regmap_config objects
 - Make regmap config more local in dwmac_socfpga
V2->V3 :
 - Fix uninitialized .autoscan field for mdio regmap configuration in
   both altera_tse and dwmac_socfpga
V1->V2 : 
 - Fix a Kconfig inconsistency
 - rework the dwmac_socfpga cleanup sequence

Maxime Chevallier (5):
  net: altera-tse: Initialize local structs before using it
  net: altera_tse: Use the correct Kconfig option for the PCS_LYNX
    dependency
  net: stmmac: make the pcs_lynx cleanup sequence specific to
    dwmac_socfpga
  net: altera_tse: explicitly disable autoscan on the regmap-mdio bus
  net: dwmac_socfpga: initialize local data for mdio regmap
    configuration

 drivers/net/ethernet/altera/Kconfig           |  2 +-
 drivers/net/ethernet/altera/altera_tse_main.c |  3 ++
 drivers/net/ethernet/stmicro/stmmac/common.h  |  1 -
 .../ethernet/stmicro/stmmac/dwmac-socfpga.c   | 29 ++++++++++++++-----
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c |  3 --
 5 files changed, 26 insertions(+), 12 deletions(-)

-- 
2.40.1

[PATCH net-next v4 1/5] net: altera-tse: Initialize local structs before using it

[Maxime Chevallier]

The regmap_config and mdio_regmap_config objects needs to be zeroed before
using them. This will cause spurious errors at probe time as config->pad_bits
is containing random uninitialized data.

Fixes: db48abbaa18e ("net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V3->V4 : Also zero "mrc" from Maciej's review
V2->V3 : No changes
V1->V2 : No changes

 drivers/net/ethernet/altera/altera_tse_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
index d866c0f1b503..215f9fb89c5b 100644
--- a/drivers/net/ethernet/altera/altera_tse_main.c
+++ b/drivers/net/ethernet/altera/altera_tse_main.c
@@ -1255,6 +1255,8 @@ static int altera_tse_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_free_netdev;
 
+	memset(&pcs_regmap_cfg, 0, sizeof(pcs_regmap_cfg));
+	memset(&mrc, 0, sizeof(mrc));
 	/* SGMII PCS address space. The location can vary depending on how the
 	 * IP is integrated. We can have a resource dedicated to it at a specific
 	 * address space, but if it's not the case, we fallback to the mdiophy0
-- 
2.40.1

[PATCH net-next v4 2/5] net: altera_tse: Use the correct Kconfig option for the PCS_LYNX dependency

[Maxime Chevallier]

Use the correct Kconfig dependency for altera_tse as PCS_ALTERA_TSE was
replaced by PCS_LYNX.

Fixes: db48abbaa18e ("net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx")
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V3->V4 : No changes
V2->V3 : Fix a typo in the commit title
V1->V2 : New patch

 drivers/net/ethernet/altera/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/altera/Kconfig b/drivers/net/ethernet/altera/Kconfig
index 93533ba03429..17985319088c 100644
--- a/drivers/net/ethernet/altera/Kconfig
+++ b/drivers/net/ethernet/altera/Kconfig
@@ -4,7 +4,7 @@ config ALTERA_TSE
 	depends on HAS_DMA
 	select PHYLIB
 	select PHYLINK
-	select PCS_ALTERA_TSE
+	select PCS_LYNX
 	select MDIO_REGMAP
 	select REGMAP_MMIO
 	help
-- 
2.40.1

[PATCH net-next v4 3/5] net: stmmac: make the pcs_lynx cleanup sequence specific to dwmac_socfpga

[Maxime Chevallier]

So far, only the dwmac_socfpga variant of stmmac uses PCS Lynx. Use a
dedicated cleanup sequence for dwmac_socfpga instead of using the
generic stmmac one.

Fixes: 5d1f3fe7d2d5 ("net: stmmac: dwmac-sogfpga: use the lynx pcs driver")
Suggested-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V3->V4 : No changes
V2->V3 : Removed extra whiteline
V1->V2 : New patch

 drivers/net/ethernet/stmicro/stmmac/common.h       |  1 -
 .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c    | 14 +++++++++++++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c  |  3 ---
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 52c5ec553276..16e67c18b6f7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -16,7 +16,6 @@
 #include <linux/stmmac.h>
 #include <linux/phy.h>
 #include <linux/pcs/pcs-xpcs.h>
-#include <linux/pcs-lynx.h>
 #include <linux/module.h>
 #if IS_ENABLED(CONFIG_VLAN_8021Q)
 #define STMMAC_VLAN_TAG_USED
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index e399fccbafe5..1fb808be843b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -11,6 +11,7 @@
 #include <linux/phy.h>
 #include <linux/regmap.h>
 #include <linux/mdio/mdio-regmap.h>
+#include <linux/pcs-lynx.h>
 #include <linux/reset.h>
 #include <linux/stmmac.h>
 
@@ -494,6 +495,17 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static void socfpga_dwmac_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct stmmac_priv *priv = netdev_priv(ndev);
+	struct phylink_pcs *pcs = priv->hw->lynx_pcs;
+
+	stmmac_pltfr_remove(pdev);
+
+	lynx_pcs_destroy(pcs);
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int socfpga_dwmac_resume(struct device *dev)
 {
@@ -565,7 +577,7 @@ MODULE_DEVICE_TABLE(of, socfpga_dwmac_match);
 
 static struct platform_driver socfpga_dwmac_driver = {
 	.probe  = socfpga_dwmac_probe,
-	.remove_new = stmmac_pltfr_remove,
+	.remove_new = socfpga_dwmac_remove,
 	.driver = {
 		.name           = "socfpga-dwmac",
 		.pm		= &socfpga_dwmac_pm_ops,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index c784a6731f08..3db1cb0fd160 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -665,9 +665,6 @@ int stmmac_mdio_unregister(struct net_device *ndev)
 	if (priv->hw->xpcs)
 		xpcs_destroy(priv->hw->xpcs);
 
-	if (priv->hw->lynx_pcs)
-		lynx_pcs_destroy(priv->hw->lynx_pcs);
-
 	mdiobus_unregister(priv->mii);
 	priv->mii->priv = NULL;
 	mdiobus_free(priv->mii);
-- 
2.40.1

[PATCH net-next v4 4/5] net: altera_tse: explicitly disable autoscan on the regmap-mdio bus

[Maxime Chevallier]

Set the .autoscan flag to false on the regmap-mdio bus, to avoid using a
random uninitialized value. We don't want autoscan in this case as the
mdio device is a PCS and not a PHY.

Fixes: db48abbaa18e ("net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx")
Suggested-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V3->V4 : No changes
V2->V3 : New patch

 drivers/net/ethernet/altera/altera_tse_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
index 215f9fb89c5b..2e15800e5310 100644
--- a/drivers/net/ethernet/altera/altera_tse_main.c
+++ b/drivers/net/ethernet/altera/altera_tse_main.c
@@ -1288,6 +1288,7 @@ static int altera_tse_probe(struct platform_device *pdev)
 	mrc.regmap = pcs_regmap;
 	mrc.parent = &pdev->dev;
 	mrc.valid_addr = 0x0;
+	mrc.autoscan = false;
 
 	/* Rx IRQ */
 	priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq");
-- 
2.40.1

[PATCH net-next v4 5/5] net: dwmac_socfpga: initialize local data for mdio regmap configuration

[Maxime Chevallier]

Explicitely zero-ize the local mdio_regmap_config data, and explicitely
set the .autoscan parameter, as we only have a PCS on this bus.

Fixes: 5d1f3fe7d2d5 ("net: stmmac: dwmac-sogfpga: use the lynx pcs driver")
Suggested-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
V3->V4 : Move pcs_regmap_cfg into a more local block, and zeroize mrc
V2->V3 : New patch

 .../net/ethernet/stmicro/stmmac/dwmac-socfpga.c   | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 1fb808be843b..6267bcb60206 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -389,7 +389,6 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	struct net_device	*ndev;
 	struct stmmac_priv	*stpriv;
 	const struct socfpga_dwmac_ops *ops;
-	struct regmap_config pcs_regmap_cfg;
 
 	ops = device_get_match_data(&pdev->dev);
 	if (!ops) {
@@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_dvr_remove;
 
-	memset(&pcs_regmap_cfg, 0, sizeof(pcs_regmap_cfg));
-	pcs_regmap_cfg.reg_bits = 16;
-	pcs_regmap_cfg.val_bits = 16;
-	pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
-
 	/* Create a regmap for the PCS so that it can be used by the PCS driver,
 	 * if we have such a PCS
 	 */
 	if (dwmac->tse_pcs_base) {
+		struct regmap_config pcs_regmap_cfg;
 		struct mdio_regmap_config mrc;
 		struct regmap *pcs_regmap;
 		struct mii_bus *pcs_bus;
 
+		memset(&pcs_regmap_cfg, 0, sizeof(pcs_regmap_cfg));
+		memset(&mrc, 0, sizeof(mrc));
+
+		pcs_regmap_cfg.reg_bits = 16;
+		pcs_regmap_cfg.val_bits = 16;
+		pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1);
+
 		pcs_regmap = devm_regmap_init_mmio(&pdev->dev, dwmac->tse_pcs_base,
 						   &pcs_regmap_cfg);
 		if (IS_ERR(pcs_regmap)) {
@@ -470,6 +472,7 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 		mrc.regmap = pcs_regmap;
 		mrc.parent = &pdev->dev;
 		mrc.valid_addr = 0x0;
+		mrc.autoscan = false;
 
 		snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name);
 		pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc);
-- 
2.40.1

Re: [PATCH net-next v4 5/5] net: dwmac_socfpga: initialize local data for mdio regmap configuration

[Russell King (Oracle)]

On Wed, Jun 07, 2023 at 04:54:09PM +0200, Maxime Chevallier wrote:
> On Wed, 7 Jun 2023 13:28:03 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> > > @@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
> > >  		struct mdio_regmap_config mrc;
> > >  		struct regmap *pcs_regmap;
> > >  		struct mii_bus *pcs_bus;
> > >    
> > ...
> > > +		memset(&mrc, 0, sizeof(mrc));  
> > ...
> > >  		mrc.parent = &pdev->dev;
> > >  		mrc.valid_addr = 0x0;
> > > +		mrc.autoscan = false;  
> > 
> > Isn't this covered by the memset() ?
> 
> I have the same answer as for the above. It's redundant, but I don't
> think there's any harm having it set explicitely ?

No harm, just redundant. I don't think this is a good enough reason not
to merge it.

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

Re: [PATCH net-next v4 0/5] Followup fixes for the dwmac and altera lynx conversion

[Russell King (Oracle)]

On Wed, Jun 07, 2023 at 03:59:36PM +0200, Maxime Chevallier wrote:
> Hello everyone,
> 
> Here's yet another version of the cleanup series for the TSE PCS replacement
> by PCS Lynx. It includes Kconfig fixups, some missing initialisations
> and a slight rework suggested by Russell for the dwmac cleanup sequence,
> along with more explicit zeroing of local structures as per MAciej's
> review.

For the series, which brings an immediate fix to the problems people
are noticing:

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

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 net-next v4 4/5] net: altera_tse: explicitly disable autoscan on the regmap-mdio bus

[Maxime Chevallier]

Hello Russell,

On Wed, 7 Jun 2023 13:26:41 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Wed, Jun 07, 2023 at 03:59:40PM +0200, Maxime Chevallier wrote:
> > Set the .autoscan flag to false on the regmap-mdio bus, to avoid using a
> > random uninitialized value. We don't want autoscan in this case as the
> > mdio device is a PCS and not a PHY.  
> 
> Isn't this now covered by patch 1's memset of mrc?
> 

Yes it is, however I thought it could be fine keeping it set explicitely
anyway, as we do have a PCS on that bus and we don't want any autoscan
happening. Since these two drivers are the first users of mdio_regmap,
my hope is that we could get these reference usages covering all fields.

Should I drop this ?

Maxime

Re: [PATCH net-next v4 5/5] net: dwmac_socfpga: initialize local data for mdio regmap configuration

[Maxime Chevallier]

On Wed, 7 Jun 2023 13:28:03 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> > @@ -447,19 +446,22 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
> >  		struct mdio_regmap_config mrc;
> >  		struct regmap *pcs_regmap;
> >  		struct mii_bus *pcs_bus;
> >    
> ...
> > +		memset(&mrc, 0, sizeof(mrc));  
> ...
> >  		mrc.parent = &pdev->dev;
> >  		mrc.valid_addr = 0x0;
> > +		mrc.autoscan = false;  
> 
> Isn't this covered by the memset() ?

I have the same answer as for the above. It's redundant, but I don't
think there's any harm having it set explicitely ?

Maxime

Re: [PATCH net-next v4 5/5] net: dwmac_socfpga: initialize local data for mdio regmap configuration

[Simon Horman]

On Wed, Jun 07, 2023 at 03:59:41PM +0200, Maxime Chevallier wrote:
> Explicitely zero-ize the local mdio_regmap_config data, and explicitely

nit: Explicitely -> explicitly

Re: [PATCH net-next v4 0/5] Followup fixes for the dwmac and altera lynx conversion

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  7 Jun 2023 15:59:36 +0200 you wrote:
> Hello everyone,
> 
> Here's yet another version of the cleanup series for the TSE PCS replacement
> by PCS Lynx. It includes Kconfig fixups, some missing initialisations
> and a slight rework suggested by Russell for the dwmac cleanup sequence,
> along with more explicit zeroing of local structures as per MAciej's
> review.
> 
> [...]

Here is the summary with links:
  - [net-next,v4,1/5] net: altera-tse: Initialize local structs before using it
    https://git.kernel.org/netdev/net-next/c/2d830f7a4134
  - [net-next,v4,2/5] net: altera_tse: Use the correct Kconfig option for the PCS_LYNX dependency
    https://git.kernel.org/netdev/net-next/c/fae555f5a56f
  - [net-next,v4,3/5] net: stmmac: make the pcs_lynx cleanup sequence specific to dwmac_socfpga
    https://git.kernel.org/netdev/net-next/c/a8dd7404c214
  - [net-next,v4,4/5] net: altera_tse: explicitly disable autoscan on the regmap-mdio bus
    https://git.kernel.org/netdev/net-next/c/fa19a5d9dcff
  - [net-next,v4,5/5] net: dwmac_socfpga: initialize local data for mdio regmap configuration
    https://git.kernel.org/netdev/net-next/c/06b9dede1e7d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html