On 4/23/24 06:55, Dan Carpenter wrote:
On Tue, Apr 23, 2024 at 12:54:55PM +0200, Paolo Abeni wrote:
On Fri, 2024-04-19 at 16:08 +0200, Markus Elfring wrote:
From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
Date: Fri, 19 Apr 2024 15:46:17 +0200
Add a minus sign before the error code “EBUSY”
so that a negative value will be used as in other cases.
This issue was transformed by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
---
drivers/net/ethernet/ibm/ibmvnic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 5e9a93bdb518..737ae83a836a 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -3212,7 +3212,7 @@ static void __ibmvnic_reset(struct work_struct *work)
adapter->state == VNIC_REMOVED) {
spin_unlock_irqrestore(&adapter->state_lock, flags);
kfree(rwi);
- rc = EBUSY;
+ rc = -EBUSY;
break;
AFAICS the error is always used as bool, so this will not change any
behavior in practice. I tend to think we should not merge this kind of
change outside some larger work in the same area, but I'd love a second
opinion from the driver owners.
I missed the original patch due to my procmail filters...
You're right that it doesn't affect the behavior of the driver except
for the debug output when we do:
netdev_dbg(adapter->netdev, "Reset failed, rc=%d\n", rc);
But the - was left off uninitentionally so I think we should apply it.
I have been trying to look for similar bugs where the - is left off.
It's a bit challenging because there places where we use positive
error codes deliberately. But in this case a static checker could
easily detect the bug with a low false positive ratio by saying, "We're
mixing normal negative error codes with positive EBUSY".
regards,
dan carpenter
Hello, small clarification, this patch will not effect the debug print
statement due to the break statement immediately following:
while () {
if () {
rc = -EBUSY;
break;
}
netdev_dbg(adapter->netdev, "Reset failed, rc=%d\n", rc);
}
Though this return code can be passed to adapter->reset_done_rc, which
is only treated as a boolean.
So, the point of the patch not doing any behavioral differences is still
true.
Personally, I don't have strong opinions on this.
Reviewed-by: Nick Child <nnac123@xxxxxxxxxxxxx>
