| Message ID | 20240211150535.3529-1-d.dulov@aladdin.ru (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: softing: remove redundant NULL check | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
On Sun, Feb 11, 2024 at 07:05:35AM -0800, Daniil Dulov wrote: > In this case dev cannot be NULL, so remove redundant check. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 03fd3cf5a179 ("can: add driver for Softing card") > Signed-off-by: Daniil Dulov <d.dulov@aladdin.ru> Hi Daniil, I am not sure that dev cannot be NULL. But I do see that the code assumes it is not, and would crash if it is. So I think that, functionally, your statement is correct. priv = netdev_priv(dev); card = priv->card; Reviewed-by: Simon Horman <horms@kernel.org>
Hi Simon, I have a general question on the "Fixes:" tag in this patch: On 16.02.24 18:27, Simon Horman wrote: > On Sun, Feb 11, 2024 at 07:05:35AM -0800, Daniil Dulov wrote: >> In this case dev cannot be NULL, so remove redundant check. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Fixes: 03fd3cf5a179 ("can: add driver for Softing card") IMHO this is simply an improvement which is done by all patches applied to the kernel but it does not really "fix" anything from a functional standpoint. Shouldn't we either invent a new tag or better leave it out to not confuse the stable maintainers? Best regards, Oliver >> Signed-off-by: Daniil Dulov <d.dulov@aladdin.ru> > > Hi Daniil, > > I am not sure that dev cannot be NULL. > But I do see that the code assumes it is not, and would crash if it is. > So I think that, functionally, your statement is correct. > > priv = netdev_priv(dev); > card = priv->card; > > Reviewed-by: Simon Horman <horms@kernel.org> >
On Fri, Feb 16, 2024 at 08:47:43PM +0100, Oliver Hartkopp wrote: > Hi Simon, > > I have a general question on the "Fixes:" tag in this patch: > > On 16.02.24 18:27, Simon Horman wrote: > > On Sun, Feb 11, 2024 at 07:05:35AM -0800, Daniil Dulov wrote: > > > In this case dev cannot be NULL, so remove redundant check. > > > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > > > > > Fixes: 03fd3cf5a179 ("can: add driver for Softing card") > > IMHO this is simply an improvement which is done by all patches applied to > the kernel but it does not really "fix" anything from a functional > standpoint. > > Shouldn't we either invent a new tag or better leave it out to not confuse > the stable maintainers? Hi Oliver, sorry for missing that in my review. Yes, I agree that this is probably not a fix, for which my rule of thumb is something that addresses a user-visible problem. So I agree it should not have a fixes tag. I would suggest that we can just change the text to something that has no tag. Something like: ... Introduced by 03fd3cf5a179 ("can: add driver for Softing card") Signed-of-by: ... > > Best regards, > Oliver > > > > Signed-off-by: Daniil Dulov <d.dulov@aladdin.ru> > > > > Hi Daniil, > > > > I am not sure that dev cannot be NULL. > > But I do see that the code assumes it is not, and would crash if it is. > > So I think that, functionally, your statement is correct. > > > > priv = netdev_priv(dev); > > card = priv->card; > > > > Reviewed-by: Simon Horman <horms@kernel.org> > > >
Hi Simon, On 2024-02-19 18:00, Simon Horman wrote: > On Fri, Feb 16, 2024 at 08:47:43PM +0100, Oliver Hartkopp wrote: >> Hi Simon, >> >> I have a general question on the "Fixes:" tag in this patch: >> >> On 16.02.24 18:27, Simon Horman wrote: >>> On Sun, Feb 11, 2024 at 07:05:35AM -0800, Daniil Dulov wrote: >>>> In this case dev cannot be NULL, so remove redundant check. >>>> >>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>> >>>> Fixes: 03fd3cf5a179 ("can: add driver for Softing card") >> >> IMHO this is simply an improvement which is done by all patches applied to >> the kernel but it does not really "fix" anything from a functional >> standpoint. >> >> Shouldn't we either invent a new tag or better leave it out to not confuse >> the stable maintainers? > > Hi Oliver, > > sorry for missing that in my review. > > Yes, I agree that this is probably not a fix, for which my > rule of thumb is something that addresses a user-visible problem. > So I agree it should not have a fixes tag. > > I would suggest that we can just change the text to something that > has no tag. Something like: > > ... > > Introduced by 03fd3cf5a179 ("can: add driver for Softing card") > Yes, but the "Introduced-by:" tag would be an optional tag for people that like blaming others, right? IMHO we should think about completely removing the "Fixes:" tag, when it has no user-visible effect that might be a candidate for stable kernels. It is common improvement work. And it has been so for years. Best regards, Oliver
On Mon, Feb 19, 2024 at 09:37:46PM +0100, Oliver Hartkopp wrote: > Hi Simon, > > On 2024-02-19 18:00, Simon Horman wrote: > > On Fri, Feb 16, 2024 at 08:47:43PM +0100, Oliver Hartkopp wrote: > > > Hi Simon, > > > > > > I have a general question on the "Fixes:" tag in this patch: > > > > > > On 16.02.24 18:27, Simon Horman wrote: > > > > On Sun, Feb 11, 2024 at 07:05:35AM -0800, Daniil Dulov wrote: > > > > > In this case dev cannot be NULL, so remove redundant check. > > > > > > > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > > > > > > > > > Fixes: 03fd3cf5a179 ("can: add driver for Softing card") > > > > > > IMHO this is simply an improvement which is done by all patches applied to > > > the kernel but it does not really "fix" anything from a functional > > > standpoint. > > > > > > Shouldn't we either invent a new tag or better leave it out to not confuse > > > the stable maintainers? > > > > Hi Oliver, > > > > sorry for missing that in my review. > > > > Yes, I agree that this is probably not a fix, for which my > > rule of thumb is something that addresses a user-visible problem. > > So I agree it should not have a fixes tag. > > > > I would suggest that we can just change the text to something that > > has no tag. Something like: > > > > ... > > > > Introduced by 03fd3cf5a179 ("can: add driver for Softing card") > > > > Yes, but the "Introduced-by:" tag would be an optional tag for people that > like blaming others, right? Yes, That does seem useful to me. > IMHO we should think about completely removing the "Fixes:" tag, when it has > no user-visible effect that might be a candidate for stable kernels. It is > common improvement work. And it has been so for years. Likewise, that does sound like a good idea to me.
diff --git a/drivers/net/can/softing/softing_fw.c b/drivers/net/can/softing/softing_fw.c index bad69a4abec1..5a3f9e4b0b62 100644 --- a/drivers/net/can/softing/softing_fw.c +++ b/drivers/net/can/softing/softing_fw.c @@ -436,7 +436,7 @@ int softing_startstop(struct net_device *dev, int up) return ret; bus_bitmask_start = 0; - if (dev && up) + if (up) /* prepare to start this bus as well */ bus_bitmask_start |= (1 << priv->index); /* bring netdevs down */
In this case dev cannot be NULL, so remove redundant check. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 03fd3cf5a179 ("can: add driver for Softing card") Signed-off-by: Daniil Dulov <d.dulov@aladdin.ru> --- drivers/net/can/softing/softing_fw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)