| Message ID | 20221117125918.203997-1-liujian56@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4305fe232b8aa59af3761adc9fe6b6aa40913960 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: sparx5: fix error handling in sparx5_port_open() | expand |
On Thu, Nov 17, 2022 at 08:59:18PM +0800, Liu Jian wrote: > If phylink_of_phy_connect() fails, the port should be disabled. > If sparx5_serdes_set()/phy_power_on() fails, the port should be > disabled and the phylink should be stopped and disconnected. > > Fixes: 946e7fd5053a ("net: sparx5: add port module support") > Fixes: f3cad2611a77 ("net: sparx5: add hostmode with phylink support") > Signed-off-by: Liu Jian <liujian56@huawei.com> The patch looks sane for the code structure that's there, but I question whether this is the best code structure. phylink_start() will call the pcs_config() method, which then goes on to call sparx5_port_pcs_set() and sparx5_port_pcs_low_set() - which then calls sparx5_serdes_set(). Is that safe with the serdes PHY powered down? I think sparx5 maintainers need to think about that, and possibly include a comment in the code if it is indeed safe.
Hi Liu and Russell, Yes, I think we should go over this and do some testing on the platform before taking it in. On Thu, 2022-11-17 at 13:15 +0000, Russell King (Oracle) wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Thu, Nov 17, 2022 at 08:59:18PM +0800, Liu Jian wrote: > > If phylink_of_phy_connect() fails, the port should be disabled. > > If sparx5_serdes_set()/phy_power_on() fails, the port should be > > disabled and the phylink should be stopped and disconnected. > > > > Fixes: 946e7fd5053a ("net: sparx5: add port module support") > > Fixes: f3cad2611a77 ("net: sparx5: add hostmode with phylink support") > > Signed-off-by: Liu Jian <liujian56@huawei.com> > > The patch looks sane for the code structure that's there, but I question > whether this is the best code structure. > > phylink_start() will call the pcs_config() method, which then goes on > to call sparx5_port_pcs_set() and sparx5_port_pcs_low_set() - which > then calls sparx5_serdes_set(). Is that safe with the serdes PHY > powered down? I think sparx5 maintainers need to think about that, > and possibly include a comment in the code if it is indeed safe. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! BR Steen
Hi Liu, Thanks for your patch, the changes look good to me. We also did a round of testing on the platform, simulating errors to see this in effect. On Thu, 2022-11-17 at 20:59 +0800, Liu Jian wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > If phylink_of_phy_connect() fails, the port should be disabled. > If sparx5_serdes_set()/phy_power_on() fails, the port should be > disabled and the phylink should be stopped and disconnected. > > Fixes: 946e7fd5053a ("net: sparx5: add port module support") > Fixes: f3cad2611a77 ("net: sparx5: add hostmode with phylink support") > Signed-off-by: Liu Jian <liujian56@huawei.com> > --- > .../net/ethernet/microchip/sparx5/sparx5_netdev.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c > b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c > index 19516ccad533..d078156581d5 100644 > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c > @@ -104,7 +104,7 @@ static int sparx5_port_open(struct net_device *ndev) > err = phylink_of_phy_connect(port->phylink, port->of_node, 0); > if (err) { > netdev_err(ndev, "Could not attach to PHY\n"); > - return err; > + goto err_connect; > } > > phylink_start(port->phylink); > @@ -116,10 +116,20 @@ static int sparx5_port_open(struct net_device *ndev) > err = sparx5_serdes_set(port->sparx5, port, &port- > >conf); > else > err = phy_power_on(port->serdes); > - if (err) > + if (err) { > netdev_err(ndev, "%s failed\n", __func__); > + goto out_power; > + } > } > > + return 0; > + > +out_power: > + phylink_stop(port->phylink); > + phylink_disconnect_phy(port->phylink); > +err_connect: > + sparx5_port_enable(port, false); > + > return err; > } > > -- > 2.17.1 > Tested-off-by: Bjarni Jonasson <bjarni.jonasson@microchip.com> Reviewed-off-by: Lars Povlsen <steen.hegelund@microchip.com> BR Steen
Hi Liu, Oops, Incorrect tagging... Thanks for your patch, the changes look good to me. We also did a round of testing on the platform, simulating errors to see this in effect. On Tue, 2022-11-22 at 10:28 +0100, Steen Hegelund wrote: > Hi Liu, > > Thanks for your patch, the changes look good to me. We also did a round of > testing on the platform, simulating errors to see this in effect. > > On Thu, 2022-11-17 at 20:59 +0800, Liu Jian wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > If phylink_of_phy_connect() fails, the port should be disabled. > > If sparx5_serdes_set()/phy_power_on() fails, the port should be > > disabled and the phylink should be stopped and disconnected. > > > > Fixes: 946e7fd5053a ("net: sparx5: add port module support") > > Fixes: f3cad2611a77 ("net: sparx5: add hostmode with phylink support") > > Signed-off-by: Liu Jian <liujian56@huawei.com> > > --- > > .../net/ethernet/microchip/sparx5/sparx5_netdev.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c > > b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c > > index 19516ccad533..d078156581d5 100644 > > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c > > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c > > @@ -104,7 +104,7 @@ static int sparx5_port_open(struct net_device *ndev) > > err = phylink_of_phy_connect(port->phylink, port->of_node, 0); > > if (err) { > > netdev_err(ndev, "Could not attach to PHY\n"); > > - return err; > > + goto err_connect; > > } > > > > phylink_start(port->phylink); > > @@ -116,10 +116,20 @@ static int sparx5_port_open(struct net_device *ndev) > > err = sparx5_serdes_set(port->sparx5, port, &port- > > > conf); > > else > > err = phy_power_on(port->serdes); > > - if (err) > > + if (err) { > > netdev_err(ndev, "%s failed\n", __func__); > > + goto out_power; > > + } > > } > > > > + return 0; > > + > > +out_power: > > + phylink_stop(port->phylink); > > + phylink_disconnect_phy(port->phylink); > > +err_connect: > > + sparx5_port_enable(port, false); > > + > > return err; > > } > > > > -- > > 2.17.1 > > > > Tested-off-by: Bjarni Jonasson <bjarni.jonasson@microchip.com> > Reviewed-off-by: Lars Povlsen <steen.hegelund@microchip.com> > > BR > Steen > Tested-by: Bjarni Jonasson <bjarni.jonasson@microchip.com> Reviewed-by: Steen Hegelund <steen.hegelund@microchip.com> BR Steen
Hello: This patch was applied to netdev/net.git (master) by Paolo Abeni <pabeni@redhat.com>: On Thu, 17 Nov 2022 20:59:18 +0800 you wrote: > If phylink_of_phy_connect() fails, the port should be disabled. > If sparx5_serdes_set()/phy_power_on() fails, the port should be > disabled and the phylink should be stopped and disconnected. > > Fixes: 946e7fd5053a ("net: sparx5: add port module support") > Fixes: f3cad2611a77 ("net: sparx5: add hostmode with phylink support") > Signed-off-by: Liu Jian <liujian56@huawei.com> > > [...] Here is the summary with links: - [net] net: sparx5: fix error handling in sparx5_port_open() https://git.kernel.org/netdev/net/c/4305fe232b8a You are awesome, thank you!
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c index 19516ccad533..d078156581d5 100644 --- a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c @@ -104,7 +104,7 @@ static int sparx5_port_open(struct net_device *ndev) err = phylink_of_phy_connect(port->phylink, port->of_node, 0); if (err) { netdev_err(ndev, "Could not attach to PHY\n"); - return err; + goto err_connect; } phylink_start(port->phylink); @@ -116,10 +116,20 @@ static int sparx5_port_open(struct net_device *ndev) err = sparx5_serdes_set(port->sparx5, port, &port->conf); else err = phy_power_on(port->serdes); - if (err) + if (err) { netdev_err(ndev, "%s failed\n", __func__); + goto out_power; + } } + return 0; + +out_power: + phylink_stop(port->phylink); + phylink_disconnect_phy(port->phylink); +err_connect: + sparx5_port_enable(port, false); + return err; }
If phylink_of_phy_connect() fails, the port should be disabled. If sparx5_serdes_set()/phy_power_on() fails, the port should be disabled and the phylink should be stopped and disconnected. Fixes: 946e7fd5053a ("net: sparx5: add port module support") Fixes: f3cad2611a77 ("net: sparx5: add hostmode with phylink support") Signed-off-by: Liu Jian <liujian56@huawei.com> --- .../net/ethernet/microchip/sparx5/sparx5_netdev.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)