Hello everyone,
Here's 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.
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 the regmap_config struct 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: explicitly disable autoscan on the regmap-mdio bus
drivers/net/ethernet/altera/Kconfig | 2 +-
drivers/net/ethernet/altera/altera_tse_main.c | 2 ++
drivers/net/ethernet/stmicro/stmmac/common.h | 1 -
.../net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 15 ++++++++++++++-
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 3 ---
5 files changed, 17 insertions(+), 6 deletions(-)
--
2.40.1
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) <[email protected]>
Signed-off-by: Maxime Chevallier <[email protected]>
---
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 df509abcd378..b0cb94fe6247 100644
--- a/drivers/net/ethernet/altera/altera_tse_main.c
+++ b/drivers/net/ethernet/altera/altera_tse_main.c
@@ -1287,6 +1287,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
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: 5d1f3fe7d2d5 ("net: stmmac: dwmac-sogfpga: use the lynx pcs driver")
Suggested-by: Russell King (Oracle) <[email protected]>
Signed-off-by: Maxime Chevallier <[email protected]>
---
V2->V3 : New patch
drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index 1fb808be843b..e1bdf132cede 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -470,6 +470,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
On Tue, Jun 06, 2023 at 05:24:56PM +0200, Maxime Chevallier wrote:
> Hello everyone,
>
> Here's 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.
Thanks, this is getting there, but now you've now made me read
altera_tse.c, and it suffers the same issue that dwmac-socfpga.c does:
ret = register_netdev(ndev);
...
priv->pcs = lynx_pcs_create_mdiodev(pcs_bus, 0);
...
priv->phylink = phylink_create(&priv->phylink_config,
This means you're publishing before you've finished setup - which is
a racy thing to do, especially if the driver is a module.
Let's think about what could happen. register_netdev() adds the network
device to the net layer and publishes it to userspace. Userspace notices
a new network interface and configures it, causing tse_open() to be
called. However, priv->phylink has not yet been initialised.
tse_open() then does:
ret = phylink_of_phy_connect(priv->phylink, priv->device->of_node, 0);
and phylink_of_phy_connect() attempts to dereference it's first
argument, resulting in a NULL pointer dereference. Even if that doesn't
get you, then:
phylink_start(priv->phylink);
will.
Golden rule: setup everything you need first, and only once that's
complete, publish. If you publish before you've completed setup, then
you're giving permission for other stuff to immediately start making
use of what you've published, which may occur before the remainder of
the initialisation has completed.
Lastly, remember that phylink_start() can result in the link coming up
_immediately_ (that means mac_link_up() could be called before it's
returned), so I would hope that the Altera TSE driver is prepared
for that to happen before napi, queues, and rx dma are ready.
Not saying that there's anything wrong with this series (there isn't),
merely that there's more issues that ought to be resolved.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Jun 06, 2023 at 05:25:00PM +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.
>
> Fixes: db48abbaa18e ("net: ethernet: altera-tse: Convert to mdio-regmap and use PCS Lynx")
> Suggested-by: Russell King (Oracle) <[email protected]>
> Signed-off-by: Maxime Chevallier <[email protected]>
> ---
> 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 df509abcd378..b0cb94fe6247 100644
> --- a/drivers/net/ethernet/altera/altera_tse_main.c
> +++ b/drivers/net/ethernet/altera/altera_tse_main.c
> @@ -1287,6 +1287,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;
ah so there was uninited value on mrc :) can you please zero this out in
one of the patches?
>
> /* Rx IRQ */
> priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq");
> --
> 2.40.1
>
>