[v2] net: atlantic: fix overwritten return value in Aquantia driver

[PATCH net v2] net: atlantic: fix overwritten return value in Aquantia driver

Posted by Tian 2 months, 1 week ago

From: Tian Liu <27392025k@gmail.com>

In hw_atl_utils.c and hw_atl_utils_fw2x.c, a return value is first assigned
to `err`, but then later calls overwrite it unconditionally. As a result,
any earlier failure is lost if the later call succeeds. This may cause the
driver to proceed when it should have stopped.

This patch uses `err |=` instead of assignment, so that any earlier error
is preserved even if later calls succeed. This is safe because all involved
functions return standard negative error codes (e.g. -EIO, -ETIMEDOUT, etc).
OR-ing such values does not convert them into success, and preserves the
indication that an error occurred.

Fixes: 6a7f2277313b4 ("net: aquantia: replace AQ_HW_WAIT_FOR with readx_poll_timeout_atomic")

Signed-off-by: Tian Liu <27392025k@gmail.com>

Changes in v2:
- Add full name in Signed-off-by
- Update subject to include [PATCH net v2]
- Add Fixes: tag
- Expand commit message to explain why OR-ing errors is safe

---
 drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c  | 2 +-
 .../net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
index 7e88d7234b14..372f30e296ec 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
@@ -492,7 +492,7 @@ static int hw_atl_utils_init_ucp(struct aq_hw_s *self,
 					self, self->mbox_addr,
 					self->mbox_addr != 0U,
 					1000U, 10000U);
-	err = readx_poll_timeout_atomic(aq_fw1x_rpc_get, self,
+	err |= readx_poll_timeout_atomic(aq_fw1x_rpc_get, self,
 					self->rpc_addr,
 					self->rpc_addr != 0U,
 					1000U, 100000U);
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
index 4d4cfbc91e19..45b7720fd49e 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
@@ -102,12 +102,12 @@ static int aq_fw2x_init(struct aq_hw_s *self)
 					self->mbox_addr != 0U,
 					1000U, 10000U);
 
-	err = readx_poll_timeout_atomic(aq_fw2x_rpc_get,
+	err |= readx_poll_timeout_atomic(aq_fw2x_rpc_get,
 					self, self->rpc_addr,
 					self->rpc_addr != 0U,
 					1000U, 100000U);
 
-	err = aq_fw2x_settings_get(self, &self->settings_addr);
+	err |= aq_fw2x_settings_get(self, &self->settings_addr);
 
 	return err;
 }
-- 
2.39.5 (Apple Git-154)

Re: [PATCH net v2] net: atlantic: fix overwritten return value in Aquantia driver

Posted by Markus Elfring 2 months, 1 week ago

…
> This patch uses …

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.16#n94


…
> Signed-off-by: Tian Liu <27392025k@gmail.com>
> 
> Changes in v2:
…
> ---

Please move the marker line.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.16#n784

Regards,
Markus

Re: [PATCH net v2] net: atlantic: fix overwritten return value in Aquantia driver

Posted by Simon Horman 2 months, 1 week ago

On Mon, Jul 28, 2025 at 05:58:52PM -0700, Tian wrote:
> From: Tian Liu <27392025k@gmail.com>
> 
> In hw_atl_utils.c and hw_atl_utils_fw2x.c, a return value is first assigned
> to `err`, but then later calls overwrite it unconditionally. As a result,
> any earlier failure is lost if the later call succeeds. This may cause the
> driver to proceed when it should have stopped.
> 
> This patch uses `err |=` instead of assignment, so that any earlier error
> is preserved even if later calls succeed. This is safe because all involved
> functions return standard negative error codes (e.g. -EIO, -ETIMEDOUT, etc).
> OR-ing such values does not convert them into success, and preserves the
> indication that an error occurred.
> 
> Fixes: 6a7f2277313b4 ("net: aquantia: replace AQ_HW_WAIT_FOR with readx_poll_timeout_atomic")

nit: No blank line here

I do agree that the Fixes tag is correct in the case of hw_atl_utils_fw2x.c

But, in the case of hw_atl_utils.c, it seems to me that the correct Fixes
tag would be as follows, because prior to this commit the value of err was
not overwritten:

Fixes: e7b5f97e6574 ("net: atlantic: check rpc result and wait for rpc address")

Given the different commits that are being fixed I would suggest splitting
the patch into two, one per file that is being updated.

> 
> Signed-off-by: Tian Liu <27392025k@gmail.com>

...

-- 
pw-bot: changes-requested

Re: [PATCH net v2] net: atlantic: fix overwritten return value in Aquantia driver

Posted by Andrew Lunn 2 months, 1 week ago

> This patch uses `err |=` instead of assignment, so that any earlier error
> is preserved even if later calls succeed. This is safe because all involved
> functions return standard negative error codes (e.g. -EIO, -ETIMEDOUT, etc).
> OR-ing such values does not convert them into success, and preserves the
> indication that an error occurred.

22 | 110 = 132 = ERFKILL

How easy will it be for somebody to debug that?

	Andrew

Re: [PATCH net v2] net: atlantic: fix overwritten return value in Aquantia driver

Posted by Simon Horman 2 months, 1 week ago

On Tue, Jul 29, 2025 at 03:39:01AM +0200, Andrew Lunn wrote:
> > This patch uses `err |=` instead of assignment, so that any earlier error
> > is preserved even if later calls succeed. This is safe because all involved
> > functions return standard negative error codes (e.g. -EIO, -ETIMEDOUT, etc).
> > OR-ing such values does not convert them into success, and preserves the
> > indication that an error occurred.
> 
> 22 | 110 = 132 = ERFKILL
> 
> How easy will it be for somebody to debug that?

Hi Andrew and Tian,

I agree that this does not seem to be a good approach.

Looking over the code, it seems that we can just bail
out if an error occurs. Returning error codes as they arise.

Does this seem reasonable to you?

diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
index 7e88d7234b14..6c490f61ce5c 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
@@ -492,6 +492,9 @@ static int hw_atl_utils_init_ucp(struct aq_hw_s *self,
 					self, self->mbox_addr,
 					self->mbox_addr != 0U,
 					1000U, 10000U);
+	if (err)
+		return err;
+
 	err = readx_poll_timeout_atomic(aq_fw1x_rpc_get, self,
 					self->rpc_addr,
 					self->rpc_addr != 0U,
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
index 4d4cfbc91e19..48f7c863c04b 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
@@ -101,11 +101,15 @@ static int aq_fw2x_init(struct aq_hw_s *self)
 					self, self->mbox_addr,
 					self->mbox_addr != 0U,
 					1000U, 10000U);
+	if (err)
+		return err;
 
 	err = readx_poll_timeout_atomic(aq_fw2x_rpc_get,
 					self, self->rpc_addr,
 					self->rpc_addr != 0U,
 					1000U, 100000U);
+	if (err)
+		return err;
 
 	err = aq_fw2x_settings_get(self, &self->settings_addr);