[PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[Liu Jian]

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

[PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[Liu Jian]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[Russell King (Oracle)]

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!

Re: [PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[Russell King (Oracle)]

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!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[Steen Hegelund]

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

Re: [PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[Steen Hegelund]

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[Steen Hegelund]

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

Re: [PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[Steen Hegelund]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[Steen Hegelund]

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

Re: [PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[Steen Hegelund]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] net: sparx5: fix error handling in sparx5_port_open()

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel