[PATCH] net: usb: pegasus: fixes of set_register(s) return value evaluation;

  - restore the behavior in enable_net_traffic() to avoid regressions - Jakub
    Kicinski;
  - hurried up and removed redundant assignment in pegasus_open() before yet
    another checker complains;
  - explicitly check for negative value in pegasus_set_wol(), even if
    usb_control_msg_send() never return positive number we'd still be in sync
    with the rest of the driver style;

Fixes: 8a160e2e9aeb net: usb: pegasus: Check the return value of get_geristers() and friends;
Reported-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Petko Manolov <redacted>
---
 drivers/net/usb/pegasus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 652e9fcf0b77..1ef93082c772 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c

@@ -446,7 +446,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
 		write_mii_word(pegasus, 0, 0x1b, &auxmode);
 	}
 
-	return 0;
+	return ret;
 fail:
 	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
 	return ret;

@@ -835,7 +835,7 @@ static int pegasus_open(struct net_device *net)
 	if (!pegasus->rx_skb)
 		goto exit;
 
-	res = set_registers(pegasus, EthID, 6, net->dev_addr);
+	set_registers(pegasus, EthID, 6, net->dev_addr);
 
 	usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
 			  usb_rcvbulkpipe(pegasus->usb, 1),

@@ -932,7 +932,7 @@ pegasus_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 	pegasus->wolopts = wol->wolopts;
 
 	ret = set_register(pegasus, WakeupControl, reg78);
-	if (!ret)
+	if (ret < 0)
 		ret = device_set_wakeup_enable(&pegasus->usb->dev,
 						wol->wolopts);
 	return ret;

-- 
2.30.2

On Thu, 19 Aug 2021 12:05:39 +0300 Petko Manolov wrote:

  - restore the behavior in enable_net_traffic() to avoid regressions - Jakub
    Kicinski;
  - hurried up and removed redundant assignment in pegasus_open() before yet
    another checker complains;
  - explicitly check for negative value in pegasus_set_wol(), even if
    usb_control_msg_send() never return positive number we'd still be in sync
    with the rest of the driver style;

Fixes: 8a160e2e9aeb net: usb: pegasus: Check the return value of get_geristers() and friends;

I guess this is fine but not exactly the preferred format, please see
Submitting patches.

yup

yup

now this looks incorrect and unrelated to recent changes (IOW the
commit under Fixes), please drop this chunk

 	return ret;

On 21-08-19 12:34:29, Jakub Kicinski wrote:

On Thu, 19 Aug 2021 12:05:39 +0300 Petko Manolov wrote:

quoted

  - restore the behavior in enable_net_traffic() to avoid regressions - Jakub
    Kicinski;
  - hurried up and removed redundant assignment in pegasus_open() before yet
    another checker complains;
  - explicitly check for negative value in pegasus_set_wol(), even if
    usb_control_msg_send() never return positive number we'd still be in sync
    with the rest of the driver style;

Fixes: 8a160e2e9aeb net: usb: pegasus: Check the return value of get_geristers() and friends;

I guess this is fine but not exactly the preferred format, please see
Submitting patches.

If the preferred format involves brackets and quotes - so be it.

quoted

Reported-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Petko Manolov <redacted>
---
 drivers/net/usb/pegasus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 652e9fcf0b77..1ef93082c772 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c

@@ -446,7 +446,7 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
 		write_mii_word(pegasus, 0, 0x1b, &auxmode);
 	}
 
-	return 0;
+	return ret;

yup

quoted

 fail:
 	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
 	return ret;

@@ -835,7 +835,7 @@ static int pegasus_open(struct net_device *net)
 	if (!pegasus->rx_skb)
 		goto exit;
 
-	res = set_registers(pegasus, EthID, 6, net->dev_addr);
+	set_registers(pegasus, EthID, 6, net->dev_addr);

yup

quoted

 	usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
 			  usb_rcvbulkpipe(pegasus->usb, 1),

@@ -932,7 +932,7 @@ pegasus_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 	pegasus->wolopts = wol->wolopts;
 
 	ret = set_register(pegasus, WakeupControl, reg78);
-	if (!ret)
+	if (ret < 0)
 		ret = device_set_wakeup_enable(&pegasus->usb->dev,
 						wol->wolopts);

now this looks incorrect and unrelated to recent changes (IOW the
commit under Fixes), please drop this chunk

It may not be related, but the change is:

	a) obviously correct; and

	b) in sync with the rest of 'ret' evaluations;

To be honest i do not envision usb_control_msg_send() returning positive value
anytime soon, but (ret < 0) looks "more" correct than (!ret).


		Petko