[PATCH] tg3: Remove residual error handling in tg3_suspend

[PATCH] tg3: Remove residual error handling in tg3_suspend

[Nikita Kiryushin]

As of now, tg3_power_down_prepare always ends with success, but
the error handling code from former tg3_set_power_state call is still here.

Remove (now unreachable) code for simplification.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: c866b7eac073 ("tg3: Do not use legacy PCI power management")
Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru>
---
  drivers/net/ethernet/broadcom/tg3.c | 26 ++------------------------
  1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c 
b/drivers/net/ethernet/broadcom/tg3.c
index 04964bbe08cf..400451e10f77 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -18090,7 +18090,6 @@ static int tg3_suspend(struct device *device)
  {
  	struct net_device *dev = dev_get_drvdata(device);
  	struct tg3 *tp = netdev_priv(dev);
-	int err = 0;
   	rtnl_lock();
  @@ -18114,32 +18113,11 @@ static int tg3_suspend(struct device *device)
  	tg3_flag_clear(tp, INIT_COMPLETE);
  	tg3_full_unlock(tp);
-	err = tg3_power_down_prepare(tp);
-	if (err) {
-		int err2;
-
-		tg3_full_lock(tp, 0);
-
-		tg3_flag_set(tp, INIT_COMPLETE);
-		err2 = tg3_restart_hw(tp, true);
-		if (err2)
-			goto out;
-
-		tg3_timer_start(tp);
-
-		netif_device_attach(dev);
-		tg3_netif_start(tp);
-
-out:
-		tg3_full_unlock(tp);
-
-		if (!err2)
-			tg3_phy_start(tp);
-	}
+	tg3_power_down_prepare(tp);
   unlock:
  	rtnl_unlock();
-	return err;
+	return 0;
  }
   static int tg3_resume(struct device *device)
-- 
2.34.1

Re: [PATCH] tg3: Remove residual error handling in tg3_suspend

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

On Thu, Mar 7, 2024 at 11:23 AM Nikita Kiryushin <kiryushin@ancud.ru> wrote:
>
>
> As of now, tg3_power_down_prepare always ends with success, but
> the error handling code from former tg3_set_power_state call is still here.
>
> Remove (now unreachable) code for simplification.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: c866b7eac073 ("tg3: Do not use legacy PCI power management")
> Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru>

The patch looks good to me.  But I think we can go one step further
and change tg3_power_down_prepare() to a void function.  Thanks.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH v2] tg3: Remove residual error handling in tg3_suspend

[Nikita Kiryushin]

As of now, tg3_power_down_prepare always ends with success, but
the error handling code from former tg3_set_power_state call is still here.

Remove (now unreachable) code for simplification and change
tg3_power_down_prepare to a void function as its result is no more checked.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: c866b7eac073 ("tg3: Do not use legacy PCI power management")
Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru>
---
v2: Change tg3_power_down_prepare() to a void function
as Michael Chan <michael.chan@broadcom.com> suggested.
  drivers/net/ethernet/broadcom/tg3.c | 30 ++++-------------------------
  1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c 
b/drivers/net/ethernet/broadcom/tg3.c
index 04964bbe08cf..bc36926a57cf 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -4019,7 +4019,7 @@ static int tg3_power_up(struct tg3 *tp)
   static int tg3_setup_phy(struct tg3 *, bool);
-static int tg3_power_down_prepare(struct tg3 *tp)
+static void tg3_power_down_prepare(struct tg3 *tp)
  {
  	u32 misc_host_ctrl;
  	bool device_should_wake, do_low_power;
@@ -4263,7 +4263,7 @@ static int tg3_power_down_prepare(struct tg3 *tp)
   	tg3_ape_driver_state_change(tp, RESET_KIND_SHUTDOWN);
-	return 0;
+	return;
  }
   static void tg3_power_down(struct tg3 *tp)
