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).
Cheers,
Quentin
