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
> > <[email protected]>
> > ---
> > 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
