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

[Varadarajan Narayanan]

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);
	}
#endif
	printf("resetting ...\n");
	mdelay(100);

	sysreset_walk_halt(reset_type);

	return 0;
}

If this seems ok, will post v2 with this change.

Thanks
Varada