Le 04/10/2024 à 13:47, Dan Carpenter a écrit :
On Thu, Oct 03, 2024 at 08:53:15PM +0200, Christophe JAILLET wrote:
If 'frame_size' is too small or if 'round_len' is an error code, it is
likely that an error code should be returned to the caller.
Actually, 'ret' is likely to be 0, so if one of these sanity checks fails,
'success' is returned.
Return -EINVAL instead.
Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
---
This patch is speculative.
If returning 0 is what was intended, then an explicit 0 would be better.
I have an unpublished Smatch warning for these:
drivers/net/ethernet/adi/adin1110.c:321 adin1110_read_fifo() info: returning a literal zero is cleaner
drivers/net/ethernet/adi/adin1110.c:325 adin1110_read_fifo() info: returning a literal zero is cleaner
It's a pity that deliberately doing a "return ret;" when ret is zero is so
common. Someone explained to me that it was "done deliberately to express that
we were propagating the success from frob_whatever()". No no no!
I don't review these warnings unless I'm fixing a bug in the driver because
they're too common. The only ones I review are:
ret = frob();
if (!ret)
return ret;
Maybe 20% of the time those warnings indicate a reversed if statement.
Your heuristic here is very clever and I'll try steal it to create a new more
specific warning.
Well my heuristic is mostly luck, here ;-)
Anyway, what I was looking for (un-initialized last argument in some
function) could be nice to have in smatch, if not already present.
Finally, if you want, I can give a look at the warnings if you share the
log.
CJ
regards,
dan carpenter
