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

[Varadarajan Narayanan]

On Fri, Jul 03, 2026 at 12:37:04PM +0200, Quentin Schulz wrote:
> 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.

Not posting the next version since this is getting reverted per [1].
Please let me know if I have missed something.

Thanks
Varada

1 - https://lore.kernel.org/u-boot/20260703-revert-sysreset-v2-1-ac999d2ff82f@cherry.de/