Skip to contentSkip to conversation

net: aquantia: fix error handling in aq_nic_init

PATCH In review Gustavo A. R. Silva 2 participants 3 messages Oct 28, 2019

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 net: aquantia: fix error handling in aq_nic_init

Fix currenty ignored returned error by properly error checking aq_phy_init().

Addresses-Coverity-ID: 1487376 ("Unused value")

Fixes dbcd6806af42 ("net: aquantia: add support for Phy access")
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/aquantia/atlantic/aq_nic.c +2 -1
index 433adc099e44..1914aa0a19d0 100644
@@ -341,7 +341,8 @@ int aq_nic_init(struct aq_nic_s *self)
 
 	if (self->aq_nic_cfg.aq_hw_caps->media_type == AQ_HW_MEDIA_TYPE_TP) {
 		self->aq_hw->phy_id = HW_ATL_PHY_ID_MAX;
-		err = aq_phy_init(self->aq_hw);
+		if (!aq_phy_init(self->aq_hw))
+			goto err_exit;
 	}
 
 	for (i = 0U, aq_vec = self->aq_vec[0];
Signature · 1 line
2.23.0
#1 lore raw
Quoted 17 lines

Fix currenty ignored returned error by properly error checking aq_phy_init().

Addresses-Coverity-ID: 1487376 ("Unused value")

Fixes dbcd6806af42 ("net: aquantia: add support for Phy access")
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/aquantia/atlantic/aq_nic.c +0 -0
index 433adc099e44..1914aa0a19d0 100644

drivers/net/ethernet/aquantia/atlantic/aq_nic.c int aq_nic_init(struct aq_nic_s *self) · line 348

Quoted 9 lines
drivers/net/ethernet/aquantia/atlantic/aq_nic.c +2 -1
@@ -341,7 +341,8 @@ int aq_nic_init(struct aq_nic_s *self)
 
 	if (self->aq_nic_cfg.aq_hw_caps->media_type == AQ_HW_MEDIA_TYPE_TP) {
 		self->aq_hw->phy_id = HW_ATL_PHY_ID_MAX;
-		err = aq_phy_init(self->aq_hw);
+		if (!aq_phy_init(self->aq_hw))
+			goto err_exit;
 	}
 
 	for (i = 0U, aq_vec = self->aq_vec[0];

Hi Gustavo,

I'd say the intention here was to ignore the error, as driver may still live if something unexpected happened on Phy access path.

Notice in the above fix you leave err as zero but do error path return - that'll break the datapath.

I'd prefer to fix this with simple

     (void)aq_phy_init(self->aq_hw);
Signature · 2 lines
Regards,
  Igor
#2 lore raw
Quoted 24 lines · Igor Russkikh
Quoted 24 lines

Fix currenty ignored returned error by properly error checking aq_phy_init().

Addresses-Coverity-ID: 1487376 ("Unused value")

Fixes dbcd6806af42 ("net: aquantia: add support for Phy access")
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/aquantia/atlantic/aq_nic.c +2 -1
index 433adc099e44..1914aa0a19d0 100644
@@ -341,7 +341,8 @@ int aq_nic_init(struct aq_nic_s *self)
 
 	if (self->aq_nic_cfg.aq_hw_caps->media_type == AQ_HW_MEDIA_TYPE_TP) {
 		self->aq_hw->phy_id = HW_ATL_PHY_ID_MAX;
-		err = aq_phy_init(self->aq_hw);
+		if (!aq_phy_init(self->aq_hw))
+			goto err_exit;
 	}
 
Quoted 5 lines

drivers/net/ethernet/aquantia/atlantic/aq_nic.c int aq_nic_init(struct aq_nic_s *self) · line 348

Quoted 2 lines
drivers/net/ethernet/aquantia/atlantic/aq_nic.c +0 -0
 	for (i = 0U, aq_vec = self->aq_vec[0];

Hi Gustavo,

Hi!

Quoted 3 lines

I'd say the intention here was to ignore the error, as driver may still live if something unexpected happened on Phy access path.

I see. Please, see my comments below...

Quoted 3 lines

Notice in the above fix you leave err as zero but do error path return - that'll break the datapath.

You're right. I totally missed that.

Quoted 4 lines

I'd prefer to fix this with simple

     (void)aq_phy_init(self->aq_hw);

Yep. This is much better. Otherwise, both static analyzers and developers get confused. :p

Something even better is to add both a cast and a comment explaining why the return value is not being checked in this case (as you do above).

Thanks
--
Gustavo