Le 04/10/2024 à 13:37, Simon Horman 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.
Hi Christophe,
I think we can say "'ret' will be 0".
Agreed.
ret = adin1110_read_reg()
--> spi_sync_transfer()
--> spi_sync()
which explicitly documents "zero on success, else a negative error code."
At least that is what my brief investigation tells me.
Return -EINVAL instead.
If the patch is considered as correct, can you confirm that -EINVAL is
the correct error code to use? If not, which one would be preferred?
Please include some information on how this was found and tested.
e.g.
Found by inspection / Found using widget-ng.
I would say: found by luck! :)
The explanation below will be of no help in the commit message and won't
be added. I just give you all the gory details because you asked for it ;-)
(and after reading bellow, you can call me crazy!)
I was looking at functions that propagate error codes as their last
argument. The idea came after submitting [1].
I read cci_read() and wondered if functions with such a semantic could
use an un-initialized last argument. In such a case, this function could
not behave as expected if the initial value of "err" was not 0.
So I wrote the following coccinelle script and several other variations.
// Options: --include-headers
@ok@
identifier fct, err;
type T;
@@
int fct(..., T *err)
{
...
}
@test depends on ok@
identifier x, fct = ok.fct;
expression res;
type T = ok.T;
@@
* T x;
...
(
fct(..., &x);
|
res = fct(..., &x);
)
(For the record, I have not found any issue with it...)
BUT, adin1110_read_fifo() was spotted because of the prototype of
adin1110_read_reg().
When I reviewed the code, I quickly saw that it was a false positive and
that using "type T" in my script was not that logical...
Anyway, when reviewing the code, I saw:
if (ret < 0)
return ret;
/* The read frame size includes the extra 2 bytes
* from the ADIN1110 frame header.
*/
if (frame_size < ADIN1110_FRAME_HEADER_LEN + ADIN1110_FEC_LEN)
return ret;
round_len = adin1110_round_len(frame_size);
if (round_len < 0)
return ret;
which looks really strange and likely broken...
Then I sent the patch we are talking about!
(yes some real people really search such things and write such
coccinelle scripts, and now you can call me crazy)
[1]:
https://lore.kernel.org/all/666ac169157f0af1c2e1d47926b68870cb39d587.1727977974.git.christophe.jaillet@xxxxxxxxxx/
Compile tested only.
As a "speculative" patch, it was only compile tested, you are correct.
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.
In my brief investigation I see that adin1110_read_fifo()
is only called by adin1110_read_frames(), like this:
while (budget) {
...
ret = adin1110_read_fifo(port_priv);
if (ret < 0)
return;
budget--;
}
So the question becomes, should a failure in reading the fifo,
because of an invalid frame size, be treated as an error
and terminate reading frames.
Like you, I speculate the answer is yes.
But I think we need a bit more certainty to take this patch.
I won't be of any help here.
I can just say that "it looks strange" and is "certainly" bogus, but
won't be able the prove it nor test it.
I'll wait a bit before sending a v2. If confirming this point is a
requirement for accepting the patch, there is no need to urge for a v2
if no-one cares about answering your point.
CJ