| Message ID | 4cff158d-b5ac-4dca-9fbb-626237c1eafe@web.de (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ibmvnic: Use -EBUSY in __ibmvnic_reset() | expand |
On Fri, 2024-04-19 at 16:08 +0200, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > 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@users.sourceforge.net> > --- > 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. Thanks, Paolo > }
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@users.sourceforge.net> > > 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@users.sourceforge.net> > > --- > > 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
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@users.sourceforge.net> >>> 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@users.sourceforge.net> >>> --- >>> 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@linux.ibm.com>
On Tue, Apr 23, 2024 at 09:55:57AM -0500, Nick Child wrote: > > 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. Ah yes. You're right. regards, dan carpenter
On Tue, 23 Apr 2024 18:41:48 +0300 Dan Carpenter wrote: > > So, the point of the patch not doing any behavioral differences is still > > true. > > Ah yes. You're right. Hard call but overall I think this wasted more reviewer time than it's worth. So in the spirit of not encouraging noise I'm not applying this.
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; }