[PATCH] drivers: sysreset: Do not end sysreset_walk_arg on -EPROTONOSUPPORT

[Quentin Schulz]

Hi Varada,

On 7/3/26 12:27 PM, Varadarajan Narayanan wrote:
> On Fri, Jul 03, 2026 at 12:11:30PM +0200, Quentin Schulz wrote:
>> Hi Varada,
>>
>> Thanks for the patch!
>>
>> On 7/3/26 9:24 AM, Varadarajan Narayanan wrote:
>>> If there are multiple sysreset devices implementing request_arg callback,
>>> the first sysreset device will consume the args and may return
>>> EPROTONOSUPPORT if it doesn't implement the given argument. This will stop
>>> the loop.
>>>
>>> Since -EPROTONOSUPPORT is used to indicate absence of support for that
>>> argument, subsequent drivers should be given a chance to see if they handle
>>> it. Hence do not terminate the loop on -EPROTONOSUPPORT return code.
>>>
>>> Fixes: fcb48b89813b ("drivers: sysreset: Add sysreset op that can take arguments")
>>>
>>> Signed-off-by: Varadarajan Narayanan <varadarajan.narayanan at oss.qualcomm.com>
>>> ---
>>>    drivers/sysreset/sysreset-uclass.c | 23 +++++++++++++----------
>>>    1 file changed, 13 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/sysreset/sysreset-uclass.c b/drivers/sysreset/sysreset-uclass.c
>>> index f25e09e9cd0..0fc096e7f0f 100644
>>> --- a/drivers/sysreset/sysreset-uclass.c
>>> +++ b/drivers/sysreset/sysreset-uclass.c
>>> @@ -89,14 +89,12 @@ int sysreset_walk_arg(int argc, char * const argv[])
>>>    	struct udevice *dev;
>>>    	int ret = -ENOSYS;
>>> -	while (ret != -EINPROGRESS && ret != -EPROTONOSUPPORT) {
>>> -		for (uclass_first_device(UCLASS_SYSRESET, &dev);
>>> -		     dev;
>>> -		     uclass_next_device(&dev)) {
>>> -			ret = sysreset_request_arg(dev, argc, argv);
>>> -			if (ret == -EINPROGRESS || ret == -EPROTONOSUPPORT)
>>> -				break;
>>> -		}
>>> +	for (uclass_first_device(UCLASS_SYSRESET, &dev);
>>> +	     dev;
>>> +	     uclass_next_device(&dev)) {
>>> +		ret = sysreset_request_arg(dev, argc, argv);
>>> +		if (ret == -EINPROGRESS)
>>> +			break;
>>>    	}
>>>    	return ret;
>>> @@ -153,6 +151,7 @@ void reset_cpu(void)
>>>    int do_reset(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>>    {
>>>    	enum sysreset_t reset_type = SYSRESET_COLD;
>>> +	int ret;
>>>    	if (argc > 2)
>>>    		return CMD_RET_USAGE;
>>> @@ -165,8 +164,12 @@ int do_reset(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>>    	mdelay(100);
>>>    #if IS_ENABLED(CONFIG_SYSRESET_CMD_RESET_ARGS)
>>> -	if (argc > 1 && sysreset_walk_arg(argc, argv) == -EINPROGRESS)
>>> -		return 0;
>>> +	if (argc > 1) {
>>> +		ret = sysreset_walk_arg(argc, argv);
>>> +		if (ret == -EINPROGRESS)
>>> +			return 0;
>>> +		log_err("No handler for reset command arguments (%d)\n", ret);
>>
>> Just use printf to be consistent with the rest of the function.
>>
>> But here's possibly another logic bug I think. If we pass -w to reset, this
>> will try all available sysreset devices if any can handle the -w argument
>> and then print that there's no handler for the reset command argument, which
>> is to be expected.
>>
>> So I'm wondering if we shouldn't bypass sysreset_walk_arg() entirely when -w
>> is given as argument to the reset command.
>>
>> Also, *any* argument starting with -w should do the warm reset, e.g. reset
>> -warm should do it too as that's the current logic we have (we only check
>> the first two characters of the argument).
> 
> Would this be ok?
> 
> int do_reset(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> {
> 	enum sysreset_t reset_type = SYSRESET_COLD;
> 	int ret;
> 
> 	if (argc > 2)
> 		return CMD_RET_USAGE;
> 
> 	if (argc == 2 && argv[1][0] == '-' && argv[1][1] == 'w') {
> 		reset_type = SYSRESET_WARM;
> 	}
> #if IS_ENABLED(CONFIG_SYSRESET_CMD_RESET_ARGS)
> 	else if (argc > 1) {
> 		ret = sysreset_walk_arg(argc, argv);
> 		if (ret == -EINPROGRESS)
> 			return 0;
> 		printf("No handler for reset command arguments (%d)\n", ret);

I don't think printing ret is interesting here as it'll be the return 
code of the last attempted sysreset device (so either ENOSYS if there is 
no sysreset device at all, or if the last one doesn't support 
request_arg callback, or -EPROTONOSUPPORT if it supports args but not 
the arg we pass it).

We should also tell the user what we're doing in this case: a cold reset.

> 	}
> #endif
> 	printf("resetting ...\n");
> 	mdelay(100);
> 
> 	sysreset_walk_halt(reset_type);
> 
> 	return 0;
> }
> 
> If this seems ok, will post v2 with this change.
> 

No, because it'll do the reset before printing it's doing a reset (and 
will also not do the delay).

But if you move the whole if block after the mdelay() then I think it's 
fine.

Cheers,
Quentin