[PATCH] fs: semihosting: Use correct variable for error check

[Sean Anderson]

On 10/17/25 14:00, Heinrich Schuchardt wrote:
> On 10/17/25 19:08, Sean Anderson wrote:
>> On 10/15/25 12:28, Tom Rini wrote:
>>> On Thu, Oct 02, 2025 at 10:36:07AM -0400, Sean Anderson wrote:
>>>> On 10/2/25 05:52, Heinrich Schuchardt wrote:
>>>>> On 10/2/25 11:39, Andrew Goodbody wrote:
>>>>>> After calling a function that can return an error, the test to detect
>>>>>> that error should use the return value not a different variable. Fix it.
>>>>>>
>>>>>> This issue was found by Smatch.
>>>>>>
>>>>>
>>>>> Fixes: f676b45151c3 ("fs: Add semihosting filesystem")
>>>>>
>>>>>> Signed-off-by: Andrew Goodbody <andrew.goodbody at linaro.org>
>>>>>> ---
>>>>>>    fs/semihostingfs.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/semihostingfs.c b/fs/semihostingfs.c
>>>>>> index 77e39ca407e4d240a1fd573497c5b6b908816454..9d7a136b9ba9b035545b34b31df58e2d65de7db9 100644
>>>>>> --- a/fs/semihostingfs.c
>>>>>> +++ b/fs/semihostingfs.c
>>>>>> @@ -35,7 +35,7 @@ static int smh_fs_read_at(const char *filename, loff_t pos, void *buffer,
>>>>>>        }
>>>>>>        if (!maxsize) {
>>>>>>            size = smh_flen(fd);
>>>>>> -        if (ret < 0) {
>>>>>> +        if (size < 0) {
>>>>>
>>>>> The ARM specification (https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fdeveloper.arm.com%2fdocumentation%2fdui0203%2fj%2fsemihosting%2fsemihosting%2doperations%2fsys%2dflen%2d%2d0x0c%2d&umid=f17ab8fc-2d9d-4cce-91e2-e7e66f117613&rct=1759398729&auth=d807158c60b7d2502abde8a2fc01f40662980862-e0799d2d4d61f93cae033d54733c8fa50ef84918) has:
>>>>>
>>>>> SYS_FLEN (0x0C)
>>>>>
>>>>> Returns the length of a specified file.
>>>>> On exit, R0 contains:
>>>>>      the current length of the file object, if the call is successful
>>>>>      -1 if an error occurs.
>>>>>
>>>>> Please, consider that the file length on 32bit systems may exceed 2^31 You must not consider this as an error.
>>>>>
>>>>> %s/if (size < 0)/if (size == -1L)/
>>>>
>>>> This cannot occur because on a 32-bit system SYS_FLEN only returns
>>>> 32-bits of information. The host must detect files that exceed 2 GiB in
>>>> size and return -1 (simulating EOVERFLOW).
>>>
>>> OK, but this is for 64-bit systems too, isn't it?
>>>
>>
>> Sorry, I worded that poorly. What I mean is that the spec says
>>
>> | On exit, R0 contains:
>> |     the current length of the file object, if the call is successful
>> |     -1 if an error occurs.
>>
>> which implies that R0 is a signed integer (otherwise they would have
> 
> The only thing that is certain here is that the register will be filled with (unsigned long)-1L if an error occurs and that any other value is not an error.

It says that any other value is the current length of the file object.

And we should not support negative lengths.

> Everything else is your personal interpretation which does not match the existing implementation by Linaro in QEMU.
> 
>> specified 0xffffffff or 0xffffffffffffffff as the error value). So
>> negative values (other than -1) indicate a negative file size (or are
>> unspecified).
>>
>> However, both OpenOCD and QEMU just take the return from stat(2) and
>> copy it into R0. With a 32-bit host there is no problem since stat(2)
>> will itself fail for files over 2 GiB. With a 32-bit target, this
>> produces erroneous values for files larger than 4 GiB. Files sizes
>> between 2 and 4 GiB could be recovered, but I don't think we should do
>> that. On a 64-bit target, there is again no ambiguity (at least until 8
> 
> You still don't provide any reason why you don't want to support files in the range [2 GiB, 4GiB[ on a 32 bit system.
> 
> Instead of restricting U-Boot we should add a fix in QEMU to report an error if the filesize exceeds ULONG_MAX - 1.
> 
> This is the only way we can ensure that a 5 GiB file is not reported as 1 GiB in U-Boot as we see it today.

https://review.openocd.org/c/openocd/+/9173
https://lore.kernel.org/qemu-devel/20251017213529.998267-1-sean.anderson@linux.dev

I think the latter may be held up on the list, but hopefully it will arrive on lore soon.

--Sean