* [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: Nick Child, netdev, Rick Lindsley, kernel-janitors, Thomas Falcon, LKML, Aneesh Kumar K.V, Nicholas Piggin, Eric Dumazet, Markus Elfring, Haren Myneni, Naveen N. Rao, Jakub Kicinski, linuxppc-dev, David S. Miller 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: netdev, Rick Lindsley, kernel-janitors, Thomas Falcon, LKML, Aneesh Kumar K.V, Nicholas Piggin, Eric Dumazet, Markus Elfring, Haren Myneni, Naveen N. Rao, Jakub Kicinski, linuxppc-dev, David S. Miller 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: netdev, Rick Lindsley, kernel-janitors, Thomas Falcon, LKML, Aneesh Kumar K.V, Nicholas Piggin, Eric Dumazet, Markus Elfring, Haren Myneni, Naveen N. Rao, Jakub Kicinski, Paolo Abeni, linuxppc-dev, David S. Miller 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, netdev, Rick Lindsley, kernel-janitors, Thomas Falcon, LKML, Aneesh Kumar K.V, Nicholas Piggin, Eric Dumazet, Markus Elfring, Haren Myneni, Naveen N. Rao, Paolo Abeni, linuxppc-dev, David S. Miller 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).