| Message ID | 20240123215606.26716-9-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: variants to drivers, interfaces to a common module | expand |
On Tue, Jan 23, 2024 at 06:56:00PM -0300, Luiz Angelo Daros de Luca wrote: > The line assigning dev.of_node in mdio_bus has been removed since the > subsequent of_mdiobus_register will always overwrite it. Please use present tense and imperative mood. "Remove the line assigning dev.of_node, because ...". > > ds->user_mii_bus is not assigned anymore[1]. "As discussed in [1], allow the DSA core to be simplified, by not assigning ds->user_mii_bus when the MDIO bus is described in OF, as it is unnecessary." > It should work as before as long as the switch ports have a valid > phy-handle property. > > Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes > to mdiobus"), we can put the "mdio" node just after the MDIO bus > registration. > The switch unregistration was moved into realtek_common_remove() as > both interfaces now use the same code path. Hopefully you can sort this part out in a separate patch that's unrelated to the user_mii_bus cleanup, ideally in "net: dsa: realtek: common rtl83xx module". > > [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > drivers/net/dsa/realtek/realtek-mdio.c | 5 ----- > drivers/net/dsa/realtek/realtek-smi.c | 15 ++------------- > drivers/net/dsa/realtek/rtl83xx.c | 2 ++ > 3 files changed, 4 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > index 0171185ec665..c75b4550802c 100644 > --- a/drivers/net/dsa/realtek/realtek-mdio.c > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > @@ -158,11 +158,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev) > { > struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); > > - if (!priv) > - return; > - The way I would structure these guards is I would keep them here, and not in rtl83xx_remove() and rtl83xx_shutdown(). Then I would make sure that rtl83xx_remove() is the exact opposite of just rtl83xx_probe(), and rtl83xx_unregister_switch() is the exact opposite of just rtl83xx_register_switch(). And I would fix the error handling path of realtek_smi_probe() and realtek_mdio_probe() to call rtl83xx_remove() when rtl83xx_register_switch() fails. > - dsa_unregister_switch(priv->ds); > - > rtl83xx_remove(priv); > } > EXPORT_SYMBOL_NS_GPL(realtek_mdio_remove, REALTEK_DSA); > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c > index 0ccb2a6059a6..a89813e527d2 100644 > --- a/drivers/net/dsa/realtek/realtek-smi.c > +++ b/drivers/net/dsa/realtek/realtek-smi.c > @@ -331,7 +331,7 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) > { > struct realtek_priv *priv = ds->priv; > struct device_node *mdio_np; > - int ret; > + int ret = 0; > > mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio"); > if (!mdio_np) { > @@ -344,15 +344,14 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) > ret = -ENOMEM; > goto err_put_node; > } > + > priv->user_mii_bus->priv = priv; > priv->user_mii_bus->name = "SMI user MII"; > priv->user_mii_bus->read = realtek_smi_mdio_read; > priv->user_mii_bus->write = realtek_smi_mdio_write; > snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "SMI-%d", > ds->index); > - priv->user_mii_bus->dev.of_node = mdio_np; > priv->user_mii_bus->parent = priv->dev; > - ds->user_mii_bus = priv->user_mii_bus; > > ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np); > if (ret) { > @@ -361,8 +360,6 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) > goto err_put_node; > } > > - return 0; > - > err_put_node: > of_node_put(mdio_np); > > @@ -434,14 +431,6 @@ void realtek_smi_remove(struct platform_device *pdev) > { > struct realtek_priv *priv = platform_get_drvdata(pdev); > > - if (!priv) > - return; > - > - dsa_unregister_switch(priv->ds); > - > - if (priv->user_mii_bus) > - of_node_put(priv->user_mii_bus->dev.of_node); > - > rtl83xx_remove(priv); > } > EXPORT_SYMBOL_NS_GPL(realtek_smi_remove, REALTEK_DSA); > diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c > index 3d07c5662fa4..53bacbacc82e 100644 > --- a/drivers/net/dsa/realtek/rtl83xx.c > +++ b/drivers/net/dsa/realtek/rtl83xx.c > @@ -190,6 +190,8 @@ void rtl83xx_remove(struct realtek_priv *priv) > if (!priv) > return; > > + dsa_unregister_switch(priv->ds); > + > /* leave the device reset asserted */ > if (priv->reset) > gpiod_set_value(priv->reset, 1); > -- > 2.43.0 >
> On Tue, Jan 23, 2024 at 06:56:00PM -0300, Luiz Angelo Daros de Luca wrote: > > The line assigning dev.of_node in mdio_bus has been removed since the > > subsequent of_mdiobus_register will always overwrite it. > > Please use present tense and imperative mood. "Remove the line assigning > dev.of_node, because ...". OK > > > > ds->user_mii_bus is not assigned anymore[1]. > > "As discussed in [1], allow the DSA core to be simplified, by not > assigning ds->user_mii_bus when the MDIO bus is described in OF, as it > is unnecessary." OK > > It should work as before as long as the switch ports have a valid > > phy-handle property. > > > > Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes > > to mdiobus"), we can put the "mdio" node just after the MDIO bus > > registration. > > > The switch unregistration was moved into realtek_common_remove() as > > both interfaces now use the same code path. > > Hopefully you can sort this part out in a separate patch that's > unrelated to the user_mii_bus cleanup, ideally in "net: dsa: realtek: > common rtl83xx module". Yes. With the introduction of rtl83xx_unregister_switch, this part is gone. > > > > [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u > > > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > --- > > drivers/net/dsa/realtek/realtek-mdio.c | 5 ----- > > drivers/net/dsa/realtek/realtek-smi.c | 15 ++------------- > > drivers/net/dsa/realtek/rtl83xx.c | 2 ++ > > 3 files changed, 4 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > > index 0171185ec665..c75b4550802c 100644 > > --- a/drivers/net/dsa/realtek/realtek-mdio.c > > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > > @@ -158,11 +158,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev) > > { > > struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); > > > > - if (!priv) > > - return; > > - > > The way I would structure these guards is I would keep them here, and > not in rtl83xx_remove() and rtl83xx_shutdown(). Then I would make sure > that rtl83xx_remove() is the exact opposite of just rtl83xx_probe(), and > rtl83xx_unregister_switch() is the exact opposite of just rtl83xx_register_switch(). It looks like it is now, although "remove" mostly leaves the job for devm. I'm still not sure if we have the correct shutdown/remove code. From what I could understand, driver shutdown is called during system shutdown while remove is called when the driver is removed. However, it looks like that both might be called in sequence. Would it be shutdown,remove? (it's probably that because there is the dev_set_drvdata(priv->dev, NULL) in shutdown). However, if shutdown should prepare the system for another OS, I believe it should be asserting the hw reset as well or remove should stop doing it. Are the dsa_switch_shutdown and dsa_switch_unregister enough to prevent leaking traffic after the driver is gone? It does disable all ports. Or should we have a fallback "isolate all ports" when a hw reset is missing? I guess the u-boot driver does something like that. I don't think it is mandatory for this series but if we got something wrong, it would be nice to fix it. > And I would fix the error handling path of realtek_smi_probe() and > realtek_mdio_probe() to call rtl83xx_remove() when rtl83xx_register_switch() > fails. With the rtl83xx_unregister_switch, it was kept in realtek_smi_probe() and realtek_mdio_probe. Regards, Luiz
On Sun, Jan 28, 2024 at 11:12:25PM -0300, Luiz Angelo Daros de Luca wrote: > It looks like it is now, although "remove" mostly leaves the job for devm. > > I'm still not sure if we have the correct shutdown/remove code. From > what I could understand, driver shutdown is called during system > shutdown while remove is called when the driver is removed. Yeah, poweroff or reboot or kexec. > However, it looks like that both might be called in sequence. Would it > be shutdown,remove? (it's probably that because there is the > dev_set_drvdata(priv->dev, NULL) in shutdown). Yeah, while shutting down, the (SPI, I2C, MDIO, ...) bus driver might call spi_unregister_controller() on shutdown(), and this will also call remove() on its child devices. Even the Raspberry Pi SPI controller does this, AFAIR. The idea of implementing .shutdown() as .remove() is to gain more code coverage by sharing code, which should reduce chances of bugs in less-tested code (remove). Or at least that's how the saying goes... > However, if shutdown should prepare the system for another OS, I > believe it should be asserting the hw reset as well or remove should > stop doing it. Are the dsa_switch_shutdown and dsa_switch_unregister > enough to prevent leaking traffic after the driver is gone? It does > disable all ports. Or should we have a fallback "isolate all ports" > when a hw reset is missing? I guess the u-boot driver does something > like that. > > I don't think it is mandatory for this series but if we got something > wrong, it would be nice to fix it. I don't really know anything at all about kexec. You might want to get input from someone who uses it. All that I know is that this should do something meaningful (not crash, and still work in the second kernel): root@debian:~# kexec -l /boot/Image.gz --reuse-cmdline && kexec -e [ 46.335430] mscc_felix 0000:00:00.5 swp3: Link is Down [ 46.345747] fsl_enetc 0000:00:00.2 eno2: Link is Down [ 46.419201] kvm: exiting hardware virtualization [ 46.424036] kexec_core: Starting new kernel [ 46.471657] psci: CPU1 killed (polled 0 ms) [ 46.486060] Bye! [ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd083] [ 0.000000] Linux version 5.16.0-rc2-07010-ga9b9500ffaac-dirty (tigrisor@skbuf) (aarch64-none-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 10.2-2020.11 (arm-10.16)) 10.2.1 20201103, GNU ld (GNU Toolchain for the A-profile Architecture 10.2-2020.11 (arm-10.16)) 2.35.1.20201028) #1519 SMP PREEMPT Wed Dec 1 08:59:13 EET 2021 [ 0.000000] Machine model: LS1028A RDB Board [ 0.000000] earlycon: uart8250 at MMIO 0x00000000021c0500 (options '') [ 0.000000] printk: bootconsole [uart8250] enabled [ 0.000000] efi: UEFI not found. [ 0.000000] NUMA: No NUMA configuration found [ 0.000000] NUMA: Faking a node at [mem 0x0000000080000000-0x00000020ffffffff] [ 0.000000] NUMA: NODE_DATA [mem 0x20ff6fab80-0x20ff6fcfff] (...) which in this case it does. From other discussions I've had, there seems to be interest in quite the opposite thing, in fact. Reboot the SoC running Linux, but do not disturb traffic flowing through the switch, and somehow pick up the state from where the previous kernel left it. Now, obviously that doesn't currently work, but it does raise the question about the usefulness of resetting the switch on shutdown.
On 1/29/2024 8:15 AM, Vladimir Oltean wrote: > On Sun, Jan 28, 2024 at 11:12:25PM -0300, Luiz Angelo Daros de Luca wrote: >> It looks like it is now, although "remove" mostly leaves the job for devm. >> >> I'm still not sure if we have the correct shutdown/remove code. From >> what I could understand, driver shutdown is called during system >> shutdown while remove is called when the driver is removed. > > Yeah, poweroff or reboot or kexec. > >> However, it looks like that both might be called in sequence. Would it >> be shutdown,remove? (it's probably that because there is the >> dev_set_drvdata(priv->dev, NULL) in shutdown). > > Yeah, while shutting down, the (SPI, I2C, MDIO, ...) bus driver might > call spi_unregister_controller() on shutdown(), and this will also call > remove() on its child devices. Even the Raspberry Pi SPI controller does > this, AFAIR. The idea of implementing .shutdown() as .remove() is to > gain more code coverage by sharing code, which should reduce chances of > bugs in less-tested code (remove). Or at least that's how the saying goes... > >> However, if shutdown should prepare the system for another OS, I >> believe it should be asserting the hw reset as well or remove should >> stop doing it. Are the dsa_switch_shutdown and dsa_switch_unregister >> enough to prevent leaking traffic after the driver is gone? It does >> disable all ports. Or should we have a fallback "isolate all ports" >> when a hw reset is missing? I guess the u-boot driver does something >> like that. >> >> I don't think it is mandatory for this series but if we got something >> wrong, it would be nice to fix it. > > I don't really know anything at all about kexec. You might want to get > input from someone who uses it. All that I know is that this should do > something meaningful (not crash, and still work in the second kernel): > > root@debian:~# kexec -l /boot/Image.gz --reuse-cmdline && kexec -e > [ 46.335430] mscc_felix 0000:00:00.5 swp3: Link is Down > [ 46.345747] fsl_enetc 0000:00:00.2 eno2: Link is Down > [ 46.419201] kvm: exiting hardware virtualization > [ 46.424036] kexec_core: Starting new kernel > [ 46.471657] psci: CPU1 killed (polled 0 ms) > [ 46.486060] Bye! > [ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd083] > [ 0.000000] Linux version 5.16.0-rc2-07010-ga9b9500ffaac-dirty (tigrisor@skbuf) (aarch64-none-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture 10.2-2020.11 (arm-10.16)) 10.2.1 20201103, GNU ld (GNU Toolchain for the A-profile Architecture 10.2-2020.11 (arm-10.16)) 2.35.1.20201028) #1519 SMP PREEMPT Wed Dec 1 08:59:13 EET 2021 > [ 0.000000] Machine model: LS1028A RDB Board > [ 0.000000] earlycon: uart8250 at MMIO 0x00000000021c0500 (options '') > [ 0.000000] printk: bootconsole [uart8250] enabled > [ 0.000000] efi: UEFI not found. > [ 0.000000] NUMA: No NUMA configuration found > [ 0.000000] NUMA: Faking a node at [mem 0x0000000080000000-0x00000020ffffffff] > [ 0.000000] NUMA: NODE_DATA [mem 0x20ff6fab80-0x20ff6fcfff] > (...) > > which in this case it does. > > From other discussions I've had, there seems to be interest in quite the > opposite thing, in fact. Reboot the SoC running Linux, but do not > disturb traffic flowing through the switch, and somehow pick up the > state from where the previous kernel left it. Yes this is actually an use case that is very dear to the users of DSA in an airplane. The entertainment system in the seat in front of you typically has a left, CPU/display and right set of switch ports. Across the 300+ units in the plane each entertainment systems runs STP to avoid loops being created when one of the display units goes bad. Occasionally cabin crew members will have to swap those units out since they tend to wear out. When they do, the switch operates in a headless mode and it would be unfortunate that plugging in a display unit into the network again would be disrupting existing traffic. I have seen out of tree patches doing that, but there was not a good way to make them upstream quality. > > Now, obviously that doesn't currently work, but it does raise the > question about the usefulness of resetting the switch on shutdown. The users would really care would likely introduce sufficient amounts of control knobs (module parameters, ethtool private flags, devlink?) to control that behavior. It does seem however universally acceptable to stop any DMAs and packets from flowing as a default and safe implementation to the upstream kernel.
On Mon, Jan 29, 2024 at 08:22:47AM -0800, Florian Fainelli wrote: > It does seem however universally acceptable to stop any DMAs and > packets from flowing as a default and safe implementation to the > upstream kernel. DMA I can understand, because you wouldn't want the hardware to notify you of a buffer you have no idea about. But DSA doesn't assume that switches have DMA, and generally speaking, stopping the offloaded traffic path seems unnecessary. It will be stopped when the new kernel sets up the interfaces as standalone, renegotiates the link, etc.
On 1/29/2024 8:43 AM, Vladimir Oltean wrote: > On Mon, Jan 29, 2024 at 08:22:47AM -0800, Florian Fainelli wrote: >> It does seem however universally acceptable to stop any DMAs and >> packets from flowing as a default and safe implementation to the >> upstream kernel. > > DMA I can understand, because you wouldn't want the hardware to notify > you of a buffer you have no idea about. But DSA doesn't assume that > switches have DMA, and generally speaking, stopping the offloaded > traffic path seems unnecessary. It will be stopped when the new kernel > sets up the interfaces as standalone, renegotiates the link, etc. Very true, I suppose most driver writers might want to stop the packet flow from the source however and simply disable switch ports. There are also power management considerations at least there were for some of the products I worked with.
On 29.01.2024 19:22, Florian Fainelli wrote: > > > On 1/29/2024 8:15 AM, Vladimir Oltean wrote: >> From other discussions I've had, there seems to be interest in quite the >> opposite thing, in fact. Reboot the SoC running Linux, but do not >> disturb traffic flowing through the switch, and somehow pick up the >> state from where the previous kernel left it. > > Yes this is actually an use case that is very dear to the users of DSA in an airplane. The entertainment system in the seat in front of you typically has a left, CPU/display and right set of switch ports. Across the 300+ units in the plane each entertainment systems runs STP to avoid loops being created when one of the display units goes bad. Occasionally cabin crew members will have to swap those units out since they tend to wear out. When they do, the switch operates in a headless mode and it would be unfortunate that plugging in a display unit into the network again would be disrupting existing traffic. I have seen out of tree patches doing that, but there was not a good way to make them upstream quality. This piqued my interest. I'm trying to understand how exactly plugging in a display unit into the network would disrupt the traffic flow. Is this about all network interfaces attached to the bridge interface being blocked when a new link is established to relearn the changed topology? Arınç
On Tue, Jan 30, 2024 at 05:40:26PM +0300, Arınç ÜNAL wrote: > On 29.01.2024 19:22, Florian Fainelli wrote: > > > > > > On 1/29/2024 8:15 AM, Vladimir Oltean wrote: > > > From other discussions I've had, there seems to be interest in quite the > > > opposite thing, in fact. Reboot the SoC running Linux, but do not > > > disturb traffic flowing through the switch, and somehow pick up the > > > state from where the previous kernel left it. > > > > Yes this is actually an use case that is very dear to the users of DSA in an airplane. The entertainment system in the seat in front of you typically has a left, CPU/display and right set of switch ports. Across the 300+ units in the plane each entertainment systems runs STP to avoid loops being created when one of the display units goes bad. Occasionally cabin crew members will have to swap those units out since they tend to wear out. When they do, the switch operates in a headless mode and it would be unfortunate that plugging in a display unit into the network again would be disrupting existing traffic. I have seen out of tree patches doing that, but there was not a good way to make them upstream quality. > > This piqued my interest. I'm trying to understand how exactly plugging in a > display unit into the network would disrupt the traffic flow. Is this about > all network interfaces attached to the bridge interface being blocked when > a new link is established to relearn the changed topology? The hardware is split into two parts, a cradle and the display unit. The switch itself is in the cradle embedded in the seat back. The display unit contains the CPU, GPU, storage etc. There is a single Ethernet interface between the display unit and the cradle, along with MDIO, power, audio cables for the headphone jack etc. When you take out the display unit, you disconnect the switches management plain. The CPU has gone, and its the CPU running STP, sending and receiving BPDUs, etc. But the switch is still powered, and switching packets, keeping the network going, at least for a while. When you plug in a display unit, it boots. As typical for any computer booting, it assumes the hardware is in an unknown state, and hits the switch with a reset. That then kills the local networking, and it takes a little while of the devices around it to swap to a redundant path. The move from STP to RSTP has been made, which speeds this all up, but you do get some disruption. It can take a while for the display unit to boot into user space and reconfigure the switch. Its only when that is complete can the switch rejoin the network. Rather than hit the switch with a reset, it would be better to somehow suck the current configuration out of the switch and prime the Linux network stack with that configuration. But that is a totally alien concept to Linux. Andrew
> > > > From other discussions I've had, there seems to be interest in quite the > > > > opposite thing, in fact. Reboot the SoC running Linux, but do not > > > > disturb traffic flowing through the switch, and somehow pick up the > > > > state from where the previous kernel left it. > > > > > > Yes this is actually an use case that is very dear to the users of DSA in an airplane. The entertainment system in the seat in front of you typically has a left, CPU/display and right set of switch ports. Across the 300+ units in the plane each entertainment systems runs STP to avoid loops being created when one of the display units goes bad. Occasionally cabin crew members will have to swap those units out since they tend to wear out. When they do, the switch operates in a headless mode and it would be unfortunate that plugging in a display unit into the network again would be disrupting existing traffic. I have seen out of tree patches doing that, but there was not a good way to make them upstream quality. > > > > This piqued my interest. I'm trying to understand how exactly plugging in a > > display unit into the network would disrupt the traffic flow. Is this about > > all network interfaces attached to the bridge interface being blocked when > > a new link is established to relearn the changed topology? > > The hardware is split into two parts, a cradle and the display > unit. The switch itself is in the cradle embedded in the seat > back. The display unit contains the CPU, GPU, storage etc. There is a > single Ethernet interface between the display unit and the cradle, > along with MDIO, power, audio cables for the headphone jack etc. > > When you take out the display unit, you disconnect the switches > management plain. The CPU has gone, and its the CPU running STP, > sending and receiving BPDUs, etc. But the switch is still powered, and > switching packets, keeping the network going, at least for a while. > > When you plug in a display unit, it boots. As typical for any computer > booting, it assumes the hardware is in an unknown state, and hits the > switch with a reset. That then kills the local networking, and it > takes a little while of the devices around it to swap to a redundant > path. The move from STP to RSTP has been made, which speeds this all > up, but you do get some disruption. > > It can take a while for the display unit to boot into user space and > reconfigure the switch. Its only when that is complete can the switch > rejoin the network. > > Rather than hit the switch with a reset, it would be better to somehow > suck the current configuration out of the switch and prime the Linux > network stack with that configuration. But that is a totally alien > concept to Linux. This is quite a particular case. You'll need to update userland config from the kernel state and the kernel state from the HW state. It's upside down from what we normally see. Anyway, we are far from that point in realtek DSA drivers. The DSA driver actually resets the switch twice (HW and then SW) during setup. Even vendor driver/lib states that without the initialization steps, the switch behavior is undefined. And those steps would probably mess with any existing switch state. Parsing the reg values into kernel state is quite a complex task if you need to be usable for many scenarios. And we still have opaque jam tables in the driver that I would love to get rid of. Now, back to the point I raised: 1) should we continue to HW reset the switch when the driver stops controlling it? 2) If that is a yes, should we do that both for shutdown and poweroff? 3) Should we take additional precautions to lock the switch before resetting it (as HW reset might be missing or misconfigured)? I'll probably send the v5 without touching this topic but it is an interesting point to think about, at least to assert the reset during shutdown. Regards, Luiz
diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c index 0171185ec665..c75b4550802c 100644 --- a/drivers/net/dsa/realtek/realtek-mdio.c +++ b/drivers/net/dsa/realtek/realtek-mdio.c @@ -158,11 +158,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev) { struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); - if (!priv) - return; - - dsa_unregister_switch(priv->ds); - rtl83xx_remove(priv); } EXPORT_SYMBOL_NS_GPL(realtek_mdio_remove, REALTEK_DSA); diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c index 0ccb2a6059a6..a89813e527d2 100644 --- a/drivers/net/dsa/realtek/realtek-smi.c +++ b/drivers/net/dsa/realtek/realtek-smi.c @@ -331,7 +331,7 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) { struct realtek_priv *priv = ds->priv; struct device_node *mdio_np; - int ret; + int ret = 0; mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio"); if (!mdio_np) { @@ -344,15 +344,14 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) ret = -ENOMEM; goto err_put_node; } + priv->user_mii_bus->priv = priv; priv->user_mii_bus->name = "SMI user MII"; priv->user_mii_bus->read = realtek_smi_mdio_read; priv->user_mii_bus->write = realtek_smi_mdio_write; snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "SMI-%d", ds->index); - priv->user_mii_bus->dev.of_node = mdio_np; priv->user_mii_bus->parent = priv->dev; - ds->user_mii_bus = priv->user_mii_bus; ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np); if (ret) { @@ -361,8 +360,6 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) goto err_put_node; } - return 0; - err_put_node: of_node_put(mdio_np); @@ -434,14 +431,6 @@ void realtek_smi_remove(struct platform_device *pdev) { struct realtek_priv *priv = platform_get_drvdata(pdev); - if (!priv) - return; - - dsa_unregister_switch(priv->ds); - - if (priv->user_mii_bus) - of_node_put(priv->user_mii_bus->dev.of_node); - rtl83xx_remove(priv); } EXPORT_SYMBOL_NS_GPL(realtek_smi_remove, REALTEK_DSA); diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c index 3d07c5662fa4..53bacbacc82e 100644 --- a/drivers/net/dsa/realtek/rtl83xx.c +++ b/drivers/net/dsa/realtek/rtl83xx.c @@ -190,6 +190,8 @@ void rtl83xx_remove(struct realtek_priv *priv) if (!priv) return; + dsa_unregister_switch(priv->ds); + /* leave the device reset asserted */ if (priv->reset) gpiod_set_value(priv->reset, 1);
The line assigning dev.of_node in mdio_bus has been removed since the subsequent of_mdiobus_register will always overwrite it. ds->user_mii_bus is not assigned anymore[1]. It should work as before as long as the switch ports have a valid phy-handle property. Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes to mdiobus"), we can put the "mdio" node just after the MDIO bus registration. The switch unregistration was moved into realtek_common_remove() as both interfaces now use the same code path. [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- drivers/net/dsa/realtek/realtek-mdio.c | 5 ----- drivers/net/dsa/realtek/realtek-smi.c | 15 ++------------- drivers/net/dsa/realtek/rtl83xx.c | 2 ++ 3 files changed, 4 insertions(+), 18 deletions(-)