@@ -18090,7 +18090,6 @@ static int tg3_suspend(struct device *device)
  {
  	struct net_device *dev = dev_get_drvdata(device);
  	struct tg3 *tp = netdev_priv(dev);
-	int err = 0;
   	rtnl_lock();
  @@ -18114,32 +18113,11 @@ static int tg3_suspend(struct device *device)
  	tg3_flag_clear(tp, INIT_COMPLETE);
  	tg3_full_unlock(tp);
-	err = tg3_power_down_prepare(tp);
-	if (err) {
-		int err2;
-
-		tg3_full_lock(tp, 0);
-
-		tg3_flag_set(tp, INIT_COMPLETE);
-		err2 = tg3_restart_hw(tp, true);
-		if (err2)
-			goto out;
-
-		tg3_timer_start(tp);
-
-		netif_device_attach(dev);
-		tg3_netif_start(tp);
-
-out:
-		tg3_full_unlock(tp);
-
-		if (!err2)
-			tg3_phy_start(tp);
-	}
+	tg3_power_down_prepare(tp);
   unlock:
  	rtnl_unlock();
-	return err;
+	return 0;
  }
   static int tg3_resume(struct device *device)
-- 
2.34.1

Re: [PATCH v2] tg3: Remove residual error handling in tg3_suspend

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

On Mon, Mar 11, 2024 at 8:45 AM Nikita Kiryushin <kiryushin@ancud.ru> wrote:
>
>
> As of now, tg3_power_down_prepare always ends with success, but
> the error handling code from former tg3_set_power_state call is still here.
>
> Remove (now unreachable) code for simplification and change
> tg3_power_down_prepare to a void function as its result is no more checked.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: c866b7eac073 ("tg3: Do not use legacy PCI power management")
> Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru>
> ---
> v2: Change tg3_power_down_prepare() to a void function
> as Michael Chan <michael.chan@broadcom.com> suggested.
>   drivers/net/ethernet/broadcom/tg3.c | 30 ++++-------------------------
>   1 file changed, 4 insertions(+), 26 deletions(-)

The patch looks good to me.  But this cleanup patch should be for
net-next and net-next is closed right now.
Please re-post in about 2 weeks.  You can add my tag when you repost:

Reviewed-by: Michael Chan <michael.chan@broadcom.com>

Thanks.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next v2] tg3: Remove residual error handling in tg3_suspend

[Nikita Kiryushin]

As of now, tg3_power_down_prepare always ends with success, but
the error handling code from former tg3_set_power_state call is still here.

Remove (now unreachable) code for simplification and change
tg3_power_down_prepare to a void function as its result is no more checked.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: c866b7eac073 ("tg3: Do not use legacy PCI power management")
Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru>
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
---
v2: Change tg3_power_down_prepare() to a void function
as Michael Chan <michael.chan@broadcom.com> suggested.
 drivers/net/ethernet/broadcom/tg3.c | 30 ++++-------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 04964bbe08cf..bc36926a57cf 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -4019,7 +4019,7 @@ static int tg3_power_up(struct tg3 *tp)
 
 static int tg3_setup_phy(struct tg3 *, bool);
 
-static int tg3_power_down_prepare(struct tg3 *tp)
+static void tg3_power_down_prepare(struct tg3 *tp)
 {
 	u32 misc_host_ctrl;
 	bool device_should_wake, do_low_power;
@@ -4263,7 +4263,7 @@ static int tg3_power_down_prepare(struct tg3 *tp)
 
 	tg3_ape_driver_state_change(tp, RESET_KIND_SHUTDOWN);
 
-	return 0;
+	return;
 }
 
 static void tg3_power_down(struct tg3 *tp)
@@ -18090,7 +18090,6 @@ static int tg3_suspend(struct device *device)
 {
 	struct net_device *dev = dev_get_drvdata(device);
 	struct tg3 *tp = netdev_priv(dev);
-	int err = 0;
 
 	rtnl_lock();
 
@@ -18114,32 +18113,11 @@ static int tg3_suspend(struct device *device)
 	tg3_flag_clear(tp, INIT_COMPLETE);
 	tg3_full_unlock(tp);
 
-	err = tg3_power_down_prepare(tp);
-	if (err) {
-		int err2;
-
-		tg3_full_lock(tp, 0);
-
-		tg3_flag_set(tp, INIT_COMPLETE);
-		err2 = tg3_restart_hw(tp, true);
-		if (err2)
-			goto out;
-
-		tg3_timer_start(tp);
-
-		netif_device_attach(dev);
-		tg3_netif_start(tp);
-
-out:
-		tg3_full_unlock(tp);
-
-		if (!err2)
-			tg3_phy_start(tp);
-	}
+	tg3_power_down_prepare(tp);
 
 unlock:
 	rtnl_unlock();
-	return err;
+	return 0;
 }
 
 static int tg3_resume(struct device *device)
