* [PATCH] ibmvnic: Use -EBUSY in __ibmvnic_reset() @ 2024-04-19 14:08 Markus Elfring 2024-04-23 10:54 ` Paolo Abeni 0 siblings, 1 reply; 6+ messages in thread From: Markus Elfring @ 2024-04-19 14:08 UTC (permalink / raw) To: linuxppc-dev, netdev, kernel-janitors, Aneesh Kumar K.V, Christophe Leroy, David S. Miller, Eric Dumazet, Haren Myneni, Jakub Kicinski, Michael Ellerman, Naveen N. Rao, Nicholas Piggin, Nick Child, Paolo Abeni, Rick Lindsley, Thomas Falcon Cc: LKML 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; } -- 2.44.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ibmvnic: Use -EBUSY in __ibmvnic_reset() 2024-04-19 14:08 [PATCH] ibmvnic: Use -EBUSY in __ibmvnic_reset() Markus Elfring @ 2024-04-23 10:54 ` Paolo Abeni 2024-04-23 11:55 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Paolo Abeni @ 2024-04-23 10:54 UTC (permalink / raw) To: Markus Elfring, linuxppc-dev, netdev, kernel-janitors, Aneesh Kumar K.V, Christophe Leroy, David S. Miller, Eric Dumazet, Haren Myneni, Jakub Kicinski, Michael Ellerman, Naveen N. Rao, Nicholas Piggin, Nick Child, Rick Lindsley, Thomas Falcon Cc: LKML 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 > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ibmvnic: Use -EBUSY in __ibmvnic_reset() 2024-04-23 10:54 ` Paolo Abeni @ 2024-04-23 11:55 ` Dan Carpenter 2024-04-23 14:55 ` Nick Child 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2024-04-23 11:55 UTC (permalink / raw) To: Paolo Abeni Cc: Markus Elfring, linuxppc-dev, netdev, kernel-janitors, Aneesh Kumar K.V, Christophe Leroy, David S. Miller, Eric Dumazet, Haren Myneni, Jakub Kicinski, Michael Ellerman, Naveen N. Rao, Nicholas Piggin, Nick Child, Rick Lindsley, Thomas Falcon, LKML 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ibmvnic: Use -EBUSY in __ibmvnic_reset() 2024-04-23 11:55 ` Dan Carpenter @ 2024-04-23 14:55 ` Nick Child 2024-04-23 15:41 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Nick Child @ 2024-04-23 14:55 UTC (permalink / raw) To: Dan Carpenter, Paolo Abeni Cc: Markus Elfring, linuxppc-dev, netdev, kernel-janitors, Aneesh Kumar K.V, Christophe Leroy, David S. Miller, Eric Dumazet, Haren Myneni, Jakub Kicinski, Michael Ellerman, Naveen N. Rao, Nicholas Piggin, Rick Lindsley, Thomas Falcon, LKML 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ibmvnic: Use -EBUSY in __ibmvnic_reset() 2024-04-23 14:55 ` Nick Child @ 2024-04-23 15:41 ` Dan Carpenter 2024-04-25 2:32 ` Jakub Kicinski 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2024-04-23 15:41 UTC (permalink / raw) To: Nick Child Cc: Paolo Abeni, Markus Elfring, linuxppc-dev, netdev, kernel-janitors, Aneesh Kumar K.V, Christophe Leroy, David S. Miller, Eric Dumazet, Haren Myneni, Jakub Kicinski, Michael Ellerman, Naveen N. Rao, Nicholas Piggin, Rick Lindsley, Thomas Falcon, LKML 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ibmvnic: Use -EBUSY in __ibmvnic_reset() 2024-04-23 15:41 ` Dan Carpenter @ 2024-04-25 2:32 ` Jakub Kicinski 0 siblings, 0 replies; 6+ messages in thread From: Jakub Kicinski @ 2024-04-25 2:32 UTC (permalink / raw) To: Dan Carpenter Cc: Nick Child, Paolo Abeni, Markus Elfring, linuxppc-dev, netdev, kernel-janitors, Aneesh Kumar K.V, Christophe Leroy, David S. Miller, Eric Dumazet, Haren Myneni, Michael Ellerman, Naveen N. Rao, Nicholas Piggin, Rick Lindsley, Thomas Falcon, LKML 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. -- pw-bot: reject ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-04-25 2:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-19 14:08 [PATCH] ibmvnic: Use -EBUSY in __ibmvnic_reset() Markus Elfring 2024-04-23 10:54 ` Paolo Abeni 2024-04-23 11:55 ` Dan Carpenter 2024-04-23 14:55 ` Nick Child 2024-04-23 15:41 ` Dan Carpenter 2024-04-25 2:32 ` Jakub Kicinski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).