Hi Casey,
Thanks for the prompt answer!
On 7/3/26 2:27 PM, Casey Connolly wrote:
Hi Quentin,
On 7/2/26 20:27, Quentin Schulz wrote:
The args support for the sysreset uclass contains a logic bug. The first
sysreset device implementing the request_arg callback will consume the
args, not support the specified arg and thus return -EPROTONOSUPPORT
which will stop the iteration over all sysreset devices.
This is an issue if one has multiple sysreset devices and each with
support for different (valid) args. If a sysreset device implements a
-dummy argument and another -foo and one calls reset -dummy on the
U-Boot CLI, it'll depend on which sysreset device will be attempted
first. If it is the one implementing -foo, it'll return it doesn't
support the argument with -EPROTONOSUPPORT in which case the device
implementing -dummy will never be attempted and instead we'll do a cold
reset which is very likely not what's expected.
I suggest to either add an additional return code for the case where the
sysreset device knows it isn't supposed to handle this argument because
it isn't among the ones it supports, and thus we try other sysreset
devices until we find one that says it tried and either is in progress
or couldn't realize the request, or no sysreset device supports this
argument and we end up doing a cold reset (but should we even try to do
a cold reset if the type of reset requested by the user isn't
supported?). Or just don't try piggy-backing on the reset command and
have a new command instead?
Maybe I misunderstood something or misread the code, so please correct
me if I'm wrong.
We've tried bringing up this issue multiple times over the last
month[1][2][3] but it seems like we're left on read. This is the last
resort to get attention on this patches, hopefully we don't have to get
through a revert a few days before the release though there is only two
working days left before the release. Sending a revert as requested by
Tom[4].
I'm so sorry I missed this feedback. That really isn't great.
It happens, I'm glad you see it now :)
I'm with you that this is a pretty fundamental issue with the
implementation here and I think the best course of action is to revert
it and come up with a more suitable approach that isn't so open to
trivial logic bugs like this, likely with some deeper refactoring of
sysreset/reboot-mode code in U-Boot.
In hindsight I think I should have pushed back harder on this
implementation, this is something I'll be more cautious of in the future.
Just to be clear I wasn't trying to shame anyone here, contributor,
reviewer or maintainer. Mistakes happen.
Acked-by: Casey Connolly <[email protected]>
I don't have a chance today to test this today as I'm OOO but if it
doesn't break other platforms then it ought to be fine on Qualcomm too,
maybe Neil can give it a spin on 8650.
Tom: feel free to pick this up if you're happy with it.
Varada sent a series trying to fix the logic, c.f.
https://lore.kernel.org/u-boot/[email protected]/T/#mf098752c30cd6a6c8e7700f61cd54cbc8f307d6d.
But if you feel like we should start from a clean slate again instead
then we can revert this now and take the time to implement this
differently for next release. I think your feedback on what to do next
is important here. But I appreciate you're OOO so we'll do without if
you don't have time :)
Thanks,
Quentin