-- 
2.34.1

Re: [PATCH net-next v2] tg3: Remove residual error handling in tg3_suspend

[Ratheesh Kannoth]

On 2024-03-27 at 00:05:44, Nikita Kiryushin (kiryushin@ancud.ru) wrote:
> As of now, tg3_power_down_prepare always ends with success, but
> the error handling code from former tg3_set_power_state call is still here.
>
> Remove (now unreachable) code for simplification and change
> tg3_power_down_prepare to a void function as its result is no more checked.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: c866b7eac073 ("tg3: Do not use legacy PCI power management")
> Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru>
> Reviewed-by: Michael Chan <michael.chan@broadcom.com>
> ---
> v2: Change tg3_power_down_prepare() to a void function
> as Michael Chan <michael.chan@broadcom.com> suggested.
>  drivers/net/ethernet/broadcom/tg3.c | 30 ++++-------------------------
>  1 file changed, 4 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 04964bbe08cf..bc36926a57cf 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -4019,7 +4019,7 @@ static int tg3_power_up(struct tg3 *tp)
>
>  static int tg3_setup_phy(struct tg3 *, bool);
>
> -static int tg3_power_down_prepare(struct tg3 *tp)
> +static void tg3_power_down_prepare(struct tg3 *tp)
>  {
>  	u32 misc_host_ctrl;
>  	bool device_should_wake, do_low_power;
> @@ -4263,7 +4263,7 @@ static int tg3_power_down_prepare(struct tg3 *tp)
>
>  	tg3_ape_driver_state_change(tp, RESET_KIND_SHUTDOWN);
>
> -	return 0;
> +	return;
>  }
>
>  static void tg3_power_down(struct tg3 *tp)
> @@ -18090,7 +18090,6 @@ static int tg3_suspend(struct device *device)
>  {
Please address Michael Chan's comment to make this function return type to "void"
instead of "int"

>

Re: [PATCH net-next v2] tg3: Remove residual error handling in tg3_suspend

[Jakub Kicinski]

On Wed, 27 Mar 2024 08:14:30 +0530 Ratheesh Kannoth wrote:
> > @@ -18090,7 +18090,6 @@ static int tg3_suspend(struct device *device)
> >  {  
> Please address Michael Chan's comment to make this function return type to "void"
> instead of "int"

It's used as a callback in SIMPLE_DEV_PM_OPS(), how would that work?

Re: [PATCH net-next v2] tg3: Remove residual error handling in tg3_suspend

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On Tue, Mar 26, 2024 at 8:24 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 27 Mar 2024 08:14:30 +0530 Ratheesh Kannoth wrote:
> > > @@ -18090,7 +18090,6 @@ static int tg3_suspend(struct device *device)
> > >  {
> > Please address Michael Chan's comment to make this function return type to "void"
> > instead of "int"
>
> It's used as a callback in SIMPLE_DEV_PM_OPS(), how would that work?

It won't work.  I only requested to change tg3_power_down_prepare() to
void and it was done in v2:

https://lore.kernel.org/netdev/CACKFLinzJjqe0j4OFkcCV+FyH0JiUpnj3j2azZkGaC9jfvFXrQ@mail.gmail.com/

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

RE: [EXTERNAL] Re: [PATCH net-next v2] tg3: Remove residual error handling in tg3_suspend

[Ratheesh Kannoth]

> From: Michael Chan <michael.chan@broadcom.com>
> Sent: Wednesday, March 27, 2024 10:39 AM
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: Ratheesh Kannoth <rkannoth@marvell.com>; Nikita Kiryushin
> <kiryushin@ancud.ru>; Michael Chan <mchan@broadcom.com>; Pavan
> Chebbi <pavan.chebbi@broadcom.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Paolo
> Abeni <pabeni@redhat.com>; Rafael J. Wysocki <rjw@rjwysocki.net>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; lvc-
> project@linuxtesting.org
> Subject: [EXTERNAL] Re: [PATCH net-next v2] tg3: Remove residual error
> handling in tg3_suspend
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> On Tue, Mar 26, 2024 at 8:24 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed, 27 Mar 2024 08:14:30 +0530 Ratheesh Kannoth wrote:
> > > > @@ -18090,7 +18090,6 @@ static int tg3_suspend(struct device
> *device)
> > > >  {
> > > Please address Michael Chan's comment to make this function return type
> to "void"
> > > instead of "int"
> >
> > It's used as a callback in SIMPLE_DEV_PM_OPS(), how would that work?
> 
> It won't work.  I only requested to change tg3_power_down_prepare() to
> void and it was done in v2:
> 
> https://lore.kernel.org/netdev/CACKFLinzJjqe0j4OFkcCV+FyH0JiUpnj3j2azZk
> GaC9jfvFXrQ@mail.gmail.com/
Got it. Thanks. 

Re: [PATCH net-next v2] tg3: Remove residual error handling in tg3_suspend

[Jakub Kicinski]

On Tue, 26 Mar 2024 21:35:44 +0300 Nikita Kiryushin wrote:
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

Please stop adding these lines, they are unless.
If you want to attribute the work to some project / employer
add (SVACE) to your author lines.

> Fixes: c866b7eac073 ("tg3: Do not use legacy PCI power management")

How is deleting dead code a fix?

Please read:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html

Hopefully you can find in there whether we suggest posting new versions
in reply to the old ones..
-- 
pw-bot: cr

Re: [PATCH net-next v2] tg3: Remove residual error handling in tg3_suspend

[Nikita Kiryushin]

Thank you for the feedback!

> Please stop adding these lines, they are unless.
> If you want to attribute the work to some project / employer
> add (SVACE) to your author lines.
>
Tool used signature line (the SVACE one) in that format was put in the
patch as required by lvc-project protocols. I guess, it is used for some
automation by the project, but I can agree, that the needs of some
specific project should not add cruft to the general code base/version control.

Thank you for expressing annoyance about it, I hope it will trigger conversation
about better way of adding the needed information for further iterations of the
project (as this thread is being mirrored to the project mailing list).
> How is deleting dead code a fix?
Originally, that was intended as a fix of a potential problematic case, that
tg3_power_down_prepare() could change in the future, returning a
non-zero status (which would make the removed code not dead, but undead).
But than, as patch evolved, it became a straight up dead code removal.
Probably, I should have removed the "fixes" line at that point, but
I think it still useful as a reference point to know, after which commit this patch
becomes relevant (to know, if it should be back-ported to some version or not,
for example).As I guess from the guide, patches "Fixes:" tag has some special treatment in the development cycle, but what would be more appropriate in that case?
> Hopefully you can find in there whether we suggest posting new versions
> in reply to the old ones..
Thank you for pointing this out! Missed the part about "The new version of
patches should be posted as a separate thread, not as a reply to the
previous posting. Change log should include a link to the previous posting"
while reading netdev guide.

Should I resubmit the patch in the separate thread now? If so, should I
make any changes to it (as discussed)? Would it bump the patch version
(the patch would essentially be the same, as only some metadata like
change log itself would change)?

Re: [PATCH net-next v2] tg3: Remove residual error handling in tg3_suspend

[Jakub Kicinski]

On Fri, 29 Mar 2024 14:51:18 +0300 Nikita Kiryushin wrote:
> > How is deleting dead code a fix?  
> Originally, that was intended as a fix of a potential problematic case, that
> tg3_power_down_prepare() could change in the future, returning a
> non-zero status (which would make the removed code not dead, but undead).
> But than, as patch evolved, it became a straight up dead code removal.
> Probably, I should have removed the "fixes" line at that point, but
> I think it still useful as a reference point to know, after which commit this patch
> becomes relevant (to know, if it should be back-ported to some version or not,
> for example).As I guess from the guide, patches "Fixes:" tag has some special treatment in the development cycle, but what would be more appropriate in that case?

You can quote the commit where code became irrelevant by saying
something like:

This came became unreachable in commit c866b7eac073 ("tg3: Do not use
legacy PCI power management").

Fixes tag are often used to indicate that something is a bug fix.
Backporters are unlikely to care about this particular change,
let's not waste their time